LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
@ 2019-11-08 21:28 Stephan Gerhold
  2019-11-08 22:12 ` [Freedreno] " Jeffrey Hugo
  0 siblings, 1 reply; 6+ messages in thread
From: Stephan Gerhold @ 2019-11-08 21:28 UTC (permalink / raw)
  To: Rob Clark, Sean Paul
  Cc: David Airlie, Daniel Vetter, Hai Li, linux-arm-msm, dri-devel,
	freedreno, linux-kernel, Stephan Gerhold, Jasper Korten

At the moment, the MSM DSI driver calls drm_panel_enable() rather early
from the DSI bridge pre_enable() function. At this point, the encoder
(e.g. MDP5) is not enabled, so we have not started transmitting
video data.

However, the drm_panel_funcs documentation states that enable()
should be called on the panel *after* video data is being transmitted:

  The .prepare() function is typically called before the display controller
  starts to transmit video data. [...] After the display controller has
  started transmitting video data, it's safe to call the .enable() function.
  This will typically enable the backlight to make the image on screen visible.

Calling drm_panel_enable() too early causes problems for some panels:
The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
backlight/brightness of the screen. The enable sequence is therefore:

  drm_panel_enable()
    drm_panel_funcs.enable():
      backlight_enable()
        backlight_ops.update_status():
          mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);

The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
command if it is sent too early. This prevents setting the initial brightness,
causing the display to be enabled with minimum brightness instead.
Adding various delays in the panel initialization code does not result
in any difference.

On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
fixes the problem, indicating that the panel requires the video stream
to be active before the brightness command is accepted.

Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
delay calling it until video data is being transmitted.

Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
(This is not strictly required for the panel affected above...)

Tested-by: Jasper Korten <jja2000@gmail.com>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Since this is a core change I thought it would be better to send this
early. I believe Jasper still wants to finish some other changes before
submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)

I also tested it on msm8916-samsung-a5u-eur, its display is working fine
with or without this patch.
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 62 ++++++++++++++++++---------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 271aa7bbca92..eea108865cd3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -432,20 +432,8 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 		}
 	}
 
-	if (panel) {
-		ret = drm_panel_enable(panel);
-		if (ret) {
-			pr_err("%s: enable panel %d failed, %d\n", __func__, id,
-									ret);
-			goto panel_en_fail;
-		}
-	}
-
 	return;
 
-panel_en_fail:
-	if (is_dual_dsi && msm_dsi1)
-		msm_dsi_host_disable(msm_dsi1->host);
 host1_en_fail:
 	msm_dsi_host_disable(host);
 host_en_fail:
@@ -464,12 +452,51 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 
 static void dsi_mgr_bridge_enable(struct drm_bridge *bridge)
 {
-	DBG("");
+	int id = dsi_mgr_bridge_get_id(bridge);
+	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+	struct drm_panel *panel = msm_dsi->panel;
+	bool is_dual_dsi = IS_DUAL_DSI();
+	int ret;
+
+	DBG("id=%d", id);
+	if (!msm_dsi_device_connected(msm_dsi))
+		return;
+
+	/* Do nothing with the host if it is slave-DSI in case of dual DSI */
+	if (is_dual_dsi && !IS_MASTER_DSI_LINK(id))
+		return;
+
+	if (panel) {
+		ret = drm_panel_enable(panel);
+		if (ret) {
+			pr_err("%s: enable panel %d failed, %d\n", __func__, id,
+									ret);
+		}
+	}
 }
 
 static void dsi_mgr_bridge_disable(struct drm_bridge *bridge)
 {
-	DBG("");
+	int id = dsi_mgr_bridge_get_id(bridge);
+	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+	struct drm_panel *panel = msm_dsi->panel;
+	bool is_dual_dsi = IS_DUAL_DSI();
+	int ret;
+
+	DBG("id=%d", id);
+	if (!msm_dsi_device_connected(msm_dsi))
+		return;
+
+	/* Do nothing with the host if it is slave-DSI in case of dual DSI */
+	if (is_dual_dsi && !IS_MASTER_DSI_LINK(id))
+		return;
+
+	if (panel) {
+		ret = drm_panel_disable(panel);
+		if (ret)
+			pr_err("%s: Panel %d OFF failed, %d\n", __func__, id,
+									ret);
+	}
 }
 
 static void dsi_mgr_bridge_post_disable(struct drm_bridge *bridge)
@@ -495,13 +522,6 @@ static void dsi_mgr_bridge_post_disable(struct drm_bridge *bridge)
 	if (is_dual_dsi && !IS_MASTER_DSI_LINK(id))
 		goto disable_phy;
 
-	if (panel) {
-		ret = drm_panel_disable(panel);
-		if (ret)
-			pr_err("%s: Panel %d OFF failed, %d\n", __func__, id,
-									ret);
-	}
-
 	ret = msm_dsi_host_disable(host);
 	if (ret)
 		pr_err("%s: host %d disable failed, %d\n", __func__, id, ret);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
  2019-11-08 21:28 [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable() Stephan Gerhold
@ 2019-11-08 22:12 ` " Jeffrey Hugo
  2019-11-08 23:46   ` Stephan Gerhold
  0 siblings, 1 reply; 6+ messages in thread
From: Jeffrey Hugo @ 2019-11-08 22:12 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Rob Clark, Sean Paul, Jasper Korten, Hai Li, David Airlie, MSM,
	lkml, open list:DRM PANEL DRIVERS, Daniel Vetter, freedreno

On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> from the DSI bridge pre_enable() function. At this point, the encoder
> (e.g. MDP5) is not enabled, so we have not started transmitting
> video data.
>
> However, the drm_panel_funcs documentation states that enable()
> should be called on the panel *after* video data is being transmitted:
>
>   The .prepare() function is typically called before the display controller
>   starts to transmit video data. [...] After the display controller has
>   started transmitting video data, it's safe to call the .enable() function.
>   This will typically enable the backlight to make the image on screen visible.
>
> Calling drm_panel_enable() too early causes problems for some panels:
> The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> backlight/brightness of the screen. The enable sequence is therefore:
>
>   drm_panel_enable()
>     drm_panel_funcs.enable():
>       backlight_enable()
>         backlight_ops.update_status():
>           mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
>
> The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> command if it is sent too early. This prevents setting the initial brightness,
> causing the display to be enabled with minimum brightness instead.
> Adding various delays in the panel initialization code does not result
> in any difference.
>
> On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> fixes the problem, indicating that the panel requires the video stream
> to be active before the brightness command is accepted.
>
> Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> delay calling it until video data is being transmitted.
>
> Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> (This is not strictly required for the panel affected above...)
>
> Tested-by: Jasper Korten <jja2000@gmail.com>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Since this is a core change I thought it would be better to send this
> early. I believe Jasper still wants to finish some other changes before
> submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)
>
> I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> with or without this patch.

Nack, please.  I was curious so I threw this on the Lenovo Miix 630
laptop.  I don't get a display back with this patch.  I'll try to
figure out why, but currently I can't get into the machine.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
  2019-11-08 22:12 ` [Freedreno] " Jeffrey Hugo
@ 2019-11-08 23:46   ` Stephan Gerhold
  2019-11-09  3:47     ` Jeffrey Hugo
  0 siblings, 1 reply; 6+ messages in thread
From: Stephan Gerhold @ 2019-11-08 23:46 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Rob Clark, Sean Paul, Jasper Korten, Hai Li, David Airlie, MSM,
	lkml, open list:DRM PANEL DRIVERS, Daniel Vetter, freedreno

On Fri, Nov 08, 2019 at 03:12:28PM -0700, Jeffrey Hugo wrote:
> On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> > from the DSI bridge pre_enable() function. At this point, the encoder
> > (e.g. MDP5) is not enabled, so we have not started transmitting
> > video data.
> >
> > However, the drm_panel_funcs documentation states that enable()
> > should be called on the panel *after* video data is being transmitted:
> >
> >   The .prepare() function is typically called before the display controller
> >   starts to transmit video data. [...] After the display controller has
> >   started transmitting video data, it's safe to call the .enable() function.
> >   This will typically enable the backlight to make the image on screen visible.
> >
> > Calling drm_panel_enable() too early causes problems for some panels:
> > The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> > uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> > backlight/brightness of the screen. The enable sequence is therefore:
> >
> >   drm_panel_enable()
> >     drm_panel_funcs.enable():
> >       backlight_enable()
> >         backlight_ops.update_status():
> >           mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> >
> > The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> > command if it is sent too early. This prevents setting the initial brightness,
> > causing the display to be enabled with minimum brightness instead.
> > Adding various delays in the panel initialization code does not result
> > in any difference.
> >
> > On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> > fixes the problem, indicating that the panel requires the video stream
> > to be active before the brightness command is accepted.
> >
> > Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> > delay calling it until video data is being transmitted.
> >
> > Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> > (This is not strictly required for the panel affected above...)
> >
> > Tested-by: Jasper Korten <jja2000@gmail.com>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > Since this is a core change I thought it would be better to send this
> > early. I believe Jasper still wants to finish some other changes before
> > submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)
> >
> > I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> > with or without this patch.
> 
> Nack, please.  I was curious so I threw this on the Lenovo Miix 630
> laptop.  I don't get a display back with this patch.  I'll try to
> figure out why, but currently I can't get into the machine.

