Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/board/rtl8730e: Update BSP driver irq priority running on cortex-A #6269

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

edwakuwaku
Copy link
Contributor

  • Currently the driver priority setting is according to cortex-M value range (ie. NVIC), but rtl8730e is running on cortex-A, which has it's own value range for setting irq priority (ie. GIC).
  • This PR revise the wrong usage on irq priority for peripheral drivers
  • Due to performance issue, Increase irq priority level for LCDC

@edwakuwaku edwakuwaku changed the title os/board/rtl8730e: Update BSP driver irq priority running on cortex-A [DO NOT MERGE] os/board/rtl8730e: Update BSP driver irq priority running on cortex-A Jul 1, 2024
Copy link
Contributor

@kishore-sn kishore-sn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about other places in code where we are using the old system?
For example amebasmart_serial.c, etc..

@@ -89,7 +89,7 @@ static u8 lcdc_nextframe = 0;
struct irq lcdc_irq_info = {
.num = LCDC_IRQ,
.data = (u32) LCDC,
.priority = INT_PRI_MIDDLE,
.priority = INT_PRI_HIGH,
};

extern struct irq mipi_irq_info;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to increase the priority of mipi also? It is currently set to middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MIPI irq is only used during sending init command table. When running LCD app, only LCD irq is being applied.

@@ -1312,7 +1312,7 @@ BOOL AUDIO_SP_LLPTXGDMA_Init(
u32 pTxData = Lli[0].LliEle.Sarx;

/*obtain a DMA channel and register DMA interrupt handler*/
GdmaChnl = GDMA_ChnlAlloc(0, CallbackFunc, (u32)CallbackData, INT_PRI_MIDDLE);
GdmaChnl = GDMA_ChnlAlloc(0, CallbackFunc, (u32)CallbackData, INT_PRI_HIGH);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we increasing the priority of dma irq? What effect will this have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used by I2S DMA, which is for audio TX (ie. Bixby app). I tried to increase this to see whether there is performance improvement..

@edwakuwaku
Copy link
Contributor Author

How about other places in code where we are using the old system? For example amebasmart_serial.c, etc..

Ya, that should be changed as well... Thank you for reminding

- Currently the driver priority setting is according to cortex-M value range (ie. NVIC), but rtl8730e is running on cortex-A, which has it's own value range for setting irq priority (ie. GIC).
- This PR revise the wrong usage on irq priority for peripheral drivers
- Due to performance issue, Increase irq priority level for LCDC and I2S DMA TX
@r0bindal
Copy link

r0bindal commented Jul 2, 2024

we are getting same performance in both sample app and benchmark app with and without this pr

Sample - 16ms (60fps) measured using rtc
Benchmark - 50fps are shown on UI

@edwakuwaku
Copy link
Contributor Author

Hello @r0bindal , thanks for updating the results.
May I know the LCD result is tested individually without Bixby audio? For this PR, I expect that when LCD + Bixby is up and running together with other driver modules, both LCD and Bixby can be prioritised.
I am not sure whether are you aware of that, but currently the parameter settings for LCD is according to 60FPS. Thus, the test results is making sense to me.

- During SPI read operation, SPI TX sem wait is not triggered, but sem post is being done during TX done irq. Thus, a flag was added to determine whether the TX sem post is needed to be done
@sunghan-chang
Copy link
Contributor

@edwakuwaku We've verified that this and PR #6271 resolves external flash hang. I am going to merge if possible. Is there any reason to mark DO NOT MERGE?

@edwakuwaku
Copy link
Contributor Author

@edwakuwaku We've verified that this and PR #6271 resolves external flash hang. I am going to merge if possible. Is there any reason to mark DO NOT MERGE?

In the second commit of this PR, I would like to check whether it works better if we increase the irq priority for driver used by bixby audio and LCD. But it seems it was not tested together yet.
Anyway, since that will not affect normal operation, it can be merged.

@edwakuwaku edwakuwaku changed the title [DO NOT MERGE] os/board/rtl8730e: Update BSP driver irq priority running on cortex-A os/board/rtl8730e: Update BSP driver irq priority running on cortex-A Jul 3, 2024
@sunghan-chang sunghan-chang merged commit 5e84694 into Samsung:master Jul 3, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants