Re: [PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
On 29.08.23 16:35, Jagan Teki wrote: On Sun, Aug 27, 2023 at 12:03 AM Mimoja wrote: I appreciate you taking the time to respond! On 26.08.23 17:18, Marek Vasut wrote: On 8/26/23 11:55, Mimoja wrote: "The .prepare() function is typically called before the display controller starts to transmit video data." and "After the display controller has started transmitting video data, it's safe to call the .enable() function." DSI commands are not DSI video, so this should be OK ? You are correct, my commit message is mixing things up here. I wanted to emphasize roughly the thought of "when enable() is called the dsi core is expected to have its clock initialized". Will take note to clarify this if I succeed to make a case for this patch below :) While generally fine this can lead to a fillup of the transmission queue before the transmission is set up on certain dsi bridges. This issue can also be seen on downstream imx8m* kernels. Can you reproduce this with current mainline Linux or linux-next tree ? I recall the display pipeline in the NXP downstream stuff is very different from mainline . You are very much correct. The NXP downstream kernel is completely different from the upstream one and is really a great example to show the issue (code cleaned up for readability): https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/drivers/gpu/drm/bridge/sec-dsim.c#L1368 ``` ret = drm_panel_prepare(dsim->panel); if (unlikely(ret)) [...] /* config esc clock, byte clock and etc */ sec_mipi_dsim_config_clkctrl(dsim); ret = drm_panel_enable(dsim->panel); if (unlikely(ret)) [...] ``` Which SoC does have this problem ? Sadly I don't have any SoCs available which would work perfectly with linux-next, let alone are confirmed affected :/ I were able to make my Kingway Panel work (Custom one and so far unsupported by the st7701 driver) with this patch on downstream 5.4 and 5.15 imx8mn as well as on a raspberry pi CM4 with 6.1. However raspberrypi/linux brings SPI support to the st7701 driver which should not affect this but I would just like to document it here. I could not find any success story with st7701 and the rpi on 6.1 online after a short search (and only one reference with 5.10 which seems to me a bit different in a short comparison) but again I can only offer circumstantial evidence. Sorry :/ If I understand correctly, 5.10 and 5.15 Would work as it is if the DSI host calls the panel's prepare and enable directly from encoder enable. Did you check that? No, I fear the downstream NXP driver is not following the correct / expected calling flow which upstream has implemented. After investigating this and the RPi issue I would like to follow Marek's recommendation and simply sunset this change in favor of fixing the downstream issues in the downstream tree(s). This seems to be mainly an issue for vendor-custom downstream host kernels, and while there might be a case of convention where panels don't sent commands in prepare() and only in enable() I don't think this patch should have a future solely to be cherry-picked into downstream kernels, when we can not make a case for current linux/next. After all: It is now on the internet for better or worse for others to pick up if needed. I appreciate you all taking the time to discuss and I will post the enablement code for the Kingway HW-021P0Z002-01 panel I tested this with in a bit. Cheers and thankful regards Mimoja
Re: [PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
On Sun, Aug 27, 2023 at 12:03 AM Mimoja wrote: > > I appreciate you taking the time to respond! > > On 26.08.23 17:18, Marek Vasut wrote: > > On 8/26/23 11:55, Mimoja wrote: > >> "The .prepare() function is typically called before the display > >> controller > >> starts to transmit video data." > >> and > >> "After the display controller has started transmitting video data, > >> it's safe > >> to call the .enable() function." > > > > DSI commands are not DSI video, so this should be OK ? > > You are correct, my commit message is mixing things up here. I wanted to > emphasize roughly the thought of > "when enable() is called the dsi core is expected to have its clock > initialized". Will take note to clarify this if I succeed to > make a case for this patch below :) > > >> While generally fine this can lead to a fillup of the transmission > >> queue before > >> the transmission is set up on certain dsi bridges. > >> This issue can also be seen on downstream imx8m* kernels. > > > > Can you reproduce this with current mainline Linux or linux-next tree ? > > I recall the display pipeline in the NXP downstream stuff is very > > different from mainline . > > You are very much correct. The NXP downstream kernel is completely > different from the upstream one > and is really a great example to show the issue (code cleaned up for > readability): > > https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/drivers/gpu/drm/bridge/sec-dsim.c#L1368 > ``` > ret = drm_panel_prepare(dsim->panel); > if (unlikely(ret)) [...] > > /* config esc clock, byte clock and etc */ > sec_mipi_dsim_config_clkctrl(dsim); > > ret = drm_panel_enable(dsim->panel); > if (unlikely(ret)) [...] > > ``` > > > Which SoC does have this problem ? > Sadly I don't have any SoCs available which would work perfectly with > linux-next, let alone are confirmed affected :/ > > I were able to make my Kingway Panel work (Custom one and so far > unsupported by the st7701 driver) with this > patch on downstream 5.4 and 5.15 imx8mn as well as on a raspberry pi CM4 > with 6.1. However raspberrypi/linux brings > SPI support to the st7701 driver which should not affect this but I > would just like to document it here. > I could not find any success story with st7701 and the rpi on 6.1 online > after a short search (and only one > reference with 5.10 which seems to me a bit different in a short > comparison) but again I can only offer > circumstantial evidence. Sorry :/ If I understand correctly, 5.10 and 5.15 Would work as it is if the DSI host calls the panel's prepare and enable directly from encoder enable. Did you check that? Thanks, Jagan.
Re: [PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
Hi Marek & Mimoja On Sat, 26 Aug 2023 at 22:02, Marek Vasut wrote: > > On 8/26/23 20:33, Mimoja wrote: > > Hi, > > +CC Dave , he might be able to help with the last part. > > > I appreciate you taking the time to respond! > > > > On 26.08.23 17:18, Marek Vasut wrote: > >> On 8/26/23 11:55, Mimoja wrote: > >>> "The .prepare() function is typically called before the display > >>> controller > >>> starts to transmit video data." > >>> and > >>> "After the display controller has started transmitting video data, > >>> it's safe > >>> to call the .enable() function." > >> > >> DSI commands are not DSI video, so this should be OK ? > > > > You are correct, my commit message is mixing things up here. I wanted to > > emphasize roughly the thought of > > "when enable() is called the dsi core is expected to have its clock > > initialized". Will take note to clarify this if I succeed to > > make a case for this patch below :) > > I vaguely recall there was this flag in include/drm/drm_bridge.h which > might be related: > > 748 /** > 749 * @pre_enable_prev_first: The bridge requires that the prev > 750 * bridge @pre_enable function is called before its @pre_enable, > 751 * and conversely for post_disable. This is most frequently a > 752 * requirement for DSI devices which need the host to be initialised > 753 * before the peripheral. > 754 */ > 755 bool pre_enable_prev_first; > > Could it be, this is what you need ? drm_panel has prepare_prev_first, which maps to drm_bridge's pre_enable_prev_first, but same concept. Most likely this is what you're after, but only got added in 6.3. > >>> While generally fine this can lead to a fillup of the transmission > >>> queue before > >>> the transmission is set up on certain dsi bridges. > >>> This issue can also be seen on downstream imx8m* kernels. > >> > >> Can you reproduce this with current mainline Linux or linux-next tree ? > >> I recall the display pipeline in the NXP downstream stuff is very > >> different from mainline . > > > > You are very much correct. The NXP downstream kernel is completely > > different from the upstream one > > and is really a great example to show the issue (code cleaned up for > > readability): > > > > https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/drivers/gpu/drm/bridge/sec-dsim.c#L1368 > > ``` > > ret = drm_panel_prepare(dsim->panel); > > if (unlikely(ret)) [...] > > > > /* config esc clock, byte clock and etc */ > > sec_mipi_dsim_config_clkctrl(dsim); > > > > ret = drm_panel_enable(dsim->panel); > > if (unlikely(ret)) [...] > > > > ``` That DSI host driver looks very strange, or perhaps just outdated. It implements atomic_enable and atomic_disable, but not atomic_pre_enable or atomic_post_disable. Any attached panel or bridge drivers therefore don't get called in the expected way by the bridge chain. You are on 5.15 which may predate some of the reworking, but should still support panel_bridge so that calling one of the of_get_bridge functions gives you a bridge irregardless of whether it is a bridge or panel. I'd question why the DSI host driver is making calls down the bridge chain for itself - either it results in calling functions multiple times, or you break the bridge chain (as vc4 used to do) but mess up atomic states as the core can't add the state of the removed nodes. > > > >> Which SoC does have this problem ? > > Sadly I don't have any SoCs available which would work perfectly with > > linux-next, let alone are confirmed affected :/ > > > > I were able to make my Kingway Panel work (Custom one and so far > > unsupported by the st7701 driver) with this > > patch on downstream 5.4 and 5.15 imx8mn as well as on a raspberry pi CM4 > > with 6.1. However raspberrypi/linux brings > > SPI support to the st7701 driver which should not affect this but I > > would just like to document it here. DPI video with SPI for configuration was added to the Pi kernel to support the Pimoroni HyperPixel 2 round display[1]. If you have the panel attached to a CM4, and prepare_prev_first is set, then I would expect it to work on the Pi. The docs[2] state that transfer can be called in any state, however I know that is an issue in vc4 that I need to address at some point. If fixed, then no change should be required. [1] https://shop.pimoroni.com/products/hyperpixel-round?variant=39381081882707 [2] https://github.com/torvalds/linux/blob/master/include/drm/drm_mipi_dsi.h#L84-L87 > > I could not find any success story with st7701 and the rpi on 6.1 online > > after a short search (and only one > > reference with 5.10 which seems to me a bit different in a short > > comparison) but again I can only offer > > circumstantial evidence. Sorry :/ > > Maybe Dave can help with this part . I don't recall having had an ST7701 panel using DSI, so I'm afraid I can't really help there. Dave
[PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
The struct drm_panel_funcs are offering a prepare() and an enable()
entrypoint for panels. According to drm/panel.h:
"The .prepare() function is typically called before the display controller
starts to transmit video data."
and
"After the display controller has started transmitting video data, it's safe
to call the .enable() function."
The st7701 driver currently does not respect this, queing DSI control commands
during enable.
While generally fine this can lead to a fillup of the transmission queue before
the transmission is set up on certain dsi bridges.
This issue can also be seen on downstream imx8m* kernels.
By moving the init sequence into the enable function we not only circumvent the
issue
but also properly soft-reset the panel on enable().
Signed-off-by: Mimoja
Cc: Marek Vasut
Cc: Guido Günther
Cc: Jagan Teki
Cc: Laurent Pinchart
Cc: Linus Walleij
Cc: Sam Ravnborg
Cc: Thierry Reding
---
drivers/gpu/drm/panel/panel-sitronix-st7701.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c
b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
index 7eae83aa0ea1..18c5a8d97cc8 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7701.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
@@ -439,6 +439,13 @@ static int st7701_prepare(struct drm_panel *panel)
gpiod_set_value(st7701->reset, 1);
msleep(150);
+ return 0;
+}
+
+static int st7701_enable(struct drm_panel *panel)
+{
+ struct st7701 *st7701 = panel_to_st7701(panel);
+
st7701_init_sequence(st7701);
if (st7701->desc->gip_sequence)
@@ -447,13 +454,6 @@ static int st7701_prepare(struct drm_panel *panel)
/* Disable Command2 */
st7701_switch_cmd_bkx(st7701, false, 0);
- return 0;
-}
-
-static int st7701_enable(struct drm_panel *panel)
-{
- struct st7701 *st7701 = panel_to_st7701(panel);
-
ST7701_DSI(st7701, MIPI_DCS_SET_DISPLAY_ON, 0x00);
return 0;
--
2.39.2
Re: [PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
I appreciate you taking the time to respond! On 26.08.23 17:18, Marek Vasut wrote: On 8/26/23 11:55, Mimoja wrote: "The .prepare() function is typically called before the display controller starts to transmit video data." and "After the display controller has started transmitting video data, it's safe to call the .enable() function." DSI commands are not DSI video, so this should be OK ? You are correct, my commit message is mixing things up here. I wanted to emphasize roughly the thought of "when enable() is called the dsi core is expected to have its clock initialized". Will take note to clarify this if I succeed to make a case for this patch below :) While generally fine this can lead to a fillup of the transmission queue before the transmission is set up on certain dsi bridges. This issue can also be seen on downstream imx8m* kernels. Can you reproduce this with current mainline Linux or linux-next tree ? I recall the display pipeline in the NXP downstream stuff is very different from mainline . You are very much correct. The NXP downstream kernel is completely different from the upstream one and is really a great example to show the issue (code cleaned up for readability): https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/drivers/gpu/drm/bridge/sec-dsim.c#L1368 ``` ret = drm_panel_prepare(dsim->panel); if (unlikely(ret)) [...] /* config esc clock, byte clock and etc */ sec_mipi_dsim_config_clkctrl(dsim); ret = drm_panel_enable(dsim->panel); if (unlikely(ret)) [...] ``` Which SoC does have this problem ? Sadly I don't have any SoCs available which would work perfectly with linux-next, let alone are confirmed affected :/ I were able to make my Kingway Panel work (Custom one and so far unsupported by the st7701 driver) with this patch on downstream 5.4 and 5.15 imx8mn as well as on a raspberry pi CM4 with 6.1. However raspberrypi/linux brings SPI support to the st7701 driver which should not affect this but I would just like to document it here. I could not find any success story with st7701 and the rpi on 6.1 online after a short search (and only one reference with 5.10 which seems to me a bit different in a short comparison) but again I can only offer circumstantial evidence. Sorry :/ Thank you again ~Mimoja
Re: [PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
On 8/26/23 20:33, Mimoja wrote: Hi, +CC Dave , he might be able to help with the last part. I appreciate you taking the time to respond! On 26.08.23 17:18, Marek Vasut wrote: On 8/26/23 11:55, Mimoja wrote: "The .prepare() function is typically called before the display controller starts to transmit video data." and "After the display controller has started transmitting video data, it's safe to call the .enable() function." DSI commands are not DSI video, so this should be OK ? You are correct, my commit message is mixing things up here. I wanted to emphasize roughly the thought of "when enable() is called the dsi core is expected to have its clock initialized". Will take note to clarify this if I succeed to make a case for this patch below :) I vaguely recall there was this flag in include/drm/drm_bridge.h which might be related: 748 /** 749 * @pre_enable_prev_first: The bridge requires that the prev 750 * bridge @pre_enable function is called before its @pre_enable, 751 * and conversely for post_disable. This is most frequently a 752 * requirement for DSI devices which need the host to be initialised 753 * before the peripheral. 754 */ 755 bool pre_enable_prev_first; Could it be, this is what you need ? While generally fine this can lead to a fillup of the transmission queue before the transmission is set up on certain dsi bridges. This issue can also be seen on downstream imx8m* kernels. Can you reproduce this with current mainline Linux or linux-next tree ? I recall the display pipeline in the NXP downstream stuff is very different from mainline . You are very much correct. The NXP downstream kernel is completely different from the upstream one and is really a great example to show the issue (code cleaned up for readability): https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/drivers/gpu/drm/bridge/sec-dsim.c#L1368 ``` ret = drm_panel_prepare(dsim->panel); if (unlikely(ret)) [...] /* config esc clock, byte clock and etc */ sec_mipi_dsim_config_clkctrl(dsim); ret = drm_panel_enable(dsim->panel); if (unlikely(ret)) [...] ``` Which SoC does have this problem ? Sadly I don't have any SoCs available which would work perfectly with linux-next, let alone are confirmed affected :/ I were able to make my Kingway Panel work (Custom one and so far unsupported by the st7701 driver) with this patch on downstream 5.4 and 5.15 imx8mn as well as on a raspberry pi CM4 with 6.1. However raspberrypi/linux brings SPI support to the st7701 driver which should not affect this but I would just like to document it here. I could not find any success story with st7701 and the rpi on 6.1 online after a short search (and only one reference with 5.10 which seems to me a bit different in a short comparison) but again I can only offer circumstantial evidence. Sorry :/ Maybe Dave can help with this part .
Re: [PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()
On 8/26/23 11:55, Mimoja wrote: The struct drm_panel_funcs are offering a prepare() and an enable() entrypoint for panels. According to drm/panel.h: "The .prepare() function is typically called before the display controller starts to transmit video data." and "After the display controller has started transmitting video data, it's safe to call the .enable() function." The st7701 driver currently does not respect this, queing DSI control commands during enable. DSI commands are not DSI video, so this should be OK ? While generally fine this can lead to a fillup of the transmission queue before the transmission is set up on certain dsi bridges. This issue can also be seen on downstream imx8m* kernels. Can you reproduce this with current mainline Linux or linux-next tree ? I recall the display pipeline in the NXP downstream stuff is very different from mainline . Which SoC does have this problem ?