Thanks for testing the patch! Let's try to figure that out...

I'm a bit confused, but this might be because I'm not very familiar with
the MSM8998 laptops. It does not seem to have display in mainline yet,
so do you have a link to all the patches you are using at the moment?

Judging from the patches I was able to find, the Lenovo Miix 630 is
using a DSI to eDP bridge.
Isn't the panel managed by the bridge driver in that case?

struct msm_dsi contains:
	/*
	 * panel/external_bridge connected to dsi bridge output, only one of the
	 * two can be valid at a time
	 */
	struct drm_panel *panel;
	struct drm_bridge *external_bridge;

So if you have "external_bridge" set in your case, "panel" should be NULL.
I have only moved code that uses msm_dsi->panel, so my patch really
shouldn't make any difference for you.

Am I confusing something here?

Thanks,
Stephan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
  2019-11-08 23:46   ` Stephan Gerhold
@ 2019-11-09  3:47     ` Jeffrey Hugo
  2019-11-09 12:01       ` Stephan Gerhold
  0 siblings, 1 reply; 6+ messages in thread
From: Jeffrey Hugo @ 2019-11-09  3:47 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jasper Korten, David Airlie, Sean Paul, lkml,
	open list:DRM PANEL DRIVERS, Rob Clark, Daniel Vetter, MSM,
	freedreno, Hai Li

On Fri, Nov 8, 2019 at 4:47 PM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Fri, Nov 08, 2019 at 03:12:28PM -0700, Jeffrey Hugo wrote:
> > On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> > >
> > > At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> > > from the DSI bridge pre_enable() function. At this point, the encoder
> > > (e.g. MDP5) is not enabled, so we have not started transmitting
> > > video data.
> > >
> > > However, the drm_panel_funcs documentation states that enable()
> > > should be called on the panel *after* video data is being transmitted:
> > >
> > >   The .prepare() function is typically called before the display controller
> > >   starts to transmit video data. [...] After the display controller has
> > >   started transmitting video data, it's safe to call the .enable() function.
> > >   This will typically enable the backlight to make the image on screen visible.
> > >
> > > Calling drm_panel_enable() too early causes problems for some panels:
> > > The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> > > uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> > > backlight/brightness of the screen. The enable sequence is therefore:
> > >
> > >   drm_panel_enable()
> > >     drm_panel_funcs.enable():
> > >       backlight_enable()
> > >         backlight_ops.update_status():
> > >           mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> > >
> > > The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> > > command if it is sent too early. This prevents setting the initial brightness,
> > > causing the display to be enabled with minimum brightness instead.
> > > Adding various delays in the panel initialization code does not result
> > > in any difference.
> > >
> > > On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> > > fixes the problem, indicating that the panel requires the video stream
> > > to be active before the brightness command is accepted.
> > >
> > > Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> > > delay calling it until video data is being transmitted.
> > >
> > > Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> > > (This is not strictly required for the panel affected above...)
> > >
> > > Tested-by: Jasper Korten <jja2000@gmail.com>
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > ---
> > > Since this is a core change I thought it would be better to send this
> > > early. I believe Jasper still wants to finish some other changes before
> > > submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)
> > >
> > > I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> > > with or without this patch.
> >
> > Nack, please.  I was curious so I threw this on the Lenovo Miix 630
> > laptop.  I don't get a display back with this patch.  I'll try to
> > figure out why, but currently I can't get into the machine.
>
> Thanks for testing the patch! Let's try to figure that out...
>
> I'm a bit confused, but this might be because I'm not very familiar with
> the MSM8998 laptops. It does not seem to have display in mainline yet,
> so do you have a link to all the patches you are using at the moment?

The mdp5 support is there.  Some of the dependencies have dragged out.
I'd have to make sense of my development tree as to what is relevant.
>
> Judging from the patches I was able to find, the Lenovo Miix 630 is
> using a DSI to eDP bridge.
> Isn't the panel managed by the bridge driver in that case?

It uses the TI SN65 bridge.

>
> struct msm_dsi contains:
>         /*
>          * panel/external_bridge connected to dsi bridge output, only one of the
>          * two can be valid at a time
>          */
>         struct drm_panel *panel;
>         struct drm_bridge *external_bridge;
>
> So if you have "external_bridge" set in your case, "panel" should be NULL.
> I have only moved code that uses msm_dsi->panel, so my patch really
> shouldn't make any difference for you.
>
> Am I confusing something here?

I don't think panel is null in my case.  I need to trace a few things
through to be sure.

Taking a quick look at the datasheet for the bridge, I suspect that
operations are occurring in the enable() phase of the bridge, that
need to occur before video data is transmitted.  Based on your
analysis in the commit message, I suspect these operations need to be
moved to pre_enable().

I'm hoping to gather more data this weekend, which will hopefully
identify what we need to do to move this forward without causing
regressions.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
  2019-11-09  3:47     ` Jeffrey Hugo
@ 2019-11-09 12:01       ` Stephan Gerhold
  2019-11-09 23:55         ` Jeffrey Hugo
  0 siblings, 1 reply; 6+ messages in thread
From: Stephan Gerhold @ 2019-11-09 12:01 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Jasper Korten, David Airlie, Sean Paul, lkml,
	open list:DRM PANEL DRIVERS, Rob Clark, Daniel Vetter, MSM,
	freedreno, Hai Li

On Fri, Nov 08, 2019 at 08:47:08PM -0700, Jeffrey Hugo wrote:
> On Fri, Nov 8, 2019 at 4:47 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Fri, Nov 08, 2019 at 03:12:28PM -0700, Jeffrey Hugo wrote:
> > > On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> > > >
> > > > At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> > > > from the DSI bridge pre_enable() function. At this point, the encoder
> > > > (e.g. MDP5) is not enabled, so we have not started transmitting
> > > > video data.
> > > >
> > > > However, the drm_panel_funcs documentation states that enable()
> > > > should be called on the panel *after* video data is being transmitted:
> > > >
> > > >   The .prepare() function is typically called before the display controller
> > > >   starts to transmit video data. [...] After the display controller has
> > > >   started transmitting video data, it's safe to call the .enable() function.
> > > >   This will typically enable the backlight to make the image on screen visible.
> > > >
> > > > Calling drm_panel_enable() too early causes problems for some panels:
> > > > The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> > > > uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> > > > backlight/brightness of the screen. The enable sequence is therefore:
> > > >
> > > >   drm_panel_enable()
> > > >     drm_panel_funcs.enable():
> > > >       backlight_enable()
> > > >         backlight_ops.update_status():
> > > >           mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> > > >
> > > > The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> > > > command if it is sent too early. This prevents setting the initial brightness,
> > > > causing the display to be enabled with minimum brightness instead.
> > > > Adding various delays in the panel initialization code does not result
> > > > in any difference.
> > > >
> > > > On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> > > > fixes the problem, indicating that the panel requires the video stream
> > > > to be active before the brightness command is accepted.
> > > >
> > > > Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> > > > delay calling it until video data is being transmitted.
> > > >
> > > > Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> > > > (This is not strictly required for the panel affected above...)
> > > >
> > > > Tested-by: Jasper Korten <jja2000@gmail.com>
> > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > ---
> > > > Since this is a core change I thought it would be better to send this
> > > > early. I believe Jasper still wants to finish some other changes before
> > > > submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)
> > > >
> > > > I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> > > > with or without this patch.
> > >
> > > Nack, please.  I was curious so I threw this on the Lenovo Miix 630
> > > laptop.  I don't get a display back with this patch.  I'll try to
> > > figure out why, but currently I can't get into the machine.
> >
> > Thanks for testing the patch! Let's try to figure that out...
> >
> > I'm a bit confused, but this might be because I'm not very familiar with
> > the MSM8998 laptops. It does not seem to have display in mainline yet,
> > so do you have a link to all the patches you are using at the moment?
> 
> The mdp5 support is there.  Some of the dependencies have dragged out.
> I'd have to make sense of my development tree as to what is relevant.

