Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
On Sat, Nov 9, 2019 at 5:02 AM Stephan Gerhold 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 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 > > > > 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 > > > > > Signed-off-by: Stephan Gerhold > > > > > --- > > > > > 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 somethin
Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()
On Fri, Nov 08, 2019 at 08:47:08PM -0700, Jeffrey Hugo wrote:
> On Fri, Nov 8, 2019 at 4:47 PM Stephan Gerhold 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
> > > 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
> > > > Signed-off-by: Stephan Gerhold
> > > > ---
> > > > 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