A dump of all the patches (whether still relevant or not) would be
helpful too. Actually I was mostly looking for the device tree part to
see which components are involved.

> >
> > Judging from the patches I was able to find, the Lenovo Miix 630 is
> > using a DSI to eDP bridge.
> > Isn't the panel managed by the bridge driver in that case?
> 
> It uses the TI SN65 bridge.
> 

It is covered by the ti-sn65dsi86 driver I assume?

> >
> > struct msm_dsi contains:
> >         /*
> >          * panel/external_bridge connected to dsi bridge output, only one of the
> >          * two can be valid at a time
> >          */
> >         struct drm_panel *panel;
> >         struct drm_bridge *external_bridge;
> >
> > So if you have "external_bridge" set in your case, "panel" should be NULL.
> > I have only moved code that uses msm_dsi->panel, so my patch really
> > shouldn't make any difference for you.
> >
> > Am I confusing something here?
> 
> I don't think panel is null in my case.  I need to trace a few things
> through to be sure.
> 

ti-sn65dsi86.c contains:

static void ti_sn_bridge_enable(struct drm_bridge *bridge)
{
	/* ... */
	drm_panel_enable(pdata->panel);
}

static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
{
	/* ... */
	drm_panel_prepare(pdata->panel);
}

So it does indeed manage the panel for you. If msm_dsi->panel is not
NULL for you it would mean that your panel is managed by two drivers
at the same time.

(Also note how it calls drm_panel_enable() in enable() instead of
pre_enable(). This is exactly the change my patch does for the case
when the panel is managed by the MSM driver...)

> Taking a quick look at the datasheet for the bridge, I suspect that
> operations are occurring in the enable() phase of the bridge, that
> need to occur before video data is transmitted.  Based on your
> analysis in the commit message, I suspect these operations need to be
> moved to pre_enable().
> 

I'm still confused how my patch makes any difference for you.
The enable sequence should be exactly the same as before.

> I'm hoping to gather more data this weekend, which will hopefully
> identify what we need to do to move this forward without causing
> regressions.

Looking forward to it, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
  2019-11-09 12:01       ` Stephan Gerhold
@ 2019-11-09 23:55         ` Jeffrey Hugo
  0 siblings, 0 replies; 6+ messages in thread
From: Jeffrey Hugo @ 2019-11-09 23:55 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jasper Korten, Hai Li, David Airlie, MSM, lkml,
	open list:DRM PANEL DRIVERS, Rob Clark, Daniel Vetter, freedreno,
	Sean Paul

On Sat, Nov 9, 2019 at 5:02 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Fri, Nov 08, 2019 at 08:47:08PM -0700, Jeffrey Hugo wrote:
> > On Fri, Nov 8, 2019 at 4:47 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> > >
> > > On Fri, Nov 08, 2019 at 03:12:28PM -0700, Jeffrey Hugo wrote:
> > > > On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> > > > >
> > > > > At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> > > > > from the DSI bridge pre_enable() function. At this point, the encoder
> > > > > (e.g. MDP5) is not enabled, so we have not started transmitting
> > > > > video data.
> > > > >
> > > > > However, the drm_panel_funcs documentation states that enable()
> > > > > should be called on the panel *after* video data is being transmitted:
> > > > >
> > > > >   The .prepare() function is typically called before the display controller
> > > > >   starts to transmit video data. [...] After the display controller has
> > > > >   started transmitting video data, it's safe to call the .enable() function.
> > > > >   This will typically enable the backlight to make the image on screen visible.
> > > > >
> > > > > Calling drm_panel_enable() too early causes problems for some panels:
> > > > > The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> > > > > uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> > > > > backlight/brightness of the screen. The enable sequence is therefore:
> > > > >
> > > > >   drm_panel_enable()
> > > > >     drm_panel_funcs.enable():
> > > > >       backlight_enable()
> > > > >         backlight_ops.update_status():
> > > > >           mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> > > > >
> > > > > The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> > > > > command if it is sent too early. This prevents setting the initial brightness,
> > > > > causing the display to be enabled with minimum brightness instead.
> > > > > Adding various delays in the panel initialization code does not result
> > > > > in any difference.
> > > > >
> > > > > On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> > > > > fixes the problem, indicating that the panel requires the video stream
> > > > > to be active before the brightness command is accepted.
> > > > >
> > > > > Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> > > > > delay calling it until video data is being transmitted.
> > > > >
> > > > > Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> > > > > (This is not strictly required for the panel affected above...)
> > > > >
> > > > > Tested-by: Jasper Korten <jja2000@gmail.com>
> > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > > ---
> > > > > Since this is a core change I thought it would be better to send this
> > > > > early. I believe Jasper still wants to finish some other changes before
> > > > > submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)
> > > > >
> > > > > I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> > > > > with or without this patch.
> > > >
> > > > Nack, please.  I was curious so I threw this on the Lenovo Miix 630
> > > > laptop.  I don't get a display back with this patch.  I'll try to
> > > > figure out why, but currently I can't get into the machine.
> > >
> > > Thanks for testing the patch! Let's try to figure that out...
> > >
> > > I'm a bit confused, but this might be because I'm not very familiar with
> > > the MSM8998 laptops. It does not seem to have display in mainline yet,
> > > so do you have a link to all the patches you are using at the moment?
> >
> > The mdp5 support is there.  Some of the dependencies have dragged out.
> > I'd have to make sense of my development tree as to what is relevant.
>
> A dump of all the patches (whether still relevant or not) would be
> helpful too. Actually I was mostly looking for the device tree part to
> see which components are involved.

DSI0 to "ti,sn65dsi86" (bridge) to "sharp,ld-d5116z01b" (panel).

>
> > >
> > > Judging from the patches I was able to find, the Lenovo Miix 630 is
> > > using a DSI to eDP bridge.
> > > Isn't the panel managed by the bridge driver in that case?
> >
> > It uses the TI SN65 bridge.
> >
>
> It is covered by the ti-sn65dsi86 driver I assume?

Yes

>
> > >
> > > struct msm_dsi contains:
> > >         /*
> > >          * panel/external_bridge connected to dsi bridge output, only one of the
> > >          * two can be valid at a time
> > >          */
> > >         struct drm_panel *panel;
> > >         struct drm_bridge *external_bridge;
> > >
> > > So if you have "external_bridge" set in your case, "panel" should be NULL.
> > > I have only moved code that uses msm_dsi->panel, so my patch really
> > > shouldn't make any difference for you.
> > >
> > > Am I confusing something here?
> >
> > I don't think panel is null in my case.  I need to trace a few things
> > through to be sure.
> >
>
> ti-sn65dsi86.c contains:
>
> static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> {
>         /* ... */
>         drm_panel_enable(pdata->panel);
> }
>
> static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> {
>         /* ... */
>         drm_panel_prepare(pdata->panel);
> }
>
> So it does indeed manage the panel for you. If msm_dsi->panel is not
> NULL for you it would mean that your panel is managed by two drivers
> at the same time.
>
> (Also note how it calls drm_panel_enable() in enable() instead of
> pre_enable(). This is exactly the change my patch does for the case
> when the panel is managed by the MSM driver...)
>
> > Taking a quick look at the datasheet for the bridge, I suspect that
> > operations are occurring in the enable() phase of the bridge, that
> > need to occur before video data is transmitted.  Based on your
> > analysis in the commit message, I suspect these operations need to be
> > moved to pre_enable().
> >
>
> I'm still confused how my patch makes any difference for you.
> The enable sequence should be exactly the same as before.
>
> > I'm hoping to gather more data this weekend, which will hopefully
> > identify what we need to do to move this forward without causing
> > regressions.
>
> Looking forward to it, thanks!

Panel was infact NULL for me, and the DSI panel operations are not
translating into direct bridge calls.  I re-applied your change, and
I'm not reproing the original failure.  I tried a couple of times to
be sure.  I have no idea what happened during the initial testing to
break things.

Therefore, I remove my nack.

Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 21:28 [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable() Stephan Gerhold
2019-11-08 22:12 ` [Freedreno] " Jeffrey Hugo
2019-11-08 23:46   ` Stephan Gerhold
2019-11-09  3:47     ` Jeffrey Hugo
2019-11-09 12:01       ` Stephan Gerhold
2019-11-09 23:55         ` Jeffrey Hugo

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git