| Message ID | 20230725-visionox-vtdr-prev-first-v1-1-3bc44cec7dc6@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: Add prepare_prev_first flag to Visionox VTDR6130 | expand |
Hi, On 26/07/2023 00:56, Jessica Zhang wrote: > Due to a recent introduction of the pre_enable_prev_first bridge flag [1], > the panel driver will be probed before the DSI is enabled, causing the > DCS commands to fail to send. > > Ensure that DSI is enabled before panel probe by setting the > prepare_prev_first flag for the panel. Well this is specific to MSM DSI driver, it's not related at all to the panel. Neil > > [1] commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") > > Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel driver") > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > index bb0dfd86ea67..e1363e128e7e 100644 > --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > dsi->format = MIPI_DSI_FMT_RGB888; > dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | > MIPI_DSI_CLOCK_NON_CONTINUOUS; > + ctx->panel.prepare_prev_first = true; > > drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, > DRM_MODE_CONNECTOR_DSI); > > --- > base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd > change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f > > Best regards,
On 7/31/2023 6:00 AM, Neil Armstrong wrote: > Hi, > > On 26/07/2023 00:56, Jessica Zhang wrote: >> Due to a recent introduction of the pre_enable_prev_first bridge flag >> [1], >> the panel driver will be probed before the DSI is enabled, causing the >> DCS commands to fail to send. >> >> Ensure that DSI is enabled before panel probe by setting the >> prepare_prev_first flag for the panel. > > Well this is specific to MSM DSI driver, it's not related at all to the > panel. Hi Neil, I think there might be some confusion caused by the commit message -- instead of "enabled before panel probe", it should be "enabled before panel pre_enable()" as the panel on commands are sent during prepare(), which is matched to bridge pre_enable(). IIRC the general rule is that the panel driver should set the prepare_prev_first flag if the on commands are sent during pre_enable(), so I'll keep the code change but correct the commit message if that's ok with you. Thanks, Jessica Zhang > > Neil > >> >> [1] commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first >> to alter bridge init order") >> >> Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel driver") >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> index bb0dfd86ea67..e1363e128e7e 100644 >> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct >> mipi_dsi_device *dsi) >> dsi->format = MIPI_DSI_FMT_RGB888; >> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >> MIPI_DSI_MODE_NO_EOT_PACKET | >> MIPI_DSI_CLOCK_NON_CONTINUOUS; >> + ctx->panel.prepare_prev_first = true; >> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >> DRM_MODE_CONNECTOR_DSI); >> >> --- >> base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd >> change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f >> >> Best regards, >
Hi Neil On 8/3/2023 10:19 AM, Jessica Zhang wrote: > > > On 7/31/2023 6:00 AM, Neil Armstrong wrote: >> Hi, >> >> On 26/07/2023 00:56, Jessica Zhang wrote: >>> Due to a recent introduction of the pre_enable_prev_first bridge flag >>> [1], >>> the panel driver will be probed before the DSI is enabled, causing the >>> DCS commands to fail to send. >>> >>> Ensure that DSI is enabled before panel probe by setting the >>> prepare_prev_first flag for the panel. >> >> Well this is specific to MSM DSI driver, it's not related at all to >> the panel. > I dont fully agree this is a MSM DSI driver specific thing. If the panel can send its commands in its enable() callback, then this flag need not be set. When a panel sends its DCS commands in its pre_enable() callback, any DSI controller will need to be ON before that otherwise DCS commands cannot be sent. With this in mind, may I know why is this a MSM change and not a panel change? As per my discussion with Dmitry during the last sync up, we were aligned on this expectation. Thanks Abhinav > Hi Neil, > > I think there might be some confusion caused by the commit message -- > instead of "enabled before panel probe", it should be "enabled before > panel pre_enable()" as the panel on commands are sent during prepare(), > which is matched to bridge pre_enable(). > > IIRC the general rule is that the panel driver should set the > prepare_prev_first flag if the on commands are sent during pre_enable(), > so I'll keep the code change but correct the commit message if that's ok > with you. > > Thanks, > > Jessica Zhang > >> >> Neil >> >>> >>> [1] commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first >>> to alter bridge init order") >>> >>> Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel >>> driver") >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> index bb0dfd86ea67..e1363e128e7e 100644 >>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct >>> mipi_dsi_device *dsi) >>> dsi->format = MIPI_DSI_FMT_RGB888; >>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>> MIPI_DSI_MODE_NO_EOT_PACKET | >>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>> + ctx->panel.prepare_prev_first = true; >>> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >>> DRM_MODE_CONNECTOR_DSI); >>> >>> --- >>> base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd >>> change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f >>> >>> Best regards, >>
Hi Abhinav, On 10/08/2023 18:26, Abhinav Kumar wrote: > Hi Neil > > On 8/3/2023 10:19 AM, Jessica Zhang wrote: >> >> >> On 7/31/2023 6:00 AM, Neil Armstrong wrote: >>> Hi, >>> >>> On 26/07/2023 00:56, Jessica Zhang wrote: >>>> Due to a recent introduction of the pre_enable_prev_first bridge flag [1], >>>> the panel driver will be probed before the DSI is enabled, causing the >>>> DCS commands to fail to send. >>>> >>>> Ensure that DSI is enabled before panel probe by setting the >>>> prepare_prev_first flag for the panel. >>> >>> Well this is specific to MSM DSI driver, it's not related at all to the panel. >> > > I dont fully agree this is a MSM DSI driver specific thing. > > If the panel can send its commands in its enable() callback, then this flag need not be set. > > When a panel sends its DCS commands in its pre_enable() callback, any DSI controller will need to be ON before that otherwise DCS commands cannot be sent. > > With this in mind, may I know why is this a MSM change and not a panel change? > > As per my discussion with Dmitry during the last sync up, we were aligned on this expectation. As of today, only the MSM DSI driver expects panels to have prepare_prev_first because it's the first one calling pre_enable() before the DSI controller to be on, all other DSI drivers I know still enables the DSI controller in mode_set() and thus can send commands in pre_enable() which is a loose way to map the pre-video state for DSI panels... A panel driver should not depend on features of a DSI controller, which is the case here with this patch. Today's expectation is to send DSI commands in pre_enable() then when enabled expect to be in video mode when enable() is called. The main reason is because some DSI controllers cannot send LP commands after switching to video mode (allwinner for example), so we must take this into account. For v6.6, I don't see other solutions than reverting 9e15123eca79 (reverting won't regress anything, because now it regresses also other panels on MSM platforms) and try to find a proper solution for v6.7... Neil > > Thanks > > Abhinav > >> Hi Neil, >> >> I think there might be some confusion caused by the commit message -- instead of "enabled before panel probe", it should be "enabled before panel pre_enable()" as the panel on commands are sent during prepare(), which is matched to bridge pre_enable(). >> >> IIRC the general rule is that the panel driver should set the prepare_prev_first flag if the on commands are sent during pre_enable(), so I'll keep the code change but correct the commit message if that's ok with you. >> >> Thanks, >> > >> Jessica Zhang >> >>> >>> Neil >>> >>>> >>>> [1] commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") It's not the right commit that cause regression here, it's : 9e15123eca79 drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset >>>> >>>> Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel driver") >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> index bb0dfd86ea67..e1363e128e7e 100644 >>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | >>>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>> + ctx->panel.prepare_prev_first = true; >>>> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >>>> DRM_MODE_CONNECTOR_DSI); >>>> >>>> --- >>>> base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd >>>> change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f >>>> >>>> Best regards, >>>
Hi Neil On 8/14/2023 1:01 AM, neil.armstrong@linaro.org wrote: > Hi Abhinav, > > On 10/08/2023 18:26, Abhinav Kumar wrote: >> Hi Neil >> >> On 8/3/2023 10:19 AM, Jessica Zhang wrote: >>> >>> >>> On 7/31/2023 6:00 AM, Neil Armstrong wrote: >>>> Hi, >>>> >>>> On 26/07/2023 00:56, Jessica Zhang wrote: >>>>> Due to a recent introduction of the pre_enable_prev_first bridge >>>>> flag [1], >>>>> the panel driver will be probed before the DSI is enabled, causing the >>>>> DCS commands to fail to send. >>>>> >>>>> Ensure that DSI is enabled before panel probe by setting the >>>>> prepare_prev_first flag for the panel. >>>> >>>> Well this is specific to MSM DSI driver, it's not related at all to >>>> the panel. >>> >> >> I dont fully agree this is a MSM DSI driver specific thing. >> >> If the panel can send its commands in its enable() callback, then this >> flag need not be set. >> >> When a panel sends its DCS commands in its pre_enable() callback, any >> DSI controller will need to be ON before that otherwise DCS commands >> cannot be sent. >> >> With this in mind, may I know why is this a MSM change and not a panel >> change? >> >> As per my discussion with Dmitry during the last sync up, we were >> aligned on this expectation. > > As of today, only the MSM DSI driver expects panels to have > prepare_prev_first because it's the first > one calling pre_enable() before the DSI controller to be on, all other > DSI drivers I know > still enables the DSI controller in mode_set() and thus can send > commands in pre_enable() which > is a loose way to map the pre-video state for DSI panels... > It looks like there are multiple panels already setting this flag so this panel will not be the first unless they were added to make those work with MSM (which seems unlikely) panel-samsung-s6d7aa0.c: ctx->panel.prepare_prev_first = true; panel-samsung-s6e3ha2.c: ctx->panel.prepare_prev_first = true; panel-samsung-s6e63j0x03.c: ctx->panel.prepare_prev_first = true; panel-samsung-s6e8aa0.c: ctx->panel.prepare_prev_first = true; This is where I would like to understand a bit that if the panel sends out the ON commands in enable() instead of pre_enable() then, this flag will not be needed. So its also depends on the panel side and thats why the bridge feeds of the panel's input in devm_drm_panel_bridge_add_typed() bridge->pre_enable_prev_first = panel->prepare_prev_first; > A panel driver should not depend on features of a DSI controller, which > is the case here > with this patch. Today's expectation is to send DSI commands in > pre_enable() then when enabled > expect to be in video mode when enable() is called. > We are not depending on any feature as such. Any DSI controller , not just MSM's would need to be ON for DCS commands to be sent out in the panel's pre_enable() callback. Its not true that MSM is the only driver powering on the DSI controller in pre_enable(). Even MTK seems to be doing that mtk_dsi_bridge_atomic_pre_enable So I assume any panel which sends out commands in pre_enable() will not work with MTK as well. > The main reason is because some DSI controllers cannot send LP commands > after switching > to video mode (allwinner for example), so we must take this into account. > > For v6.6, I don't see other solutions than reverting 9e15123eca79 > (reverting won't regress anything, > because now it regresses also other panels on MSM platforms) and try to > find a proper solution for v6.7... > No, I would prefer not to revert that. It will bring back special handling for the parade chip into MSM driver, something which I would prefer not to go back to. Powering on the DSI in modeset() was done only for the parade chip. > Neil > >> >> Thanks >> >> Abhinav >> >>> Hi Neil, >>> >>> I think there might be some confusion caused by the commit message -- >>> instead of "enabled before panel probe", it should be "enabled before >>> panel pre_enable()" as the panel on commands are sent during >>> prepare(), which is matched to bridge pre_enable(). >>> >>> IIRC the general rule is that the panel driver should set the >>> prepare_prev_first flag if the on commands are sent during >>> pre_enable(), so I'll keep the code change but correct the commit >>> message if that's ok with you. >>> >>> Thanks, >>> >> >>> Jessica Zhang >>> >>>> >>>> Neil >>>> >>>>> >>>>> [1] commit 4fb912e5e190 ("drm/bridge: Introduce >>>>> pre_enable_prev_first to alter bridge init order") > > It's not the right commit that cause regression here, it's : > > 9e15123eca79 drm/msm/dsi: Stop unconditionally powering up DSI hosts at > modeset > >>>>> >>>>> Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel >>>>> driver") >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>>> --- >>>>> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> index bb0dfd86ea67..e1363e128e7e 100644 >>>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct >>>>> mipi_dsi_device *dsi) >>>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>>>> MIPI_DSI_MODE_NO_EOT_PACKET | >>>>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>>> + ctx->panel.prepare_prev_first = true; >>>>> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >>>>> DRM_MODE_CONNECTOR_DSI); >>>>> >>>>> --- >>>>> base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd >>>>> change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f >>>>> >>>>> Best regards, >>>> >
Hi Abhinav, On 14/08/2023 20:02, Abhinav Kumar wrote: > Hi Neil > > On 8/14/2023 1:01 AM, neil.armstrong@linaro.org wrote: >> Hi Abhinav, >> >> On 10/08/2023 18:26, Abhinav Kumar wrote: >>> Hi Neil >>> >>> On 8/3/2023 10:19 AM, Jessica Zhang wrote: >>>> >>>> >>>> On 7/31/2023 6:00 AM, Neil Armstrong wrote: >>>>> Hi, >>>>> >>>>> On 26/07/2023 00:56, Jessica Zhang wrote: >>>>>> Due to a recent introduction of the pre_enable_prev_first bridge flag [1], >>>>>> the panel driver will be probed before the DSI is enabled, causing the >>>>>> DCS commands to fail to send. >>>>>> >>>>>> Ensure that DSI is enabled before panel probe by setting the >>>>>> prepare_prev_first flag for the panel. >>>>> >>>>> Well this is specific to MSM DSI driver, it's not related at all to the panel. >>>> >>> >>> I dont fully agree this is a MSM DSI driver specific thing. >>> >>> If the panel can send its commands in its enable() callback, then this flag need not be set. >>> >>> When a panel sends its DCS commands in its pre_enable() callback, any DSI controller will need to be ON before that otherwise DCS commands cannot be sent. >>> >>> With this in mind, may I know why is this a MSM change and not a panel change? >>> >>> As per my discussion with Dmitry during the last sync up, we were aligned on this expectation. >> >> As of today, only the MSM DSI driver expects panels to have prepare_prev_first because it's the first >> one calling pre_enable() before the DSI controller to be on, all other DSI drivers I know >> still enables the DSI controller in mode_set() and thus can send commands in pre_enable() which >> is a loose way to map the pre-video state for DSI panels... >> > > It looks like there are multiple panels already setting this flag so this panel will not be the first unless they were added to make those work with MSM (which seems unlikely) > > panel-samsung-s6d7aa0.c: ctx->panel.prepare_prev_first = true; > panel-samsung-s6e3ha2.c: ctx->panel.prepare_prev_first = true; > panel-samsung-s6e63j0x03.c: ctx->panel.prepare_prev_first = true; > panel-samsung-s6e8aa0.c: ctx->panel.prepare_prev_first = true; > > This is where I would like to understand a bit that if the panel sends out the ON commands in enable() instead of pre_enable() then, this flag will not be needed. So its also depends on the panel side and thats why > the bridge feeds of the panel's input in devm_drm_panel_bridge_add_typed() > > bridge->pre_enable_prev_first = panel->prepare_prev_first; > >> A panel driver should not depend on features of a DSI controller, which is the case here >> with this patch. Today's expectation is to send DSI commands in pre_enable() then when enabled >> expect to be in video mode when enable() is called. >> > > We are not depending on any feature as such. Any DSI controller , not just MSM's would need to be ON for DCS commands to be sent out in the panel's pre_enable() callback. > > Its not true that MSM is the only driver powering on the DSI controller in pre_enable(). Even MTK seems to be doing that > > mtk_dsi_bridge_atomic_pre_enable > > So I assume any panel which sends out commands in pre_enable() will not work with MTK as well. Sending HS commands will always work on any controller, it's all about LP commands. The Samsung panels you listed only send HS commands so they can use prepare_prev_first and work on any controllers. None of the panels using LP commands uses prepare_prev_first: $ grep prepare_prev_first `grep -l LPM drivers/gpu/drm/panel/panel-*` $ Note that there's a smart move for VTDR6130 with the command mode introduced in 20230728011218.14630-1-parellan@quicinc.com, in the way prepare_prev_first could only be set to true if command-mode is selected. I'll accept that since it would be logical, video mode won't work anymore but by default the panel would still work in command mode + prepare_prev_first. > >> The main reason is because some DSI controllers cannot send LP commands after switching >> to video mode (allwinner for example), so we must take this into account. >> >> For v6.6, I don't see other solutions than reverting 9e15123eca79 (reverting won't regress anything, >> because now it regresses also other panels on MSM platforms) and try to find a proper solution for v6.7... >> > > No, I would prefer not to revert that. It will bring back special handling for the parade chip into MSM driver, something which I would prefer not to go back to. Powering on the DSI in modeset() was done only for the parade chip. I understand, but this patch doesn't qualify as a fix for 9e15123eca79 and is too late to be merged in drm-misc-next for v6.6, and since 9e15123eca79 actually breaks some support it should be reverted (+ deps) since we are late in the rc cycles. It's not a fatality or the end of the world, but this is an indirect fix and not way all this should be fixed. We already had the case for the lt9611 breakage, and it's the same case here. Neil > >> Neil >> >>> >>> Thanks >>> >>> Abhinav >>> >>>> Hi Neil, >>>> >>>> I think there might be some confusion caused by the commit message -- instead of "enabled before panel probe", it should be "enabled before panel pre_enable()" as the panel on commands are sent during prepare(), which is matched to bridge pre_enable(). >>>> >>>> IIRC the general rule is that the panel driver should set the prepare_prev_first flag if the on commands are sent during pre_enable(), so I'll keep the code change but correct the commit message if that's ok with you. >>>> >>>> Thanks, >>>> >>> >>>> Jessica Zhang >>>> >>>>> >>>>> Neil >>>>> >>>>>> >>>>>> [1] commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") >> >> It's not the right commit that cause regression here, it's : >> >> 9e15123eca79 drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset >> >>>>>> >>>>>> Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel driver") >>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>> index bb0dfd86ea67..e1363e128e7e 100644 >>>>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>> @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >>>>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>>>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | >>>>>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>>>> + ctx->panel.prepare_prev_first = true; >>>>>> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >>>>>> DRM_MODE_CONNECTOR_DSI); >>>>>> >>>>>> --- >>>>>> base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd >>>>>> change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f >>>>>> >>>>>> Best regards, >>>>> >>
On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > Hi Abhinav, > > On 14/08/2023 20:02, Abhinav Kumar wrote: >> Hi Neil >> >> On 8/14/2023 1:01 AM, neil.armstrong@linaro.org wrote: >>> Hi Abhinav, >>> >>> On 10/08/2023 18:26, Abhinav Kumar wrote: >>>> Hi Neil >>>> >>>> On 8/3/2023 10:19 AM, Jessica Zhang wrote: >>>>> >>>>> >>>>> On 7/31/2023 6:00 AM, Neil Armstrong wrote: >>>>>> Hi, >>>>>> >>>>>> On 26/07/2023 00:56, Jessica Zhang wrote: >>>>>>> Due to a recent introduction of the pre_enable_prev_first bridge >>>>>>> flag [1], >>>>>>> the panel driver will be probed before the DSI is enabled, >>>>>>> causing the >>>>>>> DCS commands to fail to send. >>>>>>> >>>>>>> Ensure that DSI is enabled before panel probe by setting the >>>>>>> prepare_prev_first flag for the panel. >>>>>> >>>>>> Well this is specific to MSM DSI driver, it's not related at all >>>>>> to the panel. >>>>> >>>> >>>> I dont fully agree this is a MSM DSI driver specific thing. >>>> >>>> If the panel can send its commands in its enable() callback, then >>>> this flag need not be set. >>>> >>>> When a panel sends its DCS commands in its pre_enable() callback, >>>> any DSI controller will need to be ON before that otherwise DCS >>>> commands cannot be sent. >>>> >>>> With this in mind, may I know why is this a MSM change and not a >>>> panel change? >>>> >>>> As per my discussion with Dmitry during the last sync up, we were >>>> aligned on this expectation. >>> >>> As of today, only the MSM DSI driver expects panels to have >>> prepare_prev_first because it's the first >>> one calling pre_enable() before the DSI controller to be on, all >>> other DSI drivers I know >>> still enables the DSI controller in mode_set() and thus can send >>> commands in pre_enable() which >>> is a loose way to map the pre-video state for DSI panels... >>> >> >> It looks like there are multiple panels already setting this flag so >> this panel will not be the first unless they were added to make those >> work with MSM (which seems unlikely) >> >> panel-samsung-s6d7aa0.c: ctx->panel.prepare_prev_first = true; >> panel-samsung-s6e3ha2.c: ctx->panel.prepare_prev_first = true; >> panel-samsung-s6e63j0x03.c: ctx->panel.prepare_prev_first = true; >> panel-samsung-s6e8aa0.c: ctx->panel.prepare_prev_first = true; >> >> This is where I would like to understand a bit that if the panel sends >> out the ON commands in enable() instead of pre_enable() then, this >> flag will not be needed. So its also depends on the panel side and >> thats why >> the bridge feeds of the panel's input in >> devm_drm_panel_bridge_add_typed() >> >> bridge->pre_enable_prev_first = panel->prepare_prev_first; >> >>> A panel driver should not depend on features of a DSI controller, >>> which is the case here >>> with this patch. Today's expectation is to send DSI commands in >>> pre_enable() then when enabled >>> expect to be in video mode when enable() is called. >>> >> >> We are not depending on any feature as such. Any DSI controller , not >> just MSM's would need to be ON for DCS commands to be sent out in the >> panel's pre_enable() callback. >> >> Its not true that MSM is the only driver powering on the DSI >> controller in pre_enable(). Even MTK seems to be doing that >> >> mtk_dsi_bridge_atomic_pre_enable >> >> So I assume any panel which sends out commands in pre_enable() will >> not work with MTK as well. > > Sending HS commands will always work on any controller, it's all about > LP commands. > The Samsung panels you listed only send HS commands so they can use > prepare_prev_first > and work on any controllers. I think there is some misunderstanding there, supported by the description of the flag. If I remember correctly, some hosts (sunxi) can not send DCS commands after enabling video stream and switching to HS mode, see [1]. Thus, as you know, most of the drivers have all DSI panel setup commands in drm_panel_funcs::prepare() / drm_bridge_funcs::pre_enable() callbacks, not paying attention whether these commands are to be sent in LP or in HS mode. Previously DSI source drivers could power on the DSI link either in mode_set() or in pre_enable() callbacks, with mode_set() being the hack to make panel/bridge drivers to be able to send commands from their prepare() / pre_enable() callbacks. With the prev_first flags being introduced, we have established that DSI link should be enabled in DSI host's pre_enable() callback and switched to HS mode (be it command or video) in the enable() callback. So far so good. Unfortunately this change is not fully backwards-compatible. This requires that all DSI panels sending commands from prepare() should have the prepare_prev_first flag. In some sense, all such patches might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first flag to drm_panel"). > None of the panels using LP commands uses prepare_prev_first: > > $ grep prepare_prev_first `grep -l LPM drivers/gpu/drm/panel/panel-*` > $ > > Note that there's a smart move for VTDR6130 with the command mode > introduced in 20230728011218.14630-1-parellan@quicinc.com, > in the way prepare_prev_first could only be set to true if command-mode > is selected. > I'll accept that since it would be logical, video mode won't work > anymore but by default > the panel would still work in command mode + prepare_prev_first. > >> >>> The main reason is because some DSI controllers cannot send LP >>> commands after switching >>> to video mode (allwinner for example), so we must take this into >>> account. >>> >>> For v6.6, I don't see other solutions than reverting 9e15123eca79 >>> (reverting won't regress anything, >>> because now it regresses also other panels on MSM platforms) and try >>> to find a proper solution for v6.7... >>> >> >> No, I would prefer not to revert that. It will bring back special >> handling for the parade chip into MSM driver, something which I would >> prefer not to go back to. Powering on the DSI in modeset() was done >> only for the parade chip. > > I understand, but this patch doesn't qualify as a fix for 9e15123eca79 > and is too late to be merged in drm-misc-next for v6.6, > and since 9e15123eca79 actually breaks some support it should be > reverted (+ deps) since we are late in the rc cycles. If we go this way, we can never reapply these patches. There will be no guarantee that all panel drivers are completely converted. We already have a story without an observable end - DRM_BRIDGE_ATTACH_NO_CONNECTOR. I'd consider that the DSI driver is correct here and it is about the panel drivers that require fixes patches. If you care about the particular Fixes tag, I have provided one several lines above. > It's not a fatality or the end of the world, but this is an indirect fix > and not way all this should be fixed. But why? It is the way all the panels are expected to be fixed: DSI commands in prepare() => prepare_prev_first flag. And for the bridges, DSI commands in pre_enable() => pre_enable_prev_first flag. > We already had the case for the lt9611 breakage, and it's the same case > here. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#n776 > > Neil > >> >>> Neil >>> >>>> >>>> Thanks >>>> >>>> Abhinav >>>> >>>>> Hi Neil, >>>>> >>>>> I think there might be some confusion caused by the commit message >>>>> -- instead of "enabled before panel probe", it should be "enabled >>>>> before panel pre_enable()" as the panel on commands are sent during >>>>> prepare(), which is matched to bridge pre_enable(). >>>>> >>>>> IIRC the general rule is that the panel driver should set the >>>>> prepare_prev_first flag if the on commands are sent during >>>>> pre_enable(), so I'll keep the code change but correct the commit >>>>> message if that's ok with you. >>>>> >>>>> Thanks, >>>>> >>>> >>>>> Jessica Zhang >>>>> >>>>>> >>>>>> Neil >>>>>> >>>>>>> >>>>>>> [1] commit 4fb912e5e190 ("drm/bridge: Introduce >>>>>>> pre_enable_prev_first to alter bridge init order") >>> >>> It's not the right commit that cause regression here, it's : >>> >>> 9e15123eca79 drm/msm/dsi: Stop unconditionally powering up DSI hosts >>> at modeset Regression was caused by 9e15123eca79. But the real source of the regression is 5ea6b1702781, which didn't update all panel drivers to follow the expected sequence. >>> >>>>>>> >>>>>>> Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel >>>>>>> driver") >>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>>> index bb0dfd86ea67..e1363e128e7e 100644 >>>>>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>>> @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct >>>>>>> mipi_dsi_device *dsi) >>>>>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>>>>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>>>>>> MIPI_DSI_MODE_NO_EOT_PACKET | >>>>>>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>>>>> + ctx->panel.prepare_prev_first = true; >>>>>>> drm_panel_init(&ctx->panel, dev, >>>>>>> &visionox_vtdr6130_panel_funcs, >>>>>>> DRM_MODE_CONNECTOR_DSI); >>>>>>> >>>>>>> --- >>>>>>> base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd >>>>>>> change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f >>>>>>> >>>>>>> Best regards, >>>>>> >>> >
Hi Dmitry, On 17/08/2023 20:35, Dmitry Baryshkov wrote: > On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >> Hi Abhinav, >> >> On 14/08/2023 20:02, Abhinav Kumar wrote: <snip> >> >> Sending HS commands will always work on any controller, it's all about LP commands. >> The Samsung panels you listed only send HS commands so they can use prepare_prev_first >> and work on any controllers. > > I think there is some misunderstanding there, supported by the description of the flag. > > If I remember correctly, some hosts (sunxi) can not send DCS commands after enabling video stream and switching to HS mode, see [1]. Thus, as you know, most of the drivers have all DSI panel setup commands in drm_panel_funcs::prepare() / drm_bridge_funcs::pre_enable() callbacks, not paying attention whether these commands are to be sent in LP or in HS mode. > > Previously DSI source drivers could power on the DSI link either in mode_set() or in pre_enable() callbacks, with mode_set() being the hack to make panel/bridge drivers to be able to send commands from their prepare() / pre_enable() callbacks. > > With the prev_first flags being introduced, we have established that DSI link should be enabled in DSI host's pre_enable() callback and switched to HS mode (be it command or video) in the enable() callback. > > So far so good. It seems coherent, I would like first to have a state of all DSI host drivers and make this would actually work first before adding the prev_first flag to all the required panels. > > Unfortunately this change is not fully backwards-compatible. This requires that all DSI panels sending commands from prepare() should have the prepare_prev_first flag. In some sense, all such patches might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first flag to drm_panel"). This kind of migration should be done *before* any possible regression, not the other way round. If all panels sending commands from prepare() should have the prepare_prev_first flag, then it should be first, check for regressions then continue. <snip> >> >> I understand, but this patch doesn't qualify as a fix for 9e15123eca79 and is too late to be merged in drm-misc-next for v6.6, >> and since 9e15123eca79 actually breaks some support it should be reverted (+ deps) since we are late in the rc cycles. > > If we go this way, we can never reapply these patches. There will be no guarantee that all panel drivers are completely converted. We already have a story without an observable end - DRM_BRIDGE_ATTACH_NO_CONNECTOR. I don't understand this point, who would block re-applying the patches ? The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple Linux version and went smoothly because we reverted regressing patches and restarted when needed, I don't understand why we can't do this here aswell. > > I'd consider that the DSI driver is correct here and it is about the panel drivers that require fixes patches. If you care about the particular Fixes tag, I have provided one several lines above. Unfortunately it should be done in the other way round, prepare for migration, then migrate, I mean if it's a required migration, then it should be done and I'll support it from both bridge and panel PoV. So, first this patch has the wrong Fixes tag, and I would like a better explanation on the commit message in any case. Then I would like to have an ack from some drm-misc maintainers before applying it because it fixes a patch that was sent via the msm tree thus per the drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. Neil <snip>
On 18/08/2023 11:25, neil.armstrong@linaro.org wrote: > Hi Dmitry, > > On 17/08/2023 20:35, Dmitry Baryshkov wrote: >> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>> Hi Abhinav, >>> >>> On 14/08/2023 20:02, Abhinav Kumar wrote: > > <snip> > >>> >>> Sending HS commands will always work on any controller, it's all >>> about LP commands. >>> The Samsung panels you listed only send HS commands so they can use >>> prepare_prev_first >>> and work on any controllers. >> >> I think there is some misunderstanding there, supported by the >> description of the flag. >> >> If I remember correctly, some hosts (sunxi) can not send DCS commands >> after enabling video stream and switching to HS mode, see [1]. Thus, >> as you know, most of the drivers have all DSI panel setup commands in >> drm_panel_funcs::prepare() / drm_bridge_funcs::pre_enable() callbacks, >> not paying attention whether these commands are to be sent in LP or in >> HS mode. >> >> Previously DSI source drivers could power on the DSI link either in >> mode_set() or in pre_enable() callbacks, with mode_set() being the >> hack to make panel/bridge drivers to be able to send commands from >> their prepare() / pre_enable() callbacks. >> >> With the prev_first flags being introduced, we have established that >> DSI link should be enabled in DSI host's pre_enable() callback and >> switched to HS mode (be it command or video) in the enable() callback. >> >> So far so good. > > It seems coherent, I would like first to have a state of all DSI host > drivers and make this would actually work first before adding the > prev_first flag to all the required panels. > >> >> Unfortunately this change is not fully backwards-compatible. This >> requires that all DSI panels sending commands from prepare() should >> have the prepare_prev_first flag. In some sense, all such patches >> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >> flag to drm_panel"). > > This kind of migration should be done *before* any possible regression, > not the other way round. > > If all panels sending commands from prepare() should have the > prepare_prev_first flag, then it should be first, check for regressions > then continue. > > <snip> > >>> >>> I understand, but this patch doesn't qualify as a fix for >>> 9e15123eca79 and is too late to be merged in drm-misc-next for v6.6, >>> and since 9e15123eca79 actually breaks some support it should be >>> reverted (+ deps) since we are late in the rc cycles. >> >> If we go this way, we can never reapply these patches. There will be >> no guarantee that all panel drivers are completely converted. We >> already have a story without an observable end - >> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > I don't understand this point, who would block re-applying the patches ? Consider us reverting 9e15123eca79 now and then reapplying it next cycle. Then another panel / bridge that was not converted to use pre_enable_prev_first pops up. And suddently we have to revert them again. > The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > Linux version and went smoothly because we reverted > regressing patches and restarted when needed, I don't understand why we > can't do this here aswell. With DRM_BRIDGE_ATTACH_NO_CONNECTOR both host and peripheral drivers were involved. This way they share knowledge about the migration state. With prev_first we do not have such shared knowledge. Host assumes that it can work according to the documentation: turn DSI link to LP-11 in pre_enable(), switch to HS in enable(). It can not check whether the next bridge did not set pre_enable_prev_first because of it not being required (like for the Parade bridge) or because next bridge is not converted yet (and thus DSI host should power up the link in atomic_mode_set). Granted that there is no way for the DSI host driver to attune itself to the DSI peripheral driver requirements, I can only consider corresponding (requiring prev_first) panel drivers broken since 5ea6b1702781 ("drm/panel: Add prepare_prev_first flag to drm_panel") and all bridge drivers with this issue broken since 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order"). > >> >> I'd consider that the DSI driver is correct here and it is about the >> panel drivers that require fixes patches. If you care about the >> particular Fixes tag, I have provided one several lines above. > > Unfortunately it should be done in the other way round, prepare for > migration, then migrate, > > I mean if it's a required migration, then it should be done and I'll > support it from both bridge and panel PoV. > > So, first this patch has the wrong Fixes tag, and I would like a better > explanation on the commit message in any case. > Then I would like to have an ack from some drm-misc maintainers before > applying it because it fixes a patch that > was sent via the msm tree thus per the drm-misc rules I cannot apply it > via the drm-misc-next-fixes tree. > > Neil > > <snip> >
Hi, On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > On 17/08/2023 20:35, Dmitry Baryshkov wrote: > > On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > > > Sending HS commands will always work on any controller, it's all > > > about LP commands. The Samsung panels you listed only send HS > > > commands so they can use prepare_prev_first and work on any > > > controllers. > > > > I think there is some misunderstanding there, supported by the > > description of the flag. > > > > If I remember correctly, some hosts (sunxi) can not send DCS > > commands after enabling video stream and switching to HS mode, see > > [1]. Thus, as you know, most of the drivers have all DSI panel setup > > commands in drm_panel_funcs::prepare() / > > drm_bridge_funcs::pre_enable() callbacks, not paying attention > > whether these commands are to be sent in LP or in HS mode. > > > > Previously DSI source drivers could power on the DSI link either in > > mode_set() or in pre_enable() callbacks, with mode_set() being the > > hack to make panel/bridge drivers to be able to send commands from > > their prepare() / pre_enable() callbacks. > > > > With the prev_first flags being introduced, we have established that > > DSI link should be enabled in DSI host's pre_enable() callback and > > switched to HS mode (be it command or video) in the enable() > > callback. > > > > So far so good. > > It seems coherent, I would like first to have a state of all DSI host > drivers and make this would actually work first before adding the > prev_first flag to all the required panels. This is definitely what we should do in an ideal world, but at least for sunxi there's no easy way for it at the moment. There's no documentation for it and the driver provided doesn't allow this to happen. Note that I'm not trying to discourage you or something here, I'm simply pointing out that this will be something that we will have to take into account. And it's possible that other drivers are in a similar situation. > > Unfortunately this change is not fully backwards-compatible. This > > requires that all DSI panels sending commands from prepare() should > > have the prepare_prev_first flag. In some sense, all such patches > > might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > > flag to drm_panel"). > > This kind of migration should be done *before* any possible > regression, not the other way round. > > If all panels sending commands from prepare() should have the > prepare_prev_first flag, then it should be first, check for > regressions then continue. > > <snip> > > > > > > > I understand, but this patch doesn't qualify as a fix for > > > 9e15123eca79 and is too late to be merged in drm-misc-next for > > > v6.6, and since 9e15123eca79 actually breaks some support it > > > should be reverted (+ deps) since we are late in the rc cycles. > > > > If we go this way, we can never reapply these patches. There will be > > no guarantee that all panel drivers are completely converted. We > > already have a story without an observable end - > > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > I don't understand this point, who would block re-applying the patches ? > > The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > Linux version and went smoothly because we reverted regressing patches > and restarted when needed, I don't understand why we can't do this > here aswell. > > > I'd consider that the DSI driver is correct here and it is about the > > panel drivers that require fixes patches. If you care about the > > particular Fixes tag, I have provided one several lines above. > > Unfortunately it should be done in the other way round, prepare for > migration, then migrate, > > I mean if it's a required migration, then it should be done and I'll > support it from both bridge and panel PoV. > > So, first this patch has the wrong Fixes tag, and I would like a > better explanation on the commit message in any case. Then I would > like to have an ack from some drm-misc maintainers before applying it > because it fixes a patch that was sent via the msm tree thus per the > drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. Sorry, it's not clear to me what you'd like our feedback on exactly? Maxime
Hi Maxime, On 21/08/2023 10:17, Maxime Ripard wrote: > Hi, > > On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>> Sending HS commands will always work on any controller, it's all >>>> about LP commands. The Samsung panels you listed only send HS >>>> commands so they can use prepare_prev_first and work on any >>>> controllers. >>> >>> I think there is some misunderstanding there, supported by the >>> description of the flag. >>> >>> If I remember correctly, some hosts (sunxi) can not send DCS >>> commands after enabling video stream and switching to HS mode, see >>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>> commands in drm_panel_funcs::prepare() / >>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>> whether these commands are to be sent in LP or in HS mode. >>> >>> Previously DSI source drivers could power on the DSI link either in >>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>> hack to make panel/bridge drivers to be able to send commands from >>> their prepare() / pre_enable() callbacks. >>> >>> With the prev_first flags being introduced, we have established that >>> DSI link should be enabled in DSI host's pre_enable() callback and >>> switched to HS mode (be it command or video) in the enable() >>> callback. >>> >>> So far so good. >> >> It seems coherent, I would like first to have a state of all DSI host >> drivers and make this would actually work first before adding the >> prev_first flag to all the required panels. > > This is definitely what we should do in an ideal world, but at least for > sunxi there's no easy way for it at the moment. There's no documentation > for it and the driver provided doesn't allow this to happen. > > Note that I'm not trying to discourage you or something here, I'm simply > pointing out that this will be something that we will have to take into > account. And it's possible that other drivers are in a similar > situation. > >>> Unfortunately this change is not fully backwards-compatible. This >>> requires that all DSI panels sending commands from prepare() should >>> have the prepare_prev_first flag. In some sense, all such patches >>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>> flag to drm_panel"). >> >> This kind of migration should be done *before* any possible >> regression, not the other way round. >> >> If all panels sending commands from prepare() should have the >> prepare_prev_first flag, then it should be first, check for >> regressions then continue. >> >> <snip> >> >>>> >>>> I understand, but this patch doesn't qualify as a fix for >>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>> should be reverted (+ deps) since we are late in the rc cycles. >>> >>> If we go this way, we can never reapply these patches. There will be >>> no guarantee that all panel drivers are completely converted. We >>> already have a story without an observable end - >>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >> >> I don't understand this point, who would block re-applying the patches ? >> >> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >> Linux version and went smoothly because we reverted regressing patches >> and restarted when needed, I don't understand why we can't do this >> here aswell. >> >>> I'd consider that the DSI driver is correct here and it is about the >>> panel drivers that require fixes patches. If you care about the >>> particular Fixes tag, I have provided one several lines above. >> >> Unfortunately it should be done in the other way round, prepare for >> migration, then migrate, >> >> I mean if it's a required migration, then it should be done and I'll >> support it from both bridge and panel PoV. >> >> So, first this patch has the wrong Fixes tag, and I would like a >> better explanation on the commit message in any case. Then I would >> like to have an ack from some drm-misc maintainers before applying it >> because it fixes a patch that was sent via the msm tree thus per the >> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > > Sorry, it's not clear to me what you'd like our feedback on exactly? So let me resume the situation: - pre_enable_prev_first was introduced in [1] - some panels made use of pre_enable_prev_first - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) - this fix was sent late, and is now too late to be merged via drm-misc-next I do not consider it's the right way to fix regression caused by [2] I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again I have no objection about [2] and it should be done widely over the whole DSI controllers and DSI Video panels. I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1] should be used but it's even more wrong since [2] really caused the regression. And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be applied via drm-misc-next-fixes, right ? [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") Thanks, Neil > > Maxime
Hi Dmitry On Fri, 18 Aug 2023 at 11:27, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 18/08/2023 11:25, neil.armstrong@linaro.org wrote: > > Hi Dmitry, > > > > On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>> Hi Abhinav, > >>> > >>> On 14/08/2023 20:02, Abhinav Kumar wrote: > > > > <snip> > > > >>> > >>> Sending HS commands will always work on any controller, it's all > >>> about LP commands. > >>> The Samsung panels you listed only send HS commands so they can use > >>> prepare_prev_first > >>> and work on any controllers. > >> > >> I think there is some misunderstanding there, supported by the > >> description of the flag. > >> > >> If I remember correctly, some hosts (sunxi) can not send DCS commands > >> after enabling video stream and switching to HS mode, see [1]. Thus, > >> as you know, most of the drivers have all DSI panel setup commands in > >> drm_panel_funcs::prepare() / drm_bridge_funcs::pre_enable() callbacks, > >> not paying attention whether these commands are to be sent in LP or in > >> HS mode. > >> > >> Previously DSI source drivers could power on the DSI link either in > >> mode_set() or in pre_enable() callbacks, with mode_set() being the > >> hack to make panel/bridge drivers to be able to send commands from > >> their prepare() / pre_enable() callbacks. > >> > >> With the prev_first flags being introduced, we have established that > >> DSI link should be enabled in DSI host's pre_enable() callback and > >> switched to HS mode (be it command or video) in the enable() callback. > >> > >> So far so good. > > > > It seems coherent, I would like first to have a state of all DSI host > > drivers and make this would actually work first before adding the > > prev_first flag to all the required panels. > > > >> > >> Unfortunately this change is not fully backwards-compatible. This > >> requires that all DSI panels sending commands from prepare() should > >> have the prepare_prev_first flag. In some sense, all such patches > >> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >> flag to drm_panel"). > > > > This kind of migration should be done *before* any possible regression, > > not the other way round. > > > > If all panels sending commands from prepare() should have the > > prepare_prev_first flag, then it should be first, check for regressions > > then continue. > > > > <snip> > > > >>> > >>> I understand, but this patch doesn't qualify as a fix for > >>> 9e15123eca79 and is too late to be merged in drm-misc-next for v6.6, > >>> and since 9e15123eca79 actually breaks some support it should be > >>> reverted (+ deps) since we are late in the rc cycles. > >> > >> If we go this way, we can never reapply these patches. There will be > >> no guarantee that all panel drivers are completely converted. We > >> already have a story without an observable end - > >> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > > > I don't understand this point, who would block re-applying the patches ? > > Consider us reverting 9e15123eca79 now and then reapplying it next > cycle. Then another panel / bridge that was not converted to use > pre_enable_prev_first pops up. And suddently we have to revert them again. > > > The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > > Linux version and went smoothly because we reverted > > regressing patches and restarted when needed, I don't understand why we > > can't do this here aswell. > > With DRM_BRIDGE_ATTACH_NO_CONNECTOR both host and peripheral drivers > were involved. This way they share knowledge about the migration state. > > With prev_first we do not have such shared knowledge. Host assumes that > it can work according to the documentation: turn DSI link to LP-11 in > pre_enable(), switch to HS in enable(). It can not check whether the > next bridge did not set pre_enable_prev_first because of it not being > required (like for the Parade bridge) or because next bridge is not > converted yet (and thus DSI host should power up the link in > atomic_mode_set). > > Granted that there is no way for the DSI host driver to attune itself to > the DSI peripheral driver requirements, I can only consider > corresponding (requiring prev_first) panel drivers broken since > 5ea6b1702781 ("drm/panel: Add prepare_prev_first flag to drm_panel") and > all bridge drivers with this issue broken since 4fb912e5e190 > ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order"). Can I point out that even prior to 5ea6b1702781 the docs stated [1] "Also note that those callbacks can be called no matter the state the host is in. Drivers that need the underlying device to be powered to perform these operations will first need to make sure it’s been properly enabled." added in bacbab58f09dc. So your DSI host driver isn't working in the documented manner prior to 5ea6b1702781, therefore 5ea6b1702781 doesn't cause a regression in itself, and there was no direct requirement for 5ea6b1702781 to add the flag to all panels. Looking at JDI LT070ME05000 [2], the backlight is controlled via DCS commands, therefore transfer can be called at any time to change or read the backlight intensity, not just between pre_enable and post_disable. 5ea6b1702781 and 4fb912e5e190 were largely trying to address the requirements of devices such as TI's SN65DSI83 that require the DSI data lanes to be in LP-11 before the bridge is brought out of reset otherwise they malfunction. Being able to add better definition over when the DSI host needs to be powered up/down is a bonus, but it doesn't trump the requirement for transfer to be callable at any time. Dave [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#c.mipi_dsi_host_ops [2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c > > > >> > >> I'd consider that the DSI driver is correct here and it is about the > >> panel drivers that require fixes patches. If you care about the > >> particular Fixes tag, I have provided one several lines above. > > > > Unfortunately it should be done in the other way round, prepare for > > migration, then migrate, > > > > I mean if it's a required migration, then it should be done and I'll > > support it from both bridge and panel PoV. > > > > So, first this patch has the wrong Fixes tag, and I would like a better > > explanation on the commit message in any case. > > Then I would like to have an ack from some drm-misc maintainers before > > applying it because it fixes a patch that > > was sent via the msm tree thus per the drm-misc rules I cannot apply it > > via the drm-misc-next-fixes tree. > > > > Neil > > > > <snip> > > > > -- > With best wishes > Dmitry >
On Mon, Aug 21, 2023 at 12:01:19PM +0200, neil.armstrong@linaro.org wrote: > Hi Maxime, > > On 21/08/2023 10:17, Maxime Ripard wrote: > > Hi, > > > > On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > > > On 17/08/2023 20:35, Dmitry Baryshkov wrote: > > > > On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > > > > > Sending HS commands will always work on any controller, it's all > > > > > about LP commands. The Samsung panels you listed only send HS > > > > > commands so they can use prepare_prev_first and work on any > > > > > controllers. > > > > > > > > I think there is some misunderstanding there, supported by the > > > > description of the flag. > > > > > > > > If I remember correctly, some hosts (sunxi) can not send DCS > > > > commands after enabling video stream and switching to HS mode, see > > > > [1]. Thus, as you know, most of the drivers have all DSI panel setup > > > > commands in drm_panel_funcs::prepare() / > > > > drm_bridge_funcs::pre_enable() callbacks, not paying attention > > > > whether these commands are to be sent in LP or in HS mode. > > > > > > > > Previously DSI source drivers could power on the DSI link either in > > > > mode_set() or in pre_enable() callbacks, with mode_set() being the > > > > hack to make panel/bridge drivers to be able to send commands from > > > > their prepare() / pre_enable() callbacks. > > > > > > > > With the prev_first flags being introduced, we have established that > > > > DSI link should be enabled in DSI host's pre_enable() callback and > > > > switched to HS mode (be it command or video) in the enable() > > > > callback. > > > > > > > > So far so good. > > > > > > It seems coherent, I would like first to have a state of all DSI host > > > drivers and make this would actually work first before adding the > > > prev_first flag to all the required panels. > > > > This is definitely what we should do in an ideal world, but at least for > > sunxi there's no easy way for it at the moment. There's no documentation > > for it and the driver provided doesn't allow this to happen. > > > > Note that I'm not trying to discourage you or something here, I'm simply > > pointing out that this will be something that we will have to take into > > account. And it's possible that other drivers are in a similar > > situation. > > > > > > Unfortunately this change is not fully backwards-compatible. This > > > > requires that all DSI panels sending commands from prepare() should > > > > have the prepare_prev_first flag. In some sense, all such patches > > > > might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > > > > flag to drm_panel"). > > > > > > This kind of migration should be done *before* any possible > > > regression, not the other way round. > > > > > > If all panels sending commands from prepare() should have the > > > prepare_prev_first flag, then it should be first, check for > > > regressions then continue. > > > > > > <snip> > > > > > > > > > > > > > I understand, but this patch doesn't qualify as a fix for > > > > > 9e15123eca79 and is too late to be merged in drm-misc-next for > > > > > v6.6, and since 9e15123eca79 actually breaks some support it > > > > > should be reverted (+ deps) since we are late in the rc cycles. > > > > > > > > If we go this way, we can never reapply these patches. There will be > > > > no guarantee that all panel drivers are completely converted. We > > > > already have a story without an observable end - > > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > > > > > I don't understand this point, who would block re-applying the patches ? > > > > > > The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > > > Linux version and went smoothly because we reverted regressing patches > > > and restarted when needed, I don't understand why we can't do this > > > here aswell. > > > > > > > I'd consider that the DSI driver is correct here and it is about the > > > > panel drivers that require fixes patches. If you care about the > > > > particular Fixes tag, I have provided one several lines above. > > > > > > Unfortunately it should be done in the other way round, prepare for > > > migration, then migrate, > > > > > > I mean if it's a required migration, then it should be done and I'll > > > support it from both bridge and panel PoV. > > > > > > So, first this patch has the wrong Fixes tag, and I would like a > > > better explanation on the commit message in any case. Then I would > > > like to have an ack from some drm-misc maintainers before applying it > > > because it fixes a patch that was sent via the msm tree thus per the > > > drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > > > > Sorry, it's not clear to me what you'd like our feedback on exactly? > > So let me resume the situation: > > - pre_enable_prev_first was introduced in [1] > - some panels made use of pre_enable_prev_first > - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > - this fix was sent late, and is now too late to be merged via drm-misc-next > > I do not consider it's the right way to fix regression caused by [2] I > consider [2] should be reverted, panels migrated to > pre_enable_prev_first when needed, tested and the [2] applied again > > I have no objection about [2] and it should be done widely over the > whole DSI controllers and DSI Video panels. > > I also object about the Fixes tag of this patch, which is wrong, and > Dmitry considers [1] should be used but it's even more wrong since [2] > really caused the regression. Ok. > And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be > applied via drm-misc-next-fixes, right ? AFAICT, 2 is in 6.5 now, so it would be drm-misc-fixes. But yeah, it would make more sense to take it through the MSM tree. Maxime
On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > Hi Maxime, > > On 21/08/2023 10:17, Maxime Ripard wrote: >> Hi, >> >> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org >> wrote: >>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>> Sending HS commands will always work on any controller, it's all >>>>> about LP commands. The Samsung panels you listed only send HS >>>>> commands so they can use prepare_prev_first and work on any >>>>> controllers. >>>> >>>> I think there is some misunderstanding there, supported by the >>>> description of the flag. >>>> >>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>> commands after enabling video stream and switching to HS mode, see >>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>> commands in drm_panel_funcs::prepare() / >>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>> whether these commands are to be sent in LP or in HS mode. >>>> >>>> Previously DSI source drivers could power on the DSI link either in >>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>> hack to make panel/bridge drivers to be able to send commands from >>>> their prepare() / pre_enable() callbacks. >>>> >>>> With the prev_first flags being introduced, we have established that >>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>> switched to HS mode (be it command or video) in the enable() >>>> callback. >>>> >>>> So far so good. >>> >>> It seems coherent, I would like first to have a state of all DSI host >>> drivers and make this would actually work first before adding the >>> prev_first flag to all the required panels. >> >> This is definitely what we should do in an ideal world, but at least for >> sunxi there's no easy way for it at the moment. There's no documentation >> for it and the driver provided doesn't allow this to happen. >> >> Note that I'm not trying to discourage you or something here, I'm simply >> pointing out that this will be something that we will have to take into >> account. And it's possible that other drivers are in a similar >> situation. >> >>>> Unfortunately this change is not fully backwards-compatible. This >>>> requires that all DSI panels sending commands from prepare() should >>>> have the prepare_prev_first flag. In some sense, all such patches >>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>> flag to drm_panel"). >>> >>> This kind of migration should be done *before* any possible >>> regression, not the other way round. >>> >>> If all panels sending commands from prepare() should have the >>> prepare_prev_first flag, then it should be first, check for >>> regressions then continue. >>> >>> <snip> >>> >>>>> >>>>> I understand, but this patch doesn't qualify as a fix for >>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>> >>>> If we go this way, we can never reapply these patches. There will be >>>> no guarantee that all panel drivers are completely converted. We >>>> already have a story without an observable end - >>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>> >>> I don't understand this point, who would block re-applying the patches ? >>> >>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>> Linux version and went smoothly because we reverted regressing patches >>> and restarted when needed, I don't understand why we can't do this >>> here aswell. >>> >>>> I'd consider that the DSI driver is correct here and it is about the >>>> panel drivers that require fixes patches. If you care about the >>>> particular Fixes tag, I have provided one several lines above. >>> >>> Unfortunately it should be done in the other way round, prepare for >>> migration, then migrate, >>> >>> I mean if it's a required migration, then it should be done and I'll >>> support it from both bridge and panel PoV. >>> >>> So, first this patch has the wrong Fixes tag, and I would like a >>> better explanation on the commit message in any case. Then I would >>> like to have an ack from some drm-misc maintainers before applying it >>> because it fixes a patch that was sent via the msm tree thus per the >>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >> >> Sorry, it's not clear to me what you'd like our feedback on exactly? > > So let me resume the situation: > > - pre_enable_prev_first was introduced in [1] > - some panels made use of pre_enable_prev_first > - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 > kernels and before > - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 > systems (and probably other Video mode panels on Qcom platforms) > - this fix was sent late, and is now too late to be merged via > drm-misc-next Hi Neil and Maxime, I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). However, I'm not too keen on simply reverting that patch because 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. Thanks, Jessica Zhang > > I do not consider it's the right way to fix regression caused by [2] > I consider [2] should be reverted, panels migrated to > pre_enable_prev_first when needed, tested and the [2] applied again > > I have no objection about [2] and it should be done widely over the > whole DSI controllers > and DSI Video panels. > > I also object about the Fixes tag of this patch, which is wrong, and > Dmitry considers [1] > should be used but it's even more wrong since [2] really caused the > regression. > > And if [2] was to correct one to use, it was pushed via the MSM tree so > it couldn't be > applied via drm-misc-next-fixes, right ? > > [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter > bridge init order") > [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI > hosts at modeset") > > Thanks, > Neil > >> >> Maxime >
Hi Jessica, On 25/08/2023 20:37, Jessica Zhang wrote: > > > On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >> Hi Maxime, >> >> On 21/08/2023 10:17, Maxime Ripard wrote: >>> Hi, >>> >>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>> Sending HS commands will always work on any controller, it's all >>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>> commands so they can use prepare_prev_first and work on any >>>>>> controllers. >>>>> >>>>> I think there is some misunderstanding there, supported by the >>>>> description of the flag. >>>>> >>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>> commands after enabling video stream and switching to HS mode, see >>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>> commands in drm_panel_funcs::prepare() / >>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>> whether these commands are to be sent in LP or in HS mode. >>>>> >>>>> Previously DSI source drivers could power on the DSI link either in >>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>> hack to make panel/bridge drivers to be able to send commands from >>>>> their prepare() / pre_enable() callbacks. >>>>> >>>>> With the prev_first flags being introduced, we have established that >>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>> switched to HS mode (be it command or video) in the enable() >>>>> callback. >>>>> >>>>> So far so good. >>>> >>>> It seems coherent, I would like first to have a state of all DSI host >>>> drivers and make this would actually work first before adding the >>>> prev_first flag to all the required panels. >>> >>> This is definitely what we should do in an ideal world, but at least for >>> sunxi there's no easy way for it at the moment. There's no documentation >>> for it and the driver provided doesn't allow this to happen. >>> >>> Note that I'm not trying to discourage you or something here, I'm simply >>> pointing out that this will be something that we will have to take into >>> account. And it's possible that other drivers are in a similar >>> situation. >>> >>>>> Unfortunately this change is not fully backwards-compatible. This >>>>> requires that all DSI panels sending commands from prepare() should >>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>> flag to drm_panel"). >>>> >>>> This kind of migration should be done *before* any possible >>>> regression, not the other way round. >>>> >>>> If all panels sending commands from prepare() should have the >>>> prepare_prev_first flag, then it should be first, check for >>>> regressions then continue. >>>> >>>> <snip> >>>> >>>>>> >>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>> >>>>> If we go this way, we can never reapply these patches. There will be >>>>> no guarantee that all panel drivers are completely converted. We >>>>> already have a story without an observable end - >>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>> >>>> I don't understand this point, who would block re-applying the patches ? >>>> >>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>> Linux version and went smoothly because we reverted regressing patches >>>> and restarted when needed, I don't understand why we can't do this >>>> here aswell. >>>> >>>>> I'd consider that the DSI driver is correct here and it is about the >>>>> panel drivers that require fixes patches. If you care about the >>>>> particular Fixes tag, I have provided one several lines above. >>>> >>>> Unfortunately it should be done in the other way round, prepare for >>>> migration, then migrate, >>>> >>>> I mean if it's a required migration, then it should be done and I'll >>>> support it from both bridge and panel PoV. >>>> >>>> So, first this patch has the wrong Fixes tag, and I would like a >>>> better explanation on the commit message in any case. Then I would >>>> like to have an ack from some drm-misc maintainers before applying it >>>> because it fixes a patch that was sent via the msm tree thus per the >>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>> >>> Sorry, it's not clear to me what you'd like our feedback on exactly? >> >> So let me resume the situation: >> >> - pre_enable_prev_first was introduced in [1] >> - some panels made use of pre_enable_prev_first >> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >> - this fix was sent late, and is now too late to be merged via drm-misc-next > > Hi Neil and Maxime, > > I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > > However, I'm not too keen on simply reverting that patch because > > 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and I never objected that, it's the right path to go. > > 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. We discussed about a plan with Maxime and Dmitry about that, and it would require adding a proper atomic panel API to handle a "negociation" with the host controller. > > > That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > > It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. A proper migration should be done, yes, but not as a fix on top of v6.5. Neil > > Thanks, > > Jessica Zhang > > >> >> I do not consider it's the right way to fix regression caused by [2] >> I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again >> >> I have no objection about [2] and it should be done widely over the whole DSI controllers >> and DSI Video panels. >> >> I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1] >> should be used but it's even more wrong since [2] really caused the regression. >> >> And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be >> applied via drm-misc-next-fixes, right ? >> >> [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") >> [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") >> >> Thanks, >> Neil >> >>> >>> Maxime >>
Hi Neil Sorry I didnt respond earlier on this thread. On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: > Hi Jessica, > > On 25/08/2023 20:37, Jessica Zhang wrote: >> >> >> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>> Hi Maxime, >>> >>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>> Hi, >>>> >>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org >>>> wrote: >>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>> controllers. >>>>>> >>>>>> I think there is some misunderstanding there, supported by the >>>>>> description of the flag. >>>>>> >>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>> commands after enabling video stream and switching to HS mode, see >>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>> commands in drm_panel_funcs::prepare() / >>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>> >>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>> their prepare() / pre_enable() callbacks. >>>>>> >>>>>> With the prev_first flags being introduced, we have established that >>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>> switched to HS mode (be it command or video) in the enable() >>>>>> callback. >>>>>> >>>>>> So far so good. >>>>> >>>>> It seems coherent, I would like first to have a state of all DSI host >>>>> drivers and make this would actually work first before adding the >>>>> prev_first flag to all the required panels. >>>> >>>> This is definitely what we should do in an ideal world, but at least >>>> for >>>> sunxi there's no easy way for it at the moment. There's no >>>> documentation >>>> for it and the driver provided doesn't allow this to happen. >>>> >>>> Note that I'm not trying to discourage you or something here, I'm >>>> simply >>>> pointing out that this will be something that we will have to take into >>>> account. And it's possible that other drivers are in a similar >>>> situation. >>>> >>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>> requires that all DSI panels sending commands from prepare() should >>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>> flag to drm_panel"). >>>>> >>>>> This kind of migration should be done *before* any possible >>>>> regression, not the other way round. >>>>> >>>>> If all panels sending commands from prepare() should have the >>>>> prepare_prev_first flag, then it should be first, check for >>>>> regressions then continue. >>>>> >>>>> <snip> >>>>> >>>>>>> >>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>> >>>>>> If we go this way, we can never reapply these patches. There will be >>>>>> no guarantee that all panel drivers are completely converted. We >>>>>> already have a story without an observable end - >>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>> >>>>> I don't understand this point, who would block re-applying the >>>>> patches ? >>>>> >>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>> Linux version and went smoothly because we reverted regressing patches >>>>> and restarted when needed, I don't understand why we can't do this >>>>> here aswell. >>>>> >>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>> panel drivers that require fixes patches. If you care about the >>>>>> particular Fixes tag, I have provided one several lines above. >>>>> >>>>> Unfortunately it should be done in the other way round, prepare for >>>>> migration, then migrate, >>>>> >>>>> I mean if it's a required migration, then it should be done and I'll >>>>> support it from both bridge and panel PoV. >>>>> >>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>> better explanation on the commit message in any case. Then I would >>>>> like to have an ack from some drm-misc maintainers before applying it >>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>> >>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>> >>> So let me resume the situation: >>> >>> - pre_enable_prev_first was introduced in [1] >>> - some panels made use of pre_enable_prev_first >>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 >>> kernels and before >>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on >>> SM8550 systems (and probably other Video mode panels on Qcom platforms) >>> - this fix was sent late, and is now too late to be merged via >>> drm-misc-next >> >> Hi Neil and Maxime, >> >> I agree with Neil that 9e15123eca79 was the commit that introduced the >> issue (since it changed the MSM DSI host behavior). >> >> However, I'm not too keen on simply reverting that patch because >> >> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it >> actually makes more sense to power on DSI host in pre_enable than in >> modeset (since modeset is meant for setting the bridge mode), and > > I never objected that, it's the right path to go. > Ack. >> >> 2) I think it would be good practice to keep specific bridge chip >> checks out of the DSI host driver. > > We discussed about a plan with Maxime and Dmitry about that, and it > would require adding > a proper atomic panel API to handle a "negociation" with the host > controller. > May I know what type of negotiation is needed here? >> >> >> That being said, what do you think about setting the default value of >> prepare_prev_first to true (possibly in panel_bridge_attach)? > > As Dmitry pointed, all panels sending LP commands in pre_enable() should > have prepare_prev_first to true. > I wanted to respond to this earlier but didnt get a chance. From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. 195 * The previous controller should be prepared first, before the prepare 196 * for the panel is called. This is largely required for DSI panels 197 * where the DSI host controller should be initialised to LP-11 before 198 * the panel is powered up. 199 */ 200 bool prepare_prev_first; These are conceptually different and thats what I explained Dmitry in our call. Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. From the panels which I have seen, the opposite is never true (OR i have never seen it this way). The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. >> >> It seems to me that most panel drivers send DCS commands during >> pre_enable, so maybe it would make more sense to power on DSI host >> before panel enable() by default. Any panel that needs DSI host to be >> powered on later could then explicitly set the flag to false in their >> respective drivers. > > A proper migration should be done, yes, but not as a fix on top of v6.5. > I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. Let me know if this works as a migration plan. > Neil > >> >> Thanks, >> >> Jessica Zhang >> >> >>> >>> I do not consider it's the right way to fix regression caused by [2] >>> I consider [2] should be reverted, panels migrated to >>> pre_enable_prev_first when needed, tested and the [2] applied again >>> >>> I have no objection about [2] and it should be done widely over the >>> whole DSI controllers >>> and DSI Video panels. >>> >>> I also object about the Fixes tag of this patch, which is wrong, and >>> Dmitry considers [1] >>> should be used but it's even more wrong since [2] really caused the >>> regression. >>> >>> And if [2] was to correct one to use, it was pushed via the MSM tree >>> so it couldn't be >>> applied via drm-misc-next-fixes, right ? >>> >>> [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to >>> alter bridge init order") >>> [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI >>> hosts at modeset") >>> >>> Thanks, >>> Neil >>> >>>> >>>> Maxime >>> >
On 28/08/2023 19:07, Abhinav Kumar wrote: > Hi Neil > > Sorry I didnt respond earlier on this thread. > > On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: >> Hi Jessica, >> >> On 25/08/2023 20:37, Jessica Zhang wrote: >>> >>> >>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>> Hi Maxime, >>>> >>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>> Hi, >>>>> >>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>> controllers. >>>>>>> >>>>>>> I think there is some misunderstanding there, supported by the >>>>>>> description of the flag. >>>>>>> >>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>> >>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>> their prepare() / pre_enable() callbacks. >>>>>>> >>>>>>> With the prev_first flags being introduced, we have established that >>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>> callback. >>>>>>> >>>>>>> So far so good. >>>>>> >>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>> drivers and make this would actually work first before adding the >>>>>> prev_first flag to all the required panels. >>>>> >>>>> This is definitely what we should do in an ideal world, but at least for >>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>> for it and the driver provided doesn't allow this to happen. >>>>> >>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>> pointing out that this will be something that we will have to take into >>>>> account. And it's possible that other drivers are in a similar >>>>> situation. >>>>> >>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>> flag to drm_panel"). >>>>>> >>>>>> This kind of migration should be done *before* any possible >>>>>> regression, not the other way round. >>>>>> >>>>>> If all panels sending commands from prepare() should have the >>>>>> prepare_prev_first flag, then it should be first, check for >>>>>> regressions then continue. >>>>>> >>>>>> <snip> >>>>>> >>>>>>>> >>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>> >>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>> already have a story without an observable end - >>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>> >>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>> >>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>> and restarted when needed, I don't understand why we can't do this >>>>>> here aswell. >>>>>> >>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>> >>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>> migration, then migrate, >>>>>> >>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>> support it from both bridge and panel PoV. >>>>>> >>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>> better explanation on the commit message in any case. Then I would >>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>> >>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>> >>>> So let me resume the situation: >>>> >>>> - pre_enable_prev_first was introduced in [1] >>>> - some panels made use of pre_enable_prev_first >>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>> >>> Hi Neil and Maxime, >>> >>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>> >>> However, I'm not too keen on simply reverting that patch because >>> >>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >> >> I never objected that, it's the right path to go. >> > > Ack. > >>> >>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >> >> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >> a proper atomic panel API to handle a "negociation" with the host controller. >> > > May I know what type of negotiation is needed here? > >>> >>> >>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >> >> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. >> > > I wanted to respond to this earlier but didnt get a chance. > > From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). > > This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. > > 195 * The previous controller should be prepared first, before the prepare > 196 * for the panel is called. This is largely required for DSI panels > 197 * where the DSI host controller should be initialised to LP-11 before > 198 * the panel is powered up. > 199 */ > 200 bool prepare_prev_first; > > These are conceptually different and thats what I explained Dmitry in our call. > > Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. > > From the panels which I have seen, the opposite is never true (OR i have never seen it this way). > > The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. > > In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. > > So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. > >>> >>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >> >> A proper migration should be done, yes, but not as a fix on top of v6.5. >> > > I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. > > Let me know if this works as a migration plan. Yep agreed, let's do this The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. Neil > >> Neil >> >>> >>> Thanks, >>> >>> Jessica Zhang >>> >>> >>>> >>>> I do not consider it's the right way to fix regression caused by [2] >>>> I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again >>>> >>>> I have no objection about [2] and it should be done widely over the whole DSI controllers >>>> and DSI Video panels. >>>> >>>> I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1] >>>> should be used but it's even more wrong since [2] really caused the regression. >>>> >>>> And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be >>>> applied via drm-misc-next-fixes, right ? >>>> >>>> [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") >>>> [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") >>>> >>>> Thanks, >>>> Neil >>>> >>>>> >>>>> Maxime >>>> >>
On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: > > On 28/08/2023 19:07, Abhinav Kumar wrote: > > Hi Neil > > > > Sorry I didnt respond earlier on this thread. > > > > On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: > >> Hi Jessica, > >> > >> On 25/08/2023 20:37, Jessica Zhang wrote: > >>> > >>> > >>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > >>>> Hi Maxime, > >>>> > >>>> On 21/08/2023 10:17, Maxime Ripard wrote: > >>>>> Hi, > >>>>> > >>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > >>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>>>>>>> Sending HS commands will always work on any controller, it's all > >>>>>>>> about LP commands. The Samsung panels you listed only send HS > >>>>>>>> commands so they can use prepare_prev_first and work on any > >>>>>>>> controllers. > >>>>>>> > >>>>>>> I think there is some misunderstanding there, supported by the > >>>>>>> description of the flag. > >>>>>>> > >>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS > >>>>>>> commands after enabling video stream and switching to HS mode, see > >>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup > >>>>>>> commands in drm_panel_funcs::prepare() / > >>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention > >>>>>>> whether these commands are to be sent in LP or in HS mode. > >>>>>>> > >>>>>>> Previously DSI source drivers could power on the DSI link either in > >>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the > >>>>>>> hack to make panel/bridge drivers to be able to send commands from > >>>>>>> their prepare() / pre_enable() callbacks. > >>>>>>> > >>>>>>> With the prev_first flags being introduced, we have established that > >>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and > >>>>>>> switched to HS mode (be it command or video) in the enable() > >>>>>>> callback. > >>>>>>> > >>>>>>> So far so good. > >>>>>> > >>>>>> It seems coherent, I would like first to have a state of all DSI host > >>>>>> drivers and make this would actually work first before adding the > >>>>>> prev_first flag to all the required panels. > >>>>> > >>>>> This is definitely what we should do in an ideal world, but at least for > >>>>> sunxi there's no easy way for it at the moment. There's no documentation > >>>>> for it and the driver provided doesn't allow this to happen. > >>>>> > >>>>> Note that I'm not trying to discourage you or something here, I'm simply > >>>>> pointing out that this will be something that we will have to take into > >>>>> account. And it's possible that other drivers are in a similar > >>>>> situation. > >>>>> > >>>>>>> Unfortunately this change is not fully backwards-compatible. This > >>>>>>> requires that all DSI panels sending commands from prepare() should > >>>>>>> have the prepare_prev_first flag. In some sense, all such patches > >>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >>>>>>> flag to drm_panel"). > >>>>>> > >>>>>> This kind of migration should be done *before* any possible > >>>>>> regression, not the other way round. > >>>>>> > >>>>>> If all panels sending commands from prepare() should have the > >>>>>> prepare_prev_first flag, then it should be first, check for > >>>>>> regressions then continue. > >>>>>> > >>>>>> <snip> > >>>>>> > >>>>>>>> > >>>>>>>> I understand, but this patch doesn't qualify as a fix for > >>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for > >>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it > >>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. > >>>>>>> > >>>>>>> If we go this way, we can never reapply these patches. There will be > >>>>>>> no guarantee that all panel drivers are completely converted. We > >>>>>>> already have a story without an observable end - > >>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >>>>>> > >>>>>> I don't understand this point, who would block re-applying the patches ? > >>>>>> > >>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > >>>>>> Linux version and went smoothly because we reverted regressing patches > >>>>>> and restarted when needed, I don't understand why we can't do this > >>>>>> here aswell. > >>>>>> > >>>>>>> I'd consider that the DSI driver is correct here and it is about the > >>>>>>> panel drivers that require fixes patches. If you care about the > >>>>>>> particular Fixes tag, I have provided one several lines above. > >>>>>> > >>>>>> Unfortunately it should be done in the other way round, prepare for > >>>>>> migration, then migrate, > >>>>>> > >>>>>> I mean if it's a required migration, then it should be done and I'll > >>>>>> support it from both bridge and panel PoV. > >>>>>> > >>>>>> So, first this patch has the wrong Fixes tag, and I would like a > >>>>>> better explanation on the commit message in any case. Then I would > >>>>>> like to have an ack from some drm-misc maintainers before applying it > >>>>>> because it fixes a patch that was sent via the msm tree thus per the > >>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > >>>>> > >>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? > >>>> > >>>> So let me resume the situation: > >>>> > >>>> - pre_enable_prev_first was introduced in [1] > >>>> - some panels made use of pre_enable_prev_first > >>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > >>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > >>>> - this fix was sent late, and is now too late to be merged via drm-misc-next > >>> > >>> Hi Neil and Maxime, > >>> > >>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > >>> > >>> However, I'm not too keen on simply reverting that patch because > >>> > >>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and > >> > >> I never objected that, it's the right path to go. > >> > > > > Ack. > > > >>> > >>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. > >> > >> We discussed about a plan with Maxime and Dmitry about that, and it would require adding > >> a proper atomic panel API to handle a "negociation" with the host controller. > >> > > > > May I know what type of negotiation is needed here? > > > >>> > >>> > >>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? > >> > >> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > >> > > > > I wanted to respond to this earlier but didnt get a chance. > > > > From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). > > > > This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. > > > > 195 * The previous controller should be prepared first, before the prepare > > 196 * for the panel is called. This is largely required for DSI panels > > 197 * where the DSI host controller should be initialised to LP-11 before > > 198 * the panel is powered up. > > 199 */ > > 200 bool prepare_prev_first; > > > > These are conceptually different and thats what I explained Dmitry in our call. > > > > Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. > > > > From the panels which I have seen, the opposite is never true (OR i have never seen it this way). > > > > The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. > > > > In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. > > > > So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. > > > >>> > >>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. > >> > >> A proper migration should be done, yes, but not as a fix on top of v6.5. > >> > > > > I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. > > > > Let me know if this works as a migration plan. > > Yep agreed, let's do this > > The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. This will break all non-DSI panels, which might depend on the current bridge calling order. I started looking at the explicit DSI power up sequencing, but it will take a few more days to mature.
Hi Neil On Mon, 28 Aug 2023 at 09:49, <neil.armstrong@linaro.org> wrote: > > Hi Jessica, > > On 25/08/2023 20:37, Jessica Zhang wrote: > > > > > > On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > >> Hi Maxime, > >> > >> On 21/08/2023 10:17, Maxime Ripard wrote: > >>> Hi, > >>> > >>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > >>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>>>>> Sending HS commands will always work on any controller, it's all > >>>>>> about LP commands. The Samsung panels you listed only send HS > >>>>>> commands so they can use prepare_prev_first and work on any > >>>>>> controllers. > >>>>> > >>>>> I think there is some misunderstanding there, supported by the > >>>>> description of the flag. > >>>>> > >>>>> If I remember correctly, some hosts (sunxi) can not send DCS > >>>>> commands after enabling video stream and switching to HS mode, see > >>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup > >>>>> commands in drm_panel_funcs::prepare() / > >>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention > >>>>> whether these commands are to be sent in LP or in HS mode. > >>>>> > >>>>> Previously DSI source drivers could power on the DSI link either in > >>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the > >>>>> hack to make panel/bridge drivers to be able to send commands from > >>>>> their prepare() / pre_enable() callbacks. > >>>>> > >>>>> With the prev_first flags being introduced, we have established that > >>>>> DSI link should be enabled in DSI host's pre_enable() callback and > >>>>> switched to HS mode (be it command or video) in the enable() > >>>>> callback. > >>>>> > >>>>> So far so good. > >>>> > >>>> It seems coherent, I would like first to have a state of all DSI host > >>>> drivers and make this would actually work first before adding the > >>>> prev_first flag to all the required panels. > >>> > >>> This is definitely what we should do in an ideal world, but at least for > >>> sunxi there's no easy way for it at the moment. There's no documentation > >>> for it and the driver provided doesn't allow this to happen. > >>> > >>> Note that I'm not trying to discourage you or something here, I'm simply > >>> pointing out that this will be something that we will have to take into > >>> account. And it's possible that other drivers are in a similar > >>> situation. > >>> > >>>>> Unfortunately this change is not fully backwards-compatible. This > >>>>> requires that all DSI panels sending commands from prepare() should > >>>>> have the prepare_prev_first flag. In some sense, all such patches > >>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >>>>> flag to drm_panel"). > >>>> > >>>> This kind of migration should be done *before* any possible > >>>> regression, not the other way round. > >>>> > >>>> If all panels sending commands from prepare() should have the > >>>> prepare_prev_first flag, then it should be first, check for > >>>> regressions then continue. > >>>> > >>>> <snip> > >>>> > >>>>>> > >>>>>> I understand, but this patch doesn't qualify as a fix for > >>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for > >>>>>> v6.6, and since 9e15123eca79 actually breaks some support it > >>>>>> should be reverted (+ deps) since we are late in the rc cycles. > >>>>> > >>>>> If we go this way, we can never reapply these patches. There will be > >>>>> no guarantee that all panel drivers are completely converted. We > >>>>> already have a story without an observable end - > >>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >>>> > >>>> I don't understand this point, who would block re-applying the patches ? > >>>> > >>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > >>>> Linux version and went smoothly because we reverted regressing patches > >>>> and restarted when needed, I don't understand why we can't do this > >>>> here aswell. > >>>> > >>>>> I'd consider that the DSI driver is correct here and it is about the > >>>>> panel drivers that require fixes patches. If you care about the > >>>>> particular Fixes tag, I have provided one several lines above. > >>>> > >>>> Unfortunately it should be done in the other way round, prepare for > >>>> migration, then migrate, > >>>> > >>>> I mean if it's a required migration, then it should be done and I'll > >>>> support it from both bridge and panel PoV. > >>>> > >>>> So, first this patch has the wrong Fixes tag, and I would like a > >>>> better explanation on the commit message in any case. Then I would > >>>> like to have an ack from some drm-misc maintainers before applying it > >>>> because it fixes a patch that was sent via the msm tree thus per the > >>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > >>> > >>> Sorry, it's not clear to me what you'd like our feedback on exactly? > >> > >> So let me resume the situation: > >> > >> - pre_enable_prev_first was introduced in [1] > >> - some panels made use of pre_enable_prev_first > >> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > >> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > >> - this fix was sent late, and is now too late to be merged via drm-misc-next > > > > Hi Neil and Maxime, > > > > I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > > > > However, I'm not too keen on simply reverting that patch because > > > > 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and > > I never objected that, it's the right path to go. > > > > > 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. > > We discussed about a plan with Maxime and Dmitry about that, and it would require adding > a proper atomic panel API to handle a "negociation" with the host controller. > > > > > > > That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? > > As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. Any panel wishing the clock and data lanes to be in a defined LP-11 state before pre_enable() is called need to set prepare_prev_first to true. This is not a universal requirement of all DSI peripherals for which commands are sent from pre_enable - a number will happily power up at LP-00. It is true that no harm will occur on those devices that do support non-LP-11 power up if the host is in LP-11, so a blanket setting of the flag for any panel driver sending DSI commands in pre_enable should be safe. It is documented [1] that transfer can be called at any time, regardless of the state of the host. The MSM driver isn't supporting that, hence issues. [2] further clarifies that it is expected to power up the host controller, send the message, and power down again. [1] https://github.com/torvalds/linux/blob/master/include/drm/drm_mipi_dsi.h#L84-L87 [2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L185-L188 > > > > It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. > > A proper migration should be done, yes, but not as a fix on top of v6.5. I looked at this when adding prepare_prev_first, but as the DSI control path is separate from the bridge chain there's no obvious way to automatically set a bridge flag from the mipi_dsi registration. Dave > Neil > > > > > Thanks, > > > > Jessica Zhang > > > > > >> > >> I do not consider it's the right way to fix regression caused by [2] > >> I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again > >> > >> I have no objection about [2] and it should be done widely over the whole DSI controllers > >> and DSI Video panels. > >> > >> I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1] > >> should be used but it's even more wrong since [2] really caused the regression. > >> > >> And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be > >> applied via drm-misc-next-fixes, right ? > >> > >> [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") > >> [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") > >> > >> Thanks, > >> Neil > >> > >>> > >>> Maxime > >> >
On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: > On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: >> >> On 28/08/2023 19:07, Abhinav Kumar wrote: >>> Hi Neil >>> >>> Sorry I didnt respond earlier on this thread. >>> >>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: >>>> Hi Jessica, >>>> >>>> On 25/08/2023 20:37, Jessica Zhang wrote: >>>>> >>>>> >>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>>>> Hi Maxime, >>>>>> >>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>>>> controllers. >>>>>>>>> >>>>>>>>> I think there is some misunderstanding there, supported by the >>>>>>>>> description of the flag. >>>>>>>>> >>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>>>> >>>>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>>>> their prepare() / pre_enable() callbacks. >>>>>>>>> >>>>>>>>> With the prev_first flags being introduced, we have established that >>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>>>> callback. >>>>>>>>> >>>>>>>>> So far so good. >>>>>>>> >>>>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>>>> drivers and make this would actually work first before adding the >>>>>>>> prev_first flag to all the required panels. >>>>>>> >>>>>>> This is definitely what we should do in an ideal world, but at least for >>>>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>>>> for it and the driver provided doesn't allow this to happen. >>>>>>> >>>>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>>>> pointing out that this will be something that we will have to take into >>>>>>> account. And it's possible that other drivers are in a similar >>>>>>> situation. >>>>>>> >>>>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>>>> flag to drm_panel"). >>>>>>>> >>>>>>>> This kind of migration should be done *before* any possible >>>>>>>> regression, not the other way round. >>>>>>>> >>>>>>>> If all panels sending commands from prepare() should have the >>>>>>>> prepare_prev_first flag, then it should be first, check for >>>>>>>> regressions then continue. >>>>>>>> >>>>>>>> <snip> >>>>>>>> >>>>>>>>>> >>>>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>>>> >>>>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>>>> already have a story without an observable end - >>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>>>> >>>>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>>>> >>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>>>> and restarted when needed, I don't understand why we can't do this >>>>>>>> here aswell. >>>>>>>> >>>>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>>>> >>>>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>>>> migration, then migrate, >>>>>>>> >>>>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>>>> support it from both bridge and panel PoV. >>>>>>>> >>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>>>> better explanation on the commit message in any case. Then I would >>>>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>>>> >>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>>>> >>>>>> So let me resume the situation: >>>>>> >>>>>> - pre_enable_prev_first was introduced in [1] >>>>>> - some panels made use of pre_enable_prev_first >>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>>>> >>>>> Hi Neil and Maxime, >>>>> >>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>>>> >>>>> However, I'm not too keen on simply reverting that patch because >>>>> >>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >>>> >>>> I never objected that, it's the right path to go. >>>> >>> >>> Ack. >>> >>>>> >>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >>>> >>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >>>> a proper atomic panel API to handle a "negociation" with the host controller. >>>> >>> >>> May I know what type of negotiation is needed here? >>> >>>>> >>>>> >>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >>>> >>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. >>>> >>> >>> I wanted to respond to this earlier but didnt get a chance. >>> >>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). >>> >>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. >>> >>> 195 * The previous controller should be prepared first, before the prepare >>> 196 * for the panel is called. This is largely required for DSI panels >>> 197 * where the DSI host controller should be initialised to LP-11 before >>> 198 * the panel is powered up. >>> 199 */ >>> 200 bool prepare_prev_first; >>> >>> These are conceptually different and thats what I explained Dmitry in our call. >>> >>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. >>> >>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). >>> >>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. >>> >>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. >>> >>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. >>> >>>>> >>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >>>> >>>> A proper migration should be done, yes, but not as a fix on top of v6.5. >>>> >>> >>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. >>> >>> Let me know if this works as a migration plan. >> >> Yep agreed, let's do this >> >> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. > > This will break all non-DSI panels, which might depend on the current > bridge calling order. > > I started looking at the explicit DSI power up sequencing, but it will > take a few more days to mature. > May I know why this would break all non-DSI panels? Like I said, we dont know the full details of the parade issue but I do not see any reason why powering on a bridge chip with the DSI lanes being in proper LP11 state should cause an issue. Its a well defined and documented state in DSI spec. On the contrary, trying to turn on a bridge chip before powering on a controller could have more issues as we do not know what state the lanes are in when the MIPI devices (panel or bridge) are powered up. This sets the expectation and handshake straight. >
On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: > > On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: > >> > >> On 28/08/2023 19:07, Abhinav Kumar wrote: > >>> Hi Neil > >>> > >>> Sorry I didnt respond earlier on this thread. > >>> > >>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: > >>>> Hi Jessica, > >>>> > >>>> On 25/08/2023 20:37, Jessica Zhang wrote: > >>>>> > >>>>> > >>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > >>>>>> Hi Maxime, > >>>>>> > >>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > >>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>>>>>>>>> Sending HS commands will always work on any controller, it's all > >>>>>>>>>> about LP commands. The Samsung panels you listed only send HS > >>>>>>>>>> commands so they can use prepare_prev_first and work on any > >>>>>>>>>> controllers. > >>>>>>>>> > >>>>>>>>> I think there is some misunderstanding there, supported by the > >>>>>>>>> description of the flag. > >>>>>>>>> > >>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS > >>>>>>>>> commands after enabling video stream and switching to HS mode, see > >>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup > >>>>>>>>> commands in drm_panel_funcs::prepare() / > >>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention > >>>>>>>>> whether these commands are to be sent in LP or in HS mode. > >>>>>>>>> > >>>>>>>>> Previously DSI source drivers could power on the DSI link either in > >>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the > >>>>>>>>> hack to make panel/bridge drivers to be able to send commands from > >>>>>>>>> their prepare() / pre_enable() callbacks. > >>>>>>>>> > >>>>>>>>> With the prev_first flags being introduced, we have established that > >>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and > >>>>>>>>> switched to HS mode (be it command or video) in the enable() > >>>>>>>>> callback. > >>>>>>>>> > >>>>>>>>> So far so good. > >>>>>>>> > >>>>>>>> It seems coherent, I would like first to have a state of all DSI host > >>>>>>>> drivers and make this would actually work first before adding the > >>>>>>>> prev_first flag to all the required panels. > >>>>>>> > >>>>>>> This is definitely what we should do in an ideal world, but at least for > >>>>>>> sunxi there's no easy way for it at the moment. There's no documentation > >>>>>>> for it and the driver provided doesn't allow this to happen. > >>>>>>> > >>>>>>> Note that I'm not trying to discourage you or something here, I'm simply > >>>>>>> pointing out that this will be something that we will have to take into > >>>>>>> account. And it's possible that other drivers are in a similar > >>>>>>> situation. > >>>>>>> > >>>>>>>>> Unfortunately this change is not fully backwards-compatible. This > >>>>>>>>> requires that all DSI panels sending commands from prepare() should > >>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches > >>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >>>>>>>>> flag to drm_panel"). > >>>>>>>> > >>>>>>>> This kind of migration should be done *before* any possible > >>>>>>>> regression, not the other way round. > >>>>>>>> > >>>>>>>> If all panels sending commands from prepare() should have the > >>>>>>>> prepare_prev_first flag, then it should be first, check for > >>>>>>>> regressions then continue. > >>>>>>>> > >>>>>>>> <snip> > >>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I understand, but this patch doesn't qualify as a fix for > >>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for > >>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it > >>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. > >>>>>>>>> > >>>>>>>>> If we go this way, we can never reapply these patches. There will be > >>>>>>>>> no guarantee that all panel drivers are completely converted. We > >>>>>>>>> already have a story without an observable end - > >>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >>>>>>>> > >>>>>>>> I don't understand this point, who would block re-applying the patches ? > >>>>>>>> > >>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > >>>>>>>> Linux version and went smoothly because we reverted regressing patches > >>>>>>>> and restarted when needed, I don't understand why we can't do this > >>>>>>>> here aswell. > >>>>>>>> > >>>>>>>>> I'd consider that the DSI driver is correct here and it is about the > >>>>>>>>> panel drivers that require fixes patches. If you care about the > >>>>>>>>> particular Fixes tag, I have provided one several lines above. > >>>>>>>> > >>>>>>>> Unfortunately it should be done in the other way round, prepare for > >>>>>>>> migration, then migrate, > >>>>>>>> > >>>>>>>> I mean if it's a required migration, then it should be done and I'll > >>>>>>>> support it from both bridge and panel PoV. > >>>>>>>> > >>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a > >>>>>>>> better explanation on the commit message in any case. Then I would > >>>>>>>> like to have an ack from some drm-misc maintainers before applying it > >>>>>>>> because it fixes a patch that was sent via the msm tree thus per the > >>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > >>>>>>> > >>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? > >>>>>> > >>>>>> So let me resume the situation: > >>>>>> > >>>>>> - pre_enable_prev_first was introduced in [1] > >>>>>> - some panels made use of pre_enable_prev_first > >>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > >>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > >>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next > >>>>> > >>>>> Hi Neil and Maxime, > >>>>> > >>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > >>>>> > >>>>> However, I'm not too keen on simply reverting that patch because > >>>>> > >>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and > >>>> > >>>> I never objected that, it's the right path to go. > >>>> > >>> > >>> Ack. > >>> > >>>>> > >>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. > >>>> > >>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding > >>>> a proper atomic panel API to handle a "negociation" with the host controller. > >>>> > >>> > >>> May I know what type of negotiation is needed here? > >>> > >>>>> > >>>>> > >>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? > >>>> > >>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > >>>> > >>> > >>> I wanted to respond to this earlier but didnt get a chance. > >>> > >>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). > >>> > >>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. > >>> > >>> 195 * The previous controller should be prepared first, before the prepare > >>> 196 * for the panel is called. This is largely required for DSI panels > >>> 197 * where the DSI host controller should be initialised to LP-11 before > >>> 198 * the panel is powered up. > >>> 199 */ > >>> 200 bool prepare_prev_first; > >>> > >>> These are conceptually different and thats what I explained Dmitry in our call. > >>> > >>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. > >>> > >>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). > >>> > >>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. > >>> > >>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. > >>> > >>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. > >>> > >>>>> > >>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. > >>>> > >>>> A proper migration should be done, yes, but not as a fix on top of v6.5. > >>>> > >>> > >>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. > >>> > >>> Let me know if this works as a migration plan. > >> > >> Yep agreed, let's do this > >> > >> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. > > > > This will break all non-DSI panels, which might depend on the current > > bridge calling order. > > > > I started looking at the explicit DSI power up sequencing, but it will > > take a few more days to mature. > > > > May I know why this would break all non-DSI panels? Existing panel drivers might be depending on the init order. Do we know for sure that none of DPI panels will be broken if there is a video stream ongoing during the reset procedure? Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. > > Like I said, we dont know the full details of the parade issue but I do > not see any reason why powering on a bridge chip with the DSI lanes > being in proper LP11 state should cause an issue. Its a well defined and > documented state in DSI spec. > > On the contrary, trying to turn on a bridge chip before powering on a > controller could have more issues as we do not know what state the lanes > are in when the MIPI devices (panel or bridge) are powered up. > > This sets the expectation and handshake straight.
On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: > On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: >>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: >>>> >>>> On 28/08/2023 19:07, Abhinav Kumar wrote: >>>>> Hi Neil >>>>> >>>>> Sorry I didnt respond earlier on this thread. >>>>> >>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: >>>>>> Hi Jessica, >>>>>> >>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: >>>>>>> >>>>>>> >>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>>>>>> Hi Maxime, >>>>>>>> >>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>>>>>> controllers. >>>>>>>>>>> >>>>>>>>>>> I think there is some misunderstanding there, supported by the >>>>>>>>>>> description of the flag. >>>>>>>>>>> >>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>>>>>> >>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>>>>>> their prepare() / pre_enable() callbacks. >>>>>>>>>>> >>>>>>>>>>> With the prev_first flags being introduced, we have established that >>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>>>>>> callback. >>>>>>>>>>> >>>>>>>>>>> So far so good. >>>>>>>>>> >>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>>>>>> drivers and make this would actually work first before adding the >>>>>>>>>> prev_first flag to all the required panels. >>>>>>>>> >>>>>>>>> This is definitely what we should do in an ideal world, but at least for >>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>>>>>> for it and the driver provided doesn't allow this to happen. >>>>>>>>> >>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>>>>>> pointing out that this will be something that we will have to take into >>>>>>>>> account. And it's possible that other drivers are in a similar >>>>>>>>> situation. >>>>>>>>> >>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>>>>>> flag to drm_panel"). >>>>>>>>>> >>>>>>>>>> This kind of migration should be done *before* any possible >>>>>>>>>> regression, not the other way round. >>>>>>>>>> >>>>>>>>>> If all panels sending commands from prepare() should have the >>>>>>>>>> prepare_prev_first flag, then it should be first, check for >>>>>>>>>> regressions then continue. >>>>>>>>>> >>>>>>>>>> <snip> >>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>>>>>> >>>>>>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>>>>>> already have a story without an observable end - >>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>>>>>> >>>>>>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>>>>>> >>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>>>>>> and restarted when needed, I don't understand why we can't do this >>>>>>>>>> here aswell. >>>>>>>>>> >>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>>>>>> >>>>>>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>>>>>> migration, then migrate, >>>>>>>>>> >>>>>>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>>>>>> support it from both bridge and panel PoV. >>>>>>>>>> >>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>>>>>> better explanation on the commit message in any case. Then I would >>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>>>>>> >>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>>>>>> >>>>>>>> So let me resume the situation: >>>>>>>> >>>>>>>> - pre_enable_prev_first was introduced in [1] >>>>>>>> - some panels made use of pre_enable_prev_first >>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>>>>>> >>>>>>> Hi Neil and Maxime, >>>>>>> >>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>>>>>> >>>>>>> However, I'm not too keen on simply reverting that patch because >>>>>>> >>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >>>>>> >>>>>> I never objected that, it's the right path to go. >>>>>> >>>>> >>>>> Ack. >>>>> >>>>>>> >>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >>>>>> >>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >>>>>> a proper atomic panel API to handle a "negociation" with the host controller. >>>>>> >>>>> >>>>> May I know what type of negotiation is needed here? >>>>> >>>>>>> >>>>>>> >>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >>>>>> >>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. >>>>>> >>>>> >>>>> I wanted to respond to this earlier but didnt get a chance. >>>>> >>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). >>>>> >>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. >>>>> >>>>> 195 * The previous controller should be prepared first, before the prepare >>>>> 196 * for the panel is called. This is largely required for DSI panels >>>>> 197 * where the DSI host controller should be initialised to LP-11 before >>>>> 198 * the panel is powered up. >>>>> 199 */ >>>>> 200 bool prepare_prev_first; >>>>> >>>>> These are conceptually different and thats what I explained Dmitry in our call. >>>>> >>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. >>>>> >>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). >>>>> >>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. >>>>> >>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. >>>>> >>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. >>>>> >>>>>>> >>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >>>>>> >>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. >>>>>> >>>>> >>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. >>>>> >>>>> Let me know if this works as a migration plan. >>>> >>>> Yep agreed, let's do this >>>> >>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. >>> >>> This will break all non-DSI panels, which might depend on the current >>> bridge calling order. >>> >>> I started looking at the explicit DSI power up sequencing, but it will >>> take a few more days to mature. >>> >> >> May I know why this would break all non-DSI panels? > > Existing panel drivers might be depending on the init order. Do we > know for sure that none of DPI panels will be broken if there is a > video stream ongoing during the reset procedure? > Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. > Can you please explain why would video stream be ON in pre_enable()? Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the timing engine is not enabled until the encoder's enable and the first commit after that so video stream wont be sent till then. drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. drm_atomic_bridge_chain_enable() is the one which is called after the encoder's enable(). >> >> Like I said, we dont know the full details of the parade issue but I do >> not see any reason why powering on a bridge chip with the DSI lanes >> being in proper LP11 state should cause an issue. Its a well defined and >> documented state in DSI spec. >> >> On the contrary, trying to turn on a bridge chip before powering on a >> controller could have more issues as we do not know what state the lanes >> are in when the MIPI devices (panel or bridge) are powered up. >> >> This sets the expectation and handshake straight. > >
On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: > > On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: > >>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: > >>>> > >>>> On 28/08/2023 19:07, Abhinav Kumar wrote: > >>>>> Hi Neil > >>>>> > >>>>> Sorry I didnt respond earlier on this thread. > >>>>> > >>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: > >>>>>> Hi Jessica, > >>>>>> > >>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > >>>>>>>> Hi Maxime, > >>>>>>>> > >>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > >>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>>>>>>>>>>> Sending HS commands will always work on any controller, it's all > >>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS > >>>>>>>>>>>> commands so they can use prepare_prev_first and work on any > >>>>>>>>>>>> controllers. > >>>>>>>>>>> > >>>>>>>>>>> I think there is some misunderstanding there, supported by the > >>>>>>>>>>> description of the flag. > >>>>>>>>>>> > >>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS > >>>>>>>>>>> commands after enabling video stream and switching to HS mode, see > >>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup > >>>>>>>>>>> commands in drm_panel_funcs::prepare() / > >>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention > >>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. > >>>>>>>>>>> > >>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in > >>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the > >>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from > >>>>>>>>>>> their prepare() / pre_enable() callbacks. > >>>>>>>>>>> > >>>>>>>>>>> With the prev_first flags being introduced, we have established that > >>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and > >>>>>>>>>>> switched to HS mode (be it command or video) in the enable() > >>>>>>>>>>> callback. > >>>>>>>>>>> > >>>>>>>>>>> So far so good. > >>>>>>>>>> > >>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host > >>>>>>>>>> drivers and make this would actually work first before adding the > >>>>>>>>>> prev_first flag to all the required panels. > >>>>>>>>> > >>>>>>>>> This is definitely what we should do in an ideal world, but at least for > >>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation > >>>>>>>>> for it and the driver provided doesn't allow this to happen. > >>>>>>>>> > >>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply > >>>>>>>>> pointing out that this will be something that we will have to take into > >>>>>>>>> account. And it's possible that other drivers are in a similar > >>>>>>>>> situation. > >>>>>>>>> > >>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This > >>>>>>>>>>> requires that all DSI panels sending commands from prepare() should > >>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches > >>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >>>>>>>>>>> flag to drm_panel"). > >>>>>>>>>> > >>>>>>>>>> This kind of migration should be done *before* any possible > >>>>>>>>>> regression, not the other way round. > >>>>>>>>>> > >>>>>>>>>> If all panels sending commands from prepare() should have the > >>>>>>>>>> prepare_prev_first flag, then it should be first, check for > >>>>>>>>>> regressions then continue. > >>>>>>>>>> > >>>>>>>>>> <snip> > >>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for > >>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for > >>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it > >>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. > >>>>>>>>>>> > >>>>>>>>>>> If we go this way, we can never reapply these patches. There will be > >>>>>>>>>>> no guarantee that all panel drivers are completely converted. We > >>>>>>>>>>> already have a story without an observable end - > >>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >>>>>>>>>> > >>>>>>>>>> I don't understand this point, who would block re-applying the patches ? > >>>>>>>>>> > >>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > >>>>>>>>>> Linux version and went smoothly because we reverted regressing patches > >>>>>>>>>> and restarted when needed, I don't understand why we can't do this > >>>>>>>>>> here aswell. > >>>>>>>>>> > >>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the > >>>>>>>>>>> panel drivers that require fixes patches. If you care about the > >>>>>>>>>>> particular Fixes tag, I have provided one several lines above. > >>>>>>>>>> > >>>>>>>>>> Unfortunately it should be done in the other way round, prepare for > >>>>>>>>>> migration, then migrate, > >>>>>>>>>> > >>>>>>>>>> I mean if it's a required migration, then it should be done and I'll > >>>>>>>>>> support it from both bridge and panel PoV. > >>>>>>>>>> > >>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a > >>>>>>>>>> better explanation on the commit message in any case. Then I would > >>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it > >>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the > >>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > >>>>>>>>> > >>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? > >>>>>>>> > >>>>>>>> So let me resume the situation: > >>>>>>>> > >>>>>>>> - pre_enable_prev_first was introduced in [1] > >>>>>>>> - some panels made use of pre_enable_prev_first > >>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > >>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > >>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next > >>>>>>> > >>>>>>> Hi Neil and Maxime, > >>>>>>> > >>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > >>>>>>> > >>>>>>> However, I'm not too keen on simply reverting that patch because > >>>>>>> > >>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and > >>>>>> > >>>>>> I never objected that, it's the right path to go. > >>>>>> > >>>>> > >>>>> Ack. > >>>>> > >>>>>>> > >>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. > >>>>>> > >>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding > >>>>>> a proper atomic panel API to handle a "negociation" with the host controller. > >>>>>> > >>>>> > >>>>> May I know what type of negotiation is needed here? > >>>>> > >>>>>>> > >>>>>>> > >>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? > >>>>>> > >>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > >>>>>> > >>>>> > >>>>> I wanted to respond to this earlier but didnt get a chance. > >>>>> > >>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). > >>>>> > >>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. > >>>>> > >>>>> 195 * The previous controller should be prepared first, before the prepare > >>>>> 196 * for the panel is called. This is largely required for DSI panels > >>>>> 197 * where the DSI host controller should be initialised to LP-11 before > >>>>> 198 * the panel is powered up. > >>>>> 199 */ > >>>>> 200 bool prepare_prev_first; > >>>>> > >>>>> These are conceptually different and thats what I explained Dmitry in our call. > >>>>> > >>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. > >>>>> > >>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). > >>>>> > >>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. > >>>>> > >>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. > >>>>> > >>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. > >>>>> > >>>>>>> > >>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. > >>>>>> > >>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. > >>>>>> > >>>>> > >>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. > >>>>> > >>>>> Let me know if this works as a migration plan. > >>>> > >>>> Yep agreed, let's do this > >>>> > >>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. > >>> > >>> This will break all non-DSI panels, which might depend on the current > >>> bridge calling order. > >>> > >>> I started looking at the explicit DSI power up sequencing, but it will > >>> take a few more days to mature. > >>> > >> > >> May I know why this would break all non-DSI panels? > > > > Existing panel drivers might be depending on the init order. Do we > > know for sure that none of DPI panels will be broken if there is a > > video stream ongoing during the reset procedure? > > Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. > > > > Can you please explain why would video stream be ON in pre_enable()? > > Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the > timing engine is not enabled until the encoder's enable and the first > commit after that so video stream wont be sent till then. You are describing the MSM DSI case. I was pointing to the fact that parent's pre_enable if called too early might conflict with the next bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a full-featured video stream, this state might confuse the panel. So we can not blindly switch the order of pre_enable callbacks for the bridge-panel pair. > > drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. > > drm_atomic_bridge_chain_enable() is the one which is called after the > encoder's enable(). > > >> > >> Like I said, we dont know the full details of the parade issue but I do > >> not see any reason why powering on a bridge chip with the DSI lanes > >> being in proper LP11 state should cause an issue. Its a well defined and > >> documented state in DSI spec. > >> > >> On the contrary, trying to turn on a bridge chip before powering on a > >> controller could have more issues as we do not know what state the lanes > >> are in when the MIPI devices (panel or bridge) are powered up. > >> > >> This sets the expectation and handshake straight. > > > >
On 8/29/2023 11:51 AM, Dmitry Baryshkov wrote: > On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: >>> On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: >>>>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: >>>>>> >>>>>> On 28/08/2023 19:07, Abhinav Kumar wrote: >>>>>>> Hi Neil >>>>>>> >>>>>>> Sorry I didnt respond earlier on this thread. >>>>>>> >>>>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: >>>>>>>> Hi Jessica, >>>>>>>> >>>>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>>>>>>>> Hi Maxime, >>>>>>>>>> >>>>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>>>>>>>> controllers. >>>>>>>>>>>>> >>>>>>>>>>>>> I think there is some misunderstanding there, supported by the >>>>>>>>>>>>> description of the flag. >>>>>>>>>>>>> >>>>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>>>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>>>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>>>>>>>> >>>>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>>>>>>>> their prepare() / pre_enable() callbacks. >>>>>>>>>>>>> >>>>>>>>>>>>> With the prev_first flags being introduced, we have established that >>>>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>>>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>>>>>>>> callback. >>>>>>>>>>>>> >>>>>>>>>>>>> So far so good. >>>>>>>>>>>> >>>>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>>>>>>>> drivers and make this would actually work first before adding the >>>>>>>>>>>> prev_first flag to all the required panels. >>>>>>>>>>> >>>>>>>>>>> This is definitely what we should do in an ideal world, but at least for >>>>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>>>>>>>> for it and the driver provided doesn't allow this to happen. >>>>>>>>>>> >>>>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>>>>>>>> pointing out that this will be something that we will have to take into >>>>>>>>>>> account. And it's possible that other drivers are in a similar >>>>>>>>>>> situation. >>>>>>>>>>> >>>>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>>>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>>>>>>>> flag to drm_panel"). >>>>>>>>>>>> >>>>>>>>>>>> This kind of migration should be done *before* any possible >>>>>>>>>>>> regression, not the other way round. >>>>>>>>>>>> >>>>>>>>>>>> If all panels sending commands from prepare() should have the >>>>>>>>>>>> prepare_prev_first flag, then it should be first, check for >>>>>>>>>>>> regressions then continue. >>>>>>>>>>>> >>>>>>>>>>>> <snip> >>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>>>>>>>> >>>>>>>>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>>>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>>>>>>>> already have a story without an observable end - >>>>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>>>>>>>> >>>>>>>>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>>>>>>>> >>>>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>>>>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>>>>>>>> and restarted when needed, I don't understand why we can't do this >>>>>>>>>>>> here aswell. >>>>>>>>>>>> >>>>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>>>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>>>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>>>>>>>> >>>>>>>>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>>>>>>>> migration, then migrate, >>>>>>>>>>>> >>>>>>>>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>>>>>>>> support it from both bridge and panel PoV. >>>>>>>>>>>> >>>>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>>>>>>>> better explanation on the commit message in any case. Then I would >>>>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>>>>>>>> >>>>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>>>>>>>> >>>>>>>>>> So let me resume the situation: >>>>>>>>>> >>>>>>>>>> - pre_enable_prev_first was introduced in [1] >>>>>>>>>> - some panels made use of pre_enable_prev_first >>>>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>>>>>>>> >>>>>>>>> Hi Neil and Maxime, >>>>>>>>> >>>>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>>>>>>>> >>>>>>>>> However, I'm not too keen on simply reverting that patch because >>>>>>>>> >>>>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >>>>>>>> >>>>>>>> I never objected that, it's the right path to go. >>>>>>>> >>>>>>> >>>>>>> Ack. >>>>>>> >>>>>>>>> >>>>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >>>>>>>> >>>>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >>>>>>>> a proper atomic panel API to handle a "negociation" with the host controller. >>>>>>>> >>>>>>> >>>>>>> May I know what type of negotiation is needed here? >>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >>>>>>>> >>>>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. >>>>>>>> >>>>>>> >>>>>>> I wanted to respond to this earlier but didnt get a chance. >>>>>>> >>>>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). >>>>>>> >>>>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. >>>>>>> >>>>>>> 195 * The previous controller should be prepared first, before the prepare >>>>>>> 196 * for the panel is called. This is largely required for DSI panels >>>>>>> 197 * where the DSI host controller should be initialised to LP-11 before >>>>>>> 198 * the panel is powered up. >>>>>>> 199 */ >>>>>>> 200 bool prepare_prev_first; >>>>>>> >>>>>>> These are conceptually different and thats what I explained Dmitry in our call. >>>>>>> >>>>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. >>>>>>> >>>>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). >>>>>>> >>>>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. >>>>>>> >>>>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. >>>>>>> >>>>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. >>>>>>> >>>>>>>>> >>>>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >>>>>>>> >>>>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. >>>>>>>> >>>>>>> >>>>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. >>>>>>> >>>>>>> Let me know if this works as a migration plan. >>>>>> >>>>>> Yep agreed, let's do this >>>>>> >>>>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. >>>>> >>>>> This will break all non-DSI panels, which might depend on the current >>>>> bridge calling order. >>>>> >>>>> I started looking at the explicit DSI power up sequencing, but it will >>>>> take a few more days to mature. >>>>> >>>> >>>> May I know why this would break all non-DSI panels? >>> >>> Existing panel drivers might be depending on the init order. Do we >>> know for sure that none of DPI panels will be broken if there is a >>> video stream ongoing during the reset procedure? >>> Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. >>> >> >> Can you please explain why would video stream be ON in pre_enable()? >> >> Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the >> timing engine is not enabled until the encoder's enable and the first >> commit after that so video stream wont be sent till then. > > You are describing the MSM DSI case. I was pointing to the fact that > parent's pre_enable if called too early might conflict with the next > bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a > full-featured video stream, this state might confuse the panel. So we > can not blindly switch the order of pre_enable callbacks for the > bridge-panel pair. > Even if the end connector was a eDP or DPI, the input to the bridge was DSI so I think its unlikely that video stream was on before encoder's enable but I cannot speak for all these interfaces/vendors. So, to accommodate both worlds, does this work? diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 9316384b4474..2b38388d4e56 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -416,7 +416,10 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, return bridge; } - bridge->pre_enable_prev_first = panel->prepare_prev_first; + if (connector_type == DRM_MODE_CONNECTOR_DSI) + bridge->pre_enable_prev_first = true; + else + bridge->pre_enable_prev_first = panel->prepare_prev_first; *ptr = bridge; devres_add(dev, ptr); >> >> drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. >> >> drm_atomic_bridge_chain_enable() is the one which is called after the >> encoder's enable(). >> >>>> >>>> Like I said, we dont know the full details of the parade issue but I do >>>> not see any reason why powering on a bridge chip with the DSI lanes >>>> being in proper LP11 state should cause an issue. Its a well defined and >>>> documented state in DSI spec. >>>> >>>> On the contrary, trying to turn on a bridge chip before powering on a >>>> controller could have more issues as we do not know what state the lanes >>>> are in when the MIPI devices (panel or bridge) are powered up. >>>> >>>> This sets the expectation and handshake straight. >>> >>> > > >
On Tue, 29 Aug 2023 at 22:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 8/29/2023 11:51 AM, Dmitry Baryshkov wrote: > > On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: > >>> On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: > >>>>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: > >>>>>> > >>>>>> On 28/08/2023 19:07, Abhinav Kumar wrote: > >>>>>>> Hi Neil > >>>>>>> > >>>>>>> Sorry I didnt respond earlier on this thread. > >>>>>>> > >>>>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: > >>>>>>>> Hi Jessica, > >>>>>>>> > >>>>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > >>>>>>>>>> Hi Maxime, > >>>>>>>>>> > >>>>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: > >>>>>>>>>>> Hi, > >>>>>>>>>>> > >>>>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > >>>>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >>>>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>>>>>>>>>>>>> Sending HS commands will always work on any controller, it's all > >>>>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS > >>>>>>>>>>>>>> commands so they can use prepare_prev_first and work on any > >>>>>>>>>>>>>> controllers. > >>>>>>>>>>>>> > >>>>>>>>>>>>> I think there is some misunderstanding there, supported by the > >>>>>>>>>>>>> description of the flag. > >>>>>>>>>>>>> > >>>>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS > >>>>>>>>>>>>> commands after enabling video stream and switching to HS mode, see > >>>>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup > >>>>>>>>>>>>> commands in drm_panel_funcs::prepare() / > >>>>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention > >>>>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in > >>>>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the > >>>>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from > >>>>>>>>>>>>> their prepare() / pre_enable() callbacks. > >>>>>>>>>>>>> > >>>>>>>>>>>>> With the prev_first flags being introduced, we have established that > >>>>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and > >>>>>>>>>>>>> switched to HS mode (be it command or video) in the enable() > >>>>>>>>>>>>> callback. > >>>>>>>>>>>>> > >>>>>>>>>>>>> So far so good. > >>>>>>>>>>>> > >>>>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host > >>>>>>>>>>>> drivers and make this would actually work first before adding the > >>>>>>>>>>>> prev_first flag to all the required panels. > >>>>>>>>>>> > >>>>>>>>>>> This is definitely what we should do in an ideal world, but at least for > >>>>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation > >>>>>>>>>>> for it and the driver provided doesn't allow this to happen. > >>>>>>>>>>> > >>>>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply > >>>>>>>>>>> pointing out that this will be something that we will have to take into > >>>>>>>>>>> account. And it's possible that other drivers are in a similar > >>>>>>>>>>> situation. > >>>>>>>>>>> > >>>>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This > >>>>>>>>>>>>> requires that all DSI panels sending commands from prepare() should > >>>>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches > >>>>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >>>>>>>>>>>>> flag to drm_panel"). > >>>>>>>>>>>> > >>>>>>>>>>>> This kind of migration should be done *before* any possible > >>>>>>>>>>>> regression, not the other way round. > >>>>>>>>>>>> > >>>>>>>>>>>> If all panels sending commands from prepare() should have the > >>>>>>>>>>>> prepare_prev_first flag, then it should be first, check for > >>>>>>>>>>>> regressions then continue. > >>>>>>>>>>>> > >>>>>>>>>>>> <snip> > >>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for > >>>>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for > >>>>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it > >>>>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. > >>>>>>>>>>>>> > >>>>>>>>>>>>> If we go this way, we can never reapply these patches. There will be > >>>>>>>>>>>>> no guarantee that all panel drivers are completely converted. We > >>>>>>>>>>>>> already have a story without an observable end - > >>>>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >>>>>>>>>>>> > >>>>>>>>>>>> I don't understand this point, who would block re-applying the patches ? > >>>>>>>>>>>> > >>>>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > >>>>>>>>>>>> Linux version and went smoothly because we reverted regressing patches > >>>>>>>>>>>> and restarted when needed, I don't understand why we can't do this > >>>>>>>>>>>> here aswell. > >>>>>>>>>>>> > >>>>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the > >>>>>>>>>>>>> panel drivers that require fixes patches. If you care about the > >>>>>>>>>>>>> particular Fixes tag, I have provided one several lines above. > >>>>>>>>>>>> > >>>>>>>>>>>> Unfortunately it should be done in the other way round, prepare for > >>>>>>>>>>>> migration, then migrate, > >>>>>>>>>>>> > >>>>>>>>>>>> I mean if it's a required migration, then it should be done and I'll > >>>>>>>>>>>> support it from both bridge and panel PoV. > >>>>>>>>>>>> > >>>>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a > >>>>>>>>>>>> better explanation on the commit message in any case. Then I would > >>>>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it > >>>>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the > >>>>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > >>>>>>>>>>> > >>>>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? > >>>>>>>>>> > >>>>>>>>>> So let me resume the situation: > >>>>>>>>>> > >>>>>>>>>> - pre_enable_prev_first was introduced in [1] > >>>>>>>>>> - some panels made use of pre_enable_prev_first > >>>>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > >>>>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > >>>>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next > >>>>>>>>> > >>>>>>>>> Hi Neil and Maxime, > >>>>>>>>> > >>>>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > >>>>>>>>> > >>>>>>>>> However, I'm not too keen on simply reverting that patch because > >>>>>>>>> > >>>>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and > >>>>>>>> > >>>>>>>> I never objected that, it's the right path to go. > >>>>>>>> > >>>>>>> > >>>>>>> Ack. > >>>>>>> > >>>>>>>>> > >>>>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. > >>>>>>>> > >>>>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding > >>>>>>>> a proper atomic panel API to handle a "negociation" with the host controller. > >>>>>>>> > >>>>>>> > >>>>>>> May I know what type of negotiation is needed here? > >>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? > >>>>>>>> > >>>>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > >>>>>>>> > >>>>>>> > >>>>>>> I wanted to respond to this earlier but didnt get a chance. > >>>>>>> > >>>>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). > >>>>>>> > >>>>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. > >>>>>>> > >>>>>>> 195 * The previous controller should be prepared first, before the prepare > >>>>>>> 196 * for the panel is called. This is largely required for DSI panels > >>>>>>> 197 * where the DSI host controller should be initialised to LP-11 before > >>>>>>> 198 * the panel is powered up. > >>>>>>> 199 */ > >>>>>>> 200 bool prepare_prev_first; > >>>>>>> > >>>>>>> These are conceptually different and thats what I explained Dmitry in our call. > >>>>>>> > >>>>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. > >>>>>>> > >>>>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). > >>>>>>> > >>>>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. > >>>>>>> > >>>>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. > >>>>>>> > >>>>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. > >>>>>>> > >>>>>>>>> > >>>>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. > >>>>>>>> > >>>>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. > >>>>>>>> > >>>>>>> > >>>>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. > >>>>>>> > >>>>>>> Let me know if this works as a migration plan. > >>>>>> > >>>>>> Yep agreed, let's do this > >>>>>> > >>>>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. > >>>>> > >>>>> This will break all non-DSI panels, which might depend on the current > >>>>> bridge calling order. > >>>>> > >>>>> I started looking at the explicit DSI power up sequencing, but it will > >>>>> take a few more days to mature. > >>>>> > >>>> > >>>> May I know why this would break all non-DSI panels? > >>> > >>> Existing panel drivers might be depending on the init order. Do we > >>> know for sure that none of DPI panels will be broken if there is a > >>> video stream ongoing during the reset procedure? > >>> Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. > >>> > >> > >> Can you please explain why would video stream be ON in pre_enable()? > >> > >> Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the > >> timing engine is not enabled until the encoder's enable and the first > >> commit after that so video stream wont be sent till then. > > > > You are describing the MSM DSI case. I was pointing to the fact that > > parent's pre_enable if called too early might conflict with the next > > bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a > > full-featured video stream, this state might confuse the panel. So we > > can not blindly switch the order of pre_enable callbacks for the > > bridge-panel pair. > > > > Even if the end connector was a eDP or DPI, the input to the bridge was > DSI so I think its unlikely that video stream was on before encoder's > enable but I cannot speak for all these interfaces/vendors. > > So, to accommodate both worlds, does this work? > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 9316384b4474..2b38388d4e56 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -416,7 +416,10 @@ struct drm_bridge > *devm_drm_panel_bridge_add_typed(struct device *dev, > return bridge; > } > > - bridge->pre_enable_prev_first = panel->prepare_prev_first; > + if (connector_type == DRM_MODE_CONNECTOR_DSI) > + bridge->pre_enable_prev_first = true; > + else > + bridge->pre_enable_prev_first = panel->prepare_prev_first; looks like a hack. > > *ptr = bridge; > devres_add(dev, ptr); > > >> > >> drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. > >> > >> drm_atomic_bridge_chain_enable() is the one which is called after the > >> encoder's enable(). > >> > >>>> > >>>> Like I said, we dont know the full details of the parade issue but I do > >>>> not see any reason why powering on a bridge chip with the DSI lanes > >>>> being in proper LP11 state should cause an issue. Its a well defined and > >>>> documented state in DSI spec. > >>>> > >>>> On the contrary, trying to turn on a bridge chip before powering on a > >>>> controller could have more issues as we do not know what state the lanes > >>>> are in when the MIPI devices (panel or bridge) are powered up. > >>>> > >>>> This sets the expectation and handshake straight. > >>> > >>> > > > > > >
On 8/29/2023 12:15 PM, Dmitry Baryshkov wrote: > On Tue, 29 Aug 2023 at 22:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 8/29/2023 11:51 AM, Dmitry Baryshkov wrote: >>> On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: >>>>> On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: >>>>>>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: >>>>>>>> >>>>>>>> On 28/08/2023 19:07, Abhinav Kumar wrote: >>>>>>>>> Hi Neil >>>>>>>>> >>>>>>>>> Sorry I didnt respond earlier on this thread. >>>>>>>>> >>>>>>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: >>>>>>>>>> Hi Jessica, >>>>>>>>>> >>>>>>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>>>>>>>>>> Hi Maxime, >>>>>>>>>>>> >>>>>>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>>>>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>>>>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>>>>>>>>>> controllers. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I think there is some misunderstanding there, supported by the >>>>>>>>>>>>>>> description of the flag. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>>>>>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>>>>>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>>>>>>>>>> their prepare() / pre_enable() callbacks. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> With the prev_first flags being introduced, we have established that >>>>>>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>>>>>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>>>>>>>>>> callback. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> So far so good. >>>>>>>>>>>>>> >>>>>>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>>>>>>>>>> drivers and make this would actually work first before adding the >>>>>>>>>>>>>> prev_first flag to all the required panels. >>>>>>>>>>>>> >>>>>>>>>>>>> This is definitely what we should do in an ideal world, but at least for >>>>>>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>>>>>>>>>> for it and the driver provided doesn't allow this to happen. >>>>>>>>>>>>> >>>>>>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>>>>>>>>>> pointing out that this will be something that we will have to take into >>>>>>>>>>>>> account. And it's possible that other drivers are in a similar >>>>>>>>>>>>> situation. >>>>>>>>>>>>> >>>>>>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>>>>>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>>>>>>>>>> flag to drm_panel"). >>>>>>>>>>>>>> >>>>>>>>>>>>>> This kind of migration should be done *before* any possible >>>>>>>>>>>>>> regression, not the other way round. >>>>>>>>>>>>>> >>>>>>>>>>>>>> If all panels sending commands from prepare() should have the >>>>>>>>>>>>>> prepare_prev_first flag, then it should be first, check for >>>>>>>>>>>>>> regressions then continue. >>>>>>>>>>>>>> >>>>>>>>>>>>>> <snip> >>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>>>>>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>>>>>>>>>> already have a story without an observable end - >>>>>>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>>>>>>>>>> >>>>>>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>>>>>>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>>>>>>>>>> and restarted when needed, I don't understand why we can't do this >>>>>>>>>>>>>> here aswell. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>>>>>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>>>>>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>>>>>>>>>> migration, then migrate, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>>>>>>>>>> support it from both bridge and panel PoV. >>>>>>>>>>>>>> >>>>>>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>>>>>>>>>> better explanation on the commit message in any case. Then I would >>>>>>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>>>>>>>>>> >>>>>>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>>>>>>>>>> >>>>>>>>>>>> So let me resume the situation: >>>>>>>>>>>> >>>>>>>>>>>> - pre_enable_prev_first was introduced in [1] >>>>>>>>>>>> - some panels made use of pre_enable_prev_first >>>>>>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>>>>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>>>>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>>>>>>>>>> >>>>>>>>>>> Hi Neil and Maxime, >>>>>>>>>>> >>>>>>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>>>>>>>>>> >>>>>>>>>>> However, I'm not too keen on simply reverting that patch because >>>>>>>>>>> >>>>>>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >>>>>>>>>> >>>>>>>>>> I never objected that, it's the right path to go. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Ack. >>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >>>>>>>>>> >>>>>>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >>>>>>>>>> a proper atomic panel API to handle a "negociation" with the host controller. >>>>>>>>>> >>>>>>>>> >>>>>>>>> May I know what type of negotiation is needed here? >>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >>>>>>>>>> >>>>>>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I wanted to respond to this earlier but didnt get a chance. >>>>>>>>> >>>>>>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). >>>>>>>>> >>>>>>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. >>>>>>>>> >>>>>>>>> 195 * The previous controller should be prepared first, before the prepare >>>>>>>>> 196 * for the panel is called. This is largely required for DSI panels >>>>>>>>> 197 * where the DSI host controller should be initialised to LP-11 before >>>>>>>>> 198 * the panel is powered up. >>>>>>>>> 199 */ >>>>>>>>> 200 bool prepare_prev_first; >>>>>>>>> >>>>>>>>> These are conceptually different and thats what I explained Dmitry in our call. >>>>>>>>> >>>>>>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. >>>>>>>>> >>>>>>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). >>>>>>>>> >>>>>>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. >>>>>>>>> >>>>>>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. >>>>>>>>> >>>>>>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. >>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >>>>>>>>>> >>>>>>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. >>>>>>>>> >>>>>>>>> Let me know if this works as a migration plan. >>>>>>>> >>>>>>>> Yep agreed, let's do this >>>>>>>> >>>>>>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. >>>>>>> >>>>>>> This will break all non-DSI panels, which might depend on the current >>>>>>> bridge calling order. >>>>>>> >>>>>>> I started looking at the explicit DSI power up sequencing, but it will >>>>>>> take a few more days to mature. >>>>>>> >>>>>> >>>>>> May I know why this would break all non-DSI panels? >>>>> >>>>> Existing panel drivers might be depending on the init order. Do we >>>>> know for sure that none of DPI panels will be broken if there is a >>>>> video stream ongoing during the reset procedure? >>>>> Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. >>>>> >>>> >>>> Can you please explain why would video stream be ON in pre_enable()? >>>> >>>> Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the >>>> timing engine is not enabled until the encoder's enable and the first >>>> commit after that so video stream wont be sent till then. >>> >>> You are describing the MSM DSI case. I was pointing to the fact that >>> parent's pre_enable if called too early might conflict with the next >>> bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a >>> full-featured video stream, this state might confuse the panel. So we >>> can not blindly switch the order of pre_enable callbacks for the >>> bridge-panel pair. >>> >> >> Even if the end connector was a eDP or DPI, the input to the bridge was >> DSI so I think its unlikely that video stream was on before encoder's >> enable but I cannot speak for all these interfaces/vendors. >> >> So, to accommodate both worlds, does this work? >> >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c >> index 9316384b4474..2b38388d4e56 100644 >> --- a/drivers/gpu/drm/bridge/panel.c >> +++ b/drivers/gpu/drm/bridge/panel.c >> @@ -416,7 +416,10 @@ struct drm_bridge >> *devm_drm_panel_bridge_add_typed(struct device *dev, >> return bridge; >> } >> >> - bridge->pre_enable_prev_first = panel->prepare_prev_first; >> + if (connector_type == DRM_MODE_CONNECTOR_DSI) >> + bridge->pre_enable_prev_first = true; >> + else >> + bridge->pre_enable_prev_first = panel->prepare_prev_first; > > looks like a hack. > Nope its not, DSI spec has clear mentions about LP11 state to be used during initialization. "After power-up, the host processor shall observe an initialization period, TINIT, during which it shall drive a sustained TX-Stop state (LP-11) on all Lanes of the Link. See [MIPI04] for descriptions of TINIT and the TX-Stop state. Peripherals shall power up in the RX-Stop state and monitor the Link to determine if the host processor has asserted a TX-Stop state for at least the TINIT period. The peripheral shall ignore all Link states prior to detection of a TINIT event. The peripheral shall be ready to accept bus transactions immediately following the end of the TINIT period." I am only extending this statement by mandating that the DSI PHY / controller be powered up so that its able to drive the lanes to LP11 state. Now, pls explain why this is a hack >> >> *ptr = bridge; >> devres_add(dev, ptr); >> >>>> >>>> drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. >>>> >>>> drm_atomic_bridge_chain_enable() is the one which is called after the >>>> encoder's enable(). >>>> >>>>>> >>>>>> Like I said, we dont know the full details of the parade issue but I do >>>>>> not see any reason why powering on a bridge chip with the DSI lanes >>>>>> being in proper LP11 state should cause an issue. Its a well defined and >>>>>> documented state in DSI spec. >>>>>> >>>>>> On the contrary, trying to turn on a bridge chip before powering on a >>>>>> controller could have more issues as we do not know what state the lanes >>>>>> are in when the MIPI devices (panel or bridge) are powered up. >>>>>> >>>>>> This sets the expectation and handshake straight. >>>>> >>>>> >>> >>> >>> > > >
Hi Dave Nice to e-meet you. On 8/29/2023 7:13 AM, Dave Stevenson wrote: > Hi Neil > > On Mon, 28 Aug 2023 at 09:49, <neil.armstrong@linaro.org> wrote: >> >> Hi Jessica, >> >> On 25/08/2023 20:37, Jessica Zhang wrote: >>> >>> >>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>> Hi Maxime, >>>> >>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>> Hi, >>>>> >>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>> controllers. >>>>>>> >>>>>>> I think there is some misunderstanding there, supported by the >>>>>>> description of the flag. >>>>>>> >>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>> >>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>> their prepare() / pre_enable() callbacks. >>>>>>> >>>>>>> With the prev_first flags being introduced, we have established that >>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>> callback. >>>>>>> >>>>>>> So far so good. >>>>>> >>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>> drivers and make this would actually work first before adding the >>>>>> prev_first flag to all the required panels. >>>>> >>>>> This is definitely what we should do in an ideal world, but at least for >>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>> for it and the driver provided doesn't allow this to happen. >>>>> >>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>> pointing out that this will be something that we will have to take into >>>>> account. And it's possible that other drivers are in a similar >>>>> situation. >>>>> >>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>> flag to drm_panel"). >>>>>> >>>>>> This kind of migration should be done *before* any possible >>>>>> regression, not the other way round. >>>>>> >>>>>> If all panels sending commands from prepare() should have the >>>>>> prepare_prev_first flag, then it should be first, check for >>>>>> regressions then continue. >>>>>> >>>>>> <snip> >>>>>> >>>>>>>> >>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>> >>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>> already have a story without an observable end - >>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>> >>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>> >>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>> and restarted when needed, I don't understand why we can't do this >>>>>> here aswell. >>>>>> >>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>> >>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>> migration, then migrate, >>>>>> >>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>> support it from both bridge and panel PoV. >>>>>> >>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>> better explanation on the commit message in any case. Then I would >>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>> >>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>> >>>> So let me resume the situation: >>>> >>>> - pre_enable_prev_first was introduced in [1] >>>> - some panels made use of pre_enable_prev_first >>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>> >>> Hi Neil and Maxime, >>> >>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>> >>> However, I'm not too keen on simply reverting that patch because >>> >>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >> >> I never objected that, it's the right path to go. >> >>> >>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >> >> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >> a proper atomic panel API to handle a "negociation" with the host controller. >> >>> >>> >>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >> >> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > > Any panel wishing the clock and data lanes to be in a defined LP-11 > state before pre_enable() is called need to set prepare_prev_first to > true. This is not a universal requirement of all DSI peripherals for > which commands are sent from pre_enable - a number will happily power > up at LP-00. > It is true that no harm will occur on those devices that do support > non-LP-11 power up if the host is in LP-11, so a blanket setting of > the flag for any panel driver sending DSI commands in pre_enable > should be safe. > > It is documented [1] that transfer can be called at any time, > regardless of the state of the host. The MSM driver isn't supporting > that, hence issues. > [2] further clarifies that it is expected to power up the host > controller, send the message, and power down again. > > [1] https://github.com/torvalds/linux/blob/master/include/drm/drm_mipi_dsi.h#L84-L87 > [2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L185-L188 > This is very good information. Its not that MSM driver isn't supporting that, there seems to be a check to fail the transfer if the host is not powered on: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dsi/dsi_host.c#L1629 That was the error we were hitting. This condition has been there since ages. I will need to investigate what happens if we relax this condition. Will check it up. That being said, the documentation seems a bit non-specific. Yes, it tells us that transfer can be called even before pre_enable but its not just about this API right. When we are powering on the peripheral, it usually has a sequence to follow. So something like goto LP11 ---> send the ON commands ---> start video stream etc The documentation is talking about a "generic" call to .transfer and that it should handle it but the ON sequence of a peripheral clearly follows a sequence which needs the host to be powered on first and we cannot immediately turn it OFF as the documentation says as that will put the lanes in LP00 or even undefined state and confuse the peripheral. Now the discussion is revolving more around how to handle the pre_enable_prev_first in the DSI ON cases. >>> >>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >> >> A proper migration should be done, yes, but not as a fix on top of v6.5. > > I looked at this when adding prepare_prev_first, but as the DSI > control path is separate from the bridge chain there's no obvious way > to automatically set a bridge flag from the mipi_dsi registration. > Since there is documentation in the DSI spec about the peripheral expecting LP11 state and we agree its no harm, would something like below be incorrect? diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 9316384b4474..2b38388d4e56 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -416,7 +416,10 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, return bridge; } - bridge->pre_enable_prev_first = panel->prepare_prev_first; + if (connector_type == DRM_MODE_CONNECTOR_DSI) + bridge->pre_enable_prev_first = true; + else + bridge->pre_enable_prev_first = panel->prepare_prev_first; *ptr = bridge; devres_add(dev, ptr); > Dave > >> Neil >> >>> >>> Thanks, >>> >>> Jessica Zhang >>> >>> >>>> >>>> I do not consider it's the right way to fix regression caused by [2] >>>> I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again >>>> >>>> I have no objection about [2] and it should be done widely over the whole DSI controllers >>>> and DSI Video panels. >>>> >>>> I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1] >>>> should be used but it's even more wrong since [2] really caused the regression. >>>> >>>> And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be >>>> applied via drm-misc-next-fixes, right ? >>>> >>>> [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") >>>> [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") >>>> >>>> Thanks, >>>> Neil >>>> >>>>> >>>>> Maxime >>>> >>
On Tue, 29 Aug 2023 at 22:21, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 8/29/2023 12:15 PM, Dmitry Baryshkov wrote: > > On Tue, 29 Aug 2023 at 22:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 8/29/2023 11:51 AM, Dmitry Baryshkov wrote: > >>> On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: > >>>>> On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: > >>>>>>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: > >>>>>>>> > >>>>>>>> On 28/08/2023 19:07, Abhinav Kumar wrote: > >>>>>>>>> Hi Neil > >>>>>>>>> > >>>>>>>>> Sorry I didnt respond earlier on this thread. > >>>>>>>>> > >>>>>>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: > >>>>>>>>>> Hi Jessica, > >>>>>>>>>> > >>>>>>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: > >>>>>>>>>>>> Hi Maxime, > >>>>>>>>>>>> > >>>>>>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: > >>>>>>>>>>>>> Hi, > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: > >>>>>>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: > >>>>>>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: > >>>>>>>>>>>>>>>> Sending HS commands will always work on any controller, it's all > >>>>>>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS > >>>>>>>>>>>>>>>> commands so they can use prepare_prev_first and work on any > >>>>>>>>>>>>>>>> controllers. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I think there is some misunderstanding there, supported by the > >>>>>>>>>>>>>>> description of the flag. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS > >>>>>>>>>>>>>>> commands after enabling video stream and switching to HS mode, see > >>>>>>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup > >>>>>>>>>>>>>>> commands in drm_panel_funcs::prepare() / > >>>>>>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention > >>>>>>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in > >>>>>>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the > >>>>>>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from > >>>>>>>>>>>>>>> their prepare() / pre_enable() callbacks. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> With the prev_first flags being introduced, we have established that > >>>>>>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and > >>>>>>>>>>>>>>> switched to HS mode (be it command or video) in the enable() > >>>>>>>>>>>>>>> callback. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> So far so good. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host > >>>>>>>>>>>>>> drivers and make this would actually work first before adding the > >>>>>>>>>>>>>> prev_first flag to all the required panels. > >>>>>>>>>>>>> > >>>>>>>>>>>>> This is definitely what we should do in an ideal world, but at least for > >>>>>>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation > >>>>>>>>>>>>> for it and the driver provided doesn't allow this to happen. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply > >>>>>>>>>>>>> pointing out that this will be something that we will have to take into > >>>>>>>>>>>>> account. And it's possible that other drivers are in a similar > >>>>>>>>>>>>> situation. > >>>>>>>>>>>>> > >>>>>>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This > >>>>>>>>>>>>>>> requires that all DSI panels sending commands from prepare() should > >>>>>>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches > >>>>>>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first > >>>>>>>>>>>>>>> flag to drm_panel"). > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> This kind of migration should be done *before* any possible > >>>>>>>>>>>>>> regression, not the other way round. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> If all panels sending commands from prepare() should have the > >>>>>>>>>>>>>> prepare_prev_first flag, then it should be first, check for > >>>>>>>>>>>>>> regressions then continue. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> <snip> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for > >>>>>>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for > >>>>>>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it > >>>>>>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> If we go this way, we can never reapply these patches. There will be > >>>>>>>>>>>>>>> no guarantee that all panel drivers are completely converted. We > >>>>>>>>>>>>>>> already have a story without an observable end - > >>>>>>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I don't understand this point, who would block re-applying the patches ? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple > >>>>>>>>>>>>>> Linux version and went smoothly because we reverted regressing patches > >>>>>>>>>>>>>> and restarted when needed, I don't understand why we can't do this > >>>>>>>>>>>>>> here aswell. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the > >>>>>>>>>>>>>>> panel drivers that require fixes patches. If you care about the > >>>>>>>>>>>>>>> particular Fixes tag, I have provided one several lines above. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Unfortunately it should be done in the other way round, prepare for > >>>>>>>>>>>>>> migration, then migrate, > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I mean if it's a required migration, then it should be done and I'll > >>>>>>>>>>>>>> support it from both bridge and panel PoV. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a > >>>>>>>>>>>>>> better explanation on the commit message in any case. Then I would > >>>>>>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it > >>>>>>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the > >>>>>>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? > >>>>>>>>>>>> > >>>>>>>>>>>> So let me resume the situation: > >>>>>>>>>>>> > >>>>>>>>>>>> - pre_enable_prev_first was introduced in [1] > >>>>>>>>>>>> - some panels made use of pre_enable_prev_first > >>>>>>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before > >>>>>>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) > >>>>>>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next > >>>>>>>>>>> > >>>>>>>>>>> Hi Neil and Maxime, > >>>>>>>>>>> > >>>>>>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > >>>>>>>>>>> > >>>>>>>>>>> However, I'm not too keen on simply reverting that patch because > >>>>>>>>>>> > >>>>>>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and > >>>>>>>>>> > >>>>>>>>>> I never objected that, it's the right path to go. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> Ack. > >>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. > >>>>>>>>>> > >>>>>>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding > >>>>>>>>>> a proper atomic panel API to handle a "negociation" with the host controller. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> May I know what type of negotiation is needed here? > >>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? > >>>>>>>>>> > >>>>>>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> I wanted to respond to this earlier but didnt get a chance. > >>>>>>>>> > >>>>>>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). > >>>>>>>>> > >>>>>>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. > >>>>>>>>> > >>>>>>>>> 195 * The previous controller should be prepared first, before the prepare > >>>>>>>>> 196 * for the panel is called. This is largely required for DSI panels > >>>>>>>>> 197 * where the DSI host controller should be initialised to LP-11 before > >>>>>>>>> 198 * the panel is powered up. > >>>>>>>>> 199 */ > >>>>>>>>> 200 bool prepare_prev_first; > >>>>>>>>> > >>>>>>>>> These are conceptually different and thats what I explained Dmitry in our call. > >>>>>>>>> > >>>>>>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. > >>>>>>>>> > >>>>>>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). > >>>>>>>>> > >>>>>>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. > >>>>>>>>> > >>>>>>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. > >>>>>>>>> > >>>>>>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. > >>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. > >>>>>>>>>> > >>>>>>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. > >>>>>>>>> > >>>>>>>>> Let me know if this works as a migration plan. > >>>>>>>> > >>>>>>>> Yep agreed, let's do this > >>>>>>>> > >>>>>>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. > >>>>>>> > >>>>>>> This will break all non-DSI panels, which might depend on the current > >>>>>>> bridge calling order. > >>>>>>> > >>>>>>> I started looking at the explicit DSI power up sequencing, but it will > >>>>>>> take a few more days to mature. > >>>>>>> > >>>>>> > >>>>>> May I know why this would break all non-DSI panels? > >>>>> > >>>>> Existing panel drivers might be depending on the init order. Do we > >>>>> know for sure that none of DPI panels will be broken if there is a > >>>>> video stream ongoing during the reset procedure? > >>>>> Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. > >>>>> > >>>> > >>>> Can you please explain why would video stream be ON in pre_enable()? > >>>> > >>>> Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the > >>>> timing engine is not enabled until the encoder's enable and the first > >>>> commit after that so video stream wont be sent till then. > >>> > >>> You are describing the MSM DSI case. I was pointing to the fact that > >>> parent's pre_enable if called too early might conflict with the next > >>> bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a > >>> full-featured video stream, this state might confuse the panel. So we > >>> can not blindly switch the order of pre_enable callbacks for the > >>> bridge-panel pair. > >>> > >> > >> Even if the end connector was a eDP or DPI, the input to the bridge was > >> DSI so I think its unlikely that video stream was on before encoder's > >> enable but I cannot speak for all these interfaces/vendors. > >> > >> So, to accommodate both worlds, does this work? > >> > >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > >> index 9316384b4474..2b38388d4e56 100644 > >> --- a/drivers/gpu/drm/bridge/panel.c > >> +++ b/drivers/gpu/drm/bridge/panel.c > >> @@ -416,7 +416,10 @@ struct drm_bridge > >> *devm_drm_panel_bridge_add_typed(struct device *dev, > >> return bridge; > >> } > >> > >> - bridge->pre_enable_prev_first = panel->prepare_prev_first; > >> + if (connector_type == DRM_MODE_CONNECTOR_DSI) > >> + bridge->pre_enable_prev_first = true; > >> + else > >> + bridge->pre_enable_prev_first = panel->prepare_prev_first; > > > > looks like a hack. > > > > Nope its not, DSI spec has clear mentions about LP11 state to be used > during initialization. > > "After power-up, the host processor shall observe an initialization > period, TINIT, during which it shall drive a > sustained TX-Stop state (LP-11) on all Lanes of the Link. See [MIPI04] > for descriptions of TINIT and the TX-Stop state. > > Peripherals shall power up in the RX-Stop state and monitor the Link to > determine if the host processor has > asserted a TX-Stop state for at least the TINIT period. The peripheral > shall ignore all Link states prior to > detection of a TINIT event. The peripheral shall be ready to accept bus > transactions immediately following the end of the TINIT period." > > I am only extending this statement by mandating that the DSI PHY / > controller be powered up so that its able to drive the lanes to LP11 state. > > Now, pls explain why this is a hack Because it centers DSI panels only, not taking bridges into account. Because doesn't allow the panel to override this decision, etc. > > >> > >> *ptr = bridge; > >> devres_add(dev, ptr); > >> > >>>> > >>>> drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. > >>>> > >>>> drm_atomic_bridge_chain_enable() is the one which is called after the > >>>> encoder's enable(). > >>>> > >>>>>> > >>>>>> Like I said, we dont know the full details of the parade issue but I do > >>>>>> not see any reason why powering on a bridge chip with the DSI lanes > >>>>>> being in proper LP11 state should cause an issue. Its a well defined and > >>>>>> documented state in DSI spec. > >>>>>> > >>>>>> On the contrary, trying to turn on a bridge chip before powering on a > >>>>>> controller could have more issues as we do not know what state the lanes > >>>>>> are in when the MIPI devices (panel or bridge) are powered up. > >>>>>> > >>>>>> This sets the expectation and handshake straight. > >>>>> > >>>>> > >>> > >>> > >>> > > > > > >
On 8/29/2023 7:05 PM, Dmitry Baryshkov wrote: > On Tue, 29 Aug 2023 at 22:21, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 8/29/2023 12:15 PM, Dmitry Baryshkov wrote: >>> On Tue, 29 Aug 2023 at 22:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 8/29/2023 11:51 AM, Dmitry Baryshkov wrote: >>>>> On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote: >>>>>>> On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote: >>>>>>>>> On Tue, 29 Aug 2023 at 12:22, <neil.armstrong@linaro.org> wrote: >>>>>>>>>> >>>>>>>>>> On 28/08/2023 19:07, Abhinav Kumar wrote: >>>>>>>>>>> Hi Neil >>>>>>>>>>> >>>>>>>>>>> Sorry I didnt respond earlier on this thread. >>>>>>>>>>> >>>>>>>>>>> On 8/28/2023 1:49 AM, neil.armstrong@linaro.org wrote: >>>>>>>>>>>> Hi Jessica, >>>>>>>>>>>> >>>>>>>>>>>> On 25/08/2023 20:37, Jessica Zhang wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >>>>>>>>>>>>>> Hi Maxime, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 21/08/2023 10:17, Maxime Ripard wrote: >>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>>>>>>>>>>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>>>>>>>>>>>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>>>>>>>>>>>>>> Sending HS commands will always work on any controller, it's all >>>>>>>>>>>>>>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>>>>>>>>>>>>>> commands so they can use prepare_prev_first and work on any >>>>>>>>>>>>>>>>>> controllers. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I think there is some misunderstanding there, supported by the >>>>>>>>>>>>>>>>> description of the flag. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>>>>>>>>>>>>>> commands after enabling video stream and switching to HS mode, see >>>>>>>>>>>>>>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>>>>>>>>>>>>>> commands in drm_panel_funcs::prepare() / >>>>>>>>>>>>>>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>>>>>>>>>>>>>> whether these commands are to be sent in LP or in HS mode. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Previously DSI source drivers could power on the DSI link either in >>>>>>>>>>>>>>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>>>>>>>>>>>>>> hack to make panel/bridge drivers to be able to send commands from >>>>>>>>>>>>>>>>> their prepare() / pre_enable() callbacks. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> With the prev_first flags being introduced, we have established that >>>>>>>>>>>>>>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>>>>>>>>>>>>>> switched to HS mode (be it command or video) in the enable() >>>>>>>>>>>>>>>>> callback. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> So far so good. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> It seems coherent, I would like first to have a state of all DSI host >>>>>>>>>>>>>>>> drivers and make this would actually work first before adding the >>>>>>>>>>>>>>>> prev_first flag to all the required panels. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This is definitely what we should do in an ideal world, but at least for >>>>>>>>>>>>>>> sunxi there's no easy way for it at the moment. There's no documentation >>>>>>>>>>>>>>> for it and the driver provided doesn't allow this to happen. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Note that I'm not trying to discourage you or something here, I'm simply >>>>>>>>>>>>>>> pointing out that this will be something that we will have to take into >>>>>>>>>>>>>>> account. And it's possible that other drivers are in a similar >>>>>>>>>>>>>>> situation. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Unfortunately this change is not fully backwards-compatible. This >>>>>>>>>>>>>>>>> requires that all DSI panels sending commands from prepare() should >>>>>>>>>>>>>>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>>>>>>>>>>>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>>>>>>>>>>>>>> flag to drm_panel"). >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> This kind of migration should be done *before* any possible >>>>>>>>>>>>>>>> regression, not the other way round. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> If all panels sending commands from prepare() should have the >>>>>>>>>>>>>>>> prepare_prev_first flag, then it should be first, check for >>>>>>>>>>>>>>>> regressions then continue. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> <snip> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>>>>>>>>>>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>>>>>>>>>>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>>>>>>>>>>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> If we go this way, we can never reapply these patches. There will be >>>>>>>>>>>>>>>>> no guarantee that all panel drivers are completely converted. We >>>>>>>>>>>>>>>>> already have a story without an observable end - >>>>>>>>>>>>>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I don't understand this point, who would block re-applying the patches ? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>>>>>>>>>>>>>> Linux version and went smoothly because we reverted regressing patches >>>>>>>>>>>>>>>> and restarted when needed, I don't understand why we can't do this >>>>>>>>>>>>>>>> here aswell. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I'd consider that the DSI driver is correct here and it is about the >>>>>>>>>>>>>>>>> panel drivers that require fixes patches. If you care about the >>>>>>>>>>>>>>>>> particular Fixes tag, I have provided one several lines above. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Unfortunately it should be done in the other way round, prepare for >>>>>>>>>>>>>>>> migration, then migrate, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I mean if it's a required migration, then it should be done and I'll >>>>>>>>>>>>>>>> support it from both bridge and panel PoV. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> So, first this patch has the wrong Fixes tag, and I would like a >>>>>>>>>>>>>>>> better explanation on the commit message in any case. Then I would >>>>>>>>>>>>>>>> like to have an ack from some drm-misc maintainers before applying it >>>>>>>>>>>>>>>> because it fixes a patch that was sent via the msm tree thus per the >>>>>>>>>>>>>>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Sorry, it's not clear to me what you'd like our feedback on exactly? >>>>>>>>>>>>>> >>>>>>>>>>>>>> So let me resume the situation: >>>>>>>>>>>>>> >>>>>>>>>>>>>> - pre_enable_prev_first was introduced in [1] >>>>>>>>>>>>>> - some panels made use of pre_enable_prev_first >>>>>>>>>>>>>> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >>>>>>>>>>>>>> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >>>>>>>>>>>>>> - this fix was sent late, and is now too late to be merged via drm-misc-next >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Neil and Maxime, >>>>>>>>>>>>> >>>>>>>>>>>>> I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). >>>>>>>>>>>>> >>>>>>>>>>>>> However, I'm not too keen on simply reverting that patch because >>>>>>>>>>>>> >>>>>>>>>>>>> 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and >>>>>>>>>>>> >>>>>>>>>>>> I never objected that, it's the right path to go. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Ack. >>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. >>>>>>>>>>>> >>>>>>>>>>>> We discussed about a plan with Maxime and Dmitry about that, and it would require adding >>>>>>>>>>>> a proper atomic panel API to handle a "negociation" with the host controller. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> May I know what type of negotiation is needed here? >>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? >>>>>>>>>>>> >>>>>>>>>>>> As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I wanted to respond to this earlier but didnt get a chance. >>>>>>>>>>> >>>>>>>>>>> From the documentation of this flag, this has nothing to do whether panels are sending the LP commands (commands sent in LP mode) OR HS commands (commands sent in HS mode). >>>>>>>>>>> >>>>>>>>>>> This is more about sending the commands whether the lanes are in LP11 state before sending the ON commands. >>>>>>>>>>> >>>>>>>>>>> 195 * The previous controller should be prepared first, before the prepare >>>>>>>>>>> 196 * for the panel is called. This is largely required for DSI panels >>>>>>>>>>> 197 * where the DSI host controller should be initialised to LP-11 before >>>>>>>>>>> 198 * the panel is powered up. >>>>>>>>>>> 199 */ >>>>>>>>>>> 200 bool prepare_prev_first; >>>>>>>>>>> >>>>>>>>>>> These are conceptually different and thats what I explained Dmitry in our call. >>>>>>>>>>> >>>>>>>>>>> Sending ON commands in LP11 state is a requirement I have seen with many panels and its actually the right expectation as well to send the commands when the lanes are in a well-defined LP11 state. >>>>>>>>>>> >>>>>>>>>>> From the panels which I have seen, the opposite is never true (OR i have never seen it this way). >>>>>>>>>>> >>>>>>>>>>> The parade chip was the only exception and that issue was never root-caused leading us to have bridge specific handling in MSM driver. >>>>>>>>>>> >>>>>>>>>>> In other words, it would be very unlikely that a panel should be broken or shouldn't work when the ON commands are sent when the lanes are in LP11 state. >>>>>>>>>>> >>>>>>>>>>> So I agree with Jessica, that we should set the default value of this flag to true in the framework so that only the bridges/panels which need this to be false do that explicitly. From the examples I pointed out including MTK, even those vendors are powering on their DSI in pre_enable() which means none of these panels will work there too. >>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. >>>>>>>>>>>> >>>>>>>>>>>> A proper migration should be done, yes, but not as a fix on top of v6.5. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I am fine to drop this fix in favor of making the prepare_prev_first as default true but we need an agreement first. From what I can see, parade chip will be the only one which will need this to be set to false and we can make that change. >>>>>>>>>>> >>>>>>>>>>> Let me know if this works as a migration plan. >>>>>>>>>> >>>>>>>>>> Yep agreed, let's do this >>>>>>>>>> >>>>>>>>>> The panel's prepare_prev_first should be reversed to something like not_prepare_prev_first to make it an exception. >>>>>>>>> >>>>>>>>> This will break all non-DSI panels, which might depend on the current >>>>>>>>> bridge calling order. >>>>>>>>> >>>>>>>>> I started looking at the explicit DSI power up sequencing, but it will >>>>>>>>> take a few more days to mature. >>>>>>>>> >>>>>>>> >>>>>>>> May I know why this would break all non-DSI panels? >>>>>>> >>>>>>> Existing panel drivers might be depending on the init order. Do we >>>>>>> know for sure that none of DPI panels will be broken if there is a >>>>>>> video stream ongoing during the reset procedure? >>>>>>> Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first. >>>>>>> >>>>>> >>>>>> Can you please explain why would video stream be ON in pre_enable()? >>>>>> >>>>>> Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the >>>>>> timing engine is not enabled until the encoder's enable and the first >>>>>> commit after that so video stream wont be sent till then. >>>>> >>>>> You are describing the MSM DSI case. I was pointing to the fact that >>>>> parent's pre_enable if called too early might conflict with the next >>>>> bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a >>>>> full-featured video stream, this state might confuse the panel. So we >>>>> can not blindly switch the order of pre_enable callbacks for the >>>>> bridge-panel pair. >>>>> >>>> >>>> Even if the end connector was a eDP or DPI, the input to the bridge was >>>> DSI so I think its unlikely that video stream was on before encoder's >>>> enable but I cannot speak for all these interfaces/vendors. >>>> >>>> So, to accommodate both worlds, does this work? >>>> >>>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c >>>> index 9316384b4474..2b38388d4e56 100644 >>>> --- a/drivers/gpu/drm/bridge/panel.c >>>> +++ b/drivers/gpu/drm/bridge/panel.c >>>> @@ -416,7 +416,10 @@ struct drm_bridge >>>> *devm_drm_panel_bridge_add_typed(struct device *dev, >>>> return bridge; >>>> } >>>> >>>> - bridge->pre_enable_prev_first = panel->prepare_prev_first; >>>> + if (connector_type == DRM_MODE_CONNECTOR_DSI) >>>> + bridge->pre_enable_prev_first = true; >>>> + else >>>> + bridge->pre_enable_prev_first = panel->prepare_prev_first; >>> >>> looks like a hack. >>> >> >> Nope its not, DSI spec has clear mentions about LP11 state to be used >> during initialization. >> >> "After power-up, the host processor shall observe an initialization >> period, TINIT, during which it shall drive a >> sustained TX-Stop state (LP-11) on all Lanes of the Link. See [MIPI04] >> for descriptions of TINIT and the TX-Stop state. >> >> Peripherals shall power up in the RX-Stop state and monitor the Link to >> determine if the host processor has >> asserted a TX-Stop state for at least the TINIT period. The peripheral >> shall ignore all Link states prior to >> detection of a TINIT event. The peripheral shall be ready to accept bus >> transactions immediately following the end of the TINIT period." >> >> I am only extending this statement by mandating that the DSI PHY / >> controller be powered up so that its able to drive the lanes to LP11 state. >> >> Now, pls explain why this is a hack > > Because it centers DSI panels only, not taking bridges into account. > Because doesn't allow the panel to override this decision, etc. > Ok understood. Thats an implementation detail in that snippet I posted. But in principle, do we all agree here that we should be defaulting to true for DSI panels/bridges here? We can work out the details based on that. >> >>>> >>>> *ptr = bridge; >>>> devres_add(dev, ptr); >>>> >>>>>> >>>>>> drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable. >>>>>> >>>>>> drm_atomic_bridge_chain_enable() is the one which is called after the >>>>>> encoder's enable(). >>>>>> >>>>>>>> >>>>>>>> Like I said, we dont know the full details of the parade issue but I do >>>>>>>> not see any reason why powering on a bridge chip with the DSI lanes >>>>>>>> being in proper LP11 state should cause an issue. Its a well defined and >>>>>>>> documented state in DSI spec. >>>>>>>> >>>>>>>> On the contrary, trying to turn on a bridge chip before powering on a >>>>>>>> controller could have more issues as we do not know what state the lanes >>>>>>>> are in when the MIPI devices (panel or bridge) are powered up. >>>>>>>> >>>>>>>> This sets the expectation and handshake straight. >>>>>>> >>>>>>> >>>>> >>>>> >>>>> >>> >>> >>> > > >
diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c index bb0dfd86ea67..e1363e128e7e 100644 --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) dsi->format = MIPI_DSI_FMT_RGB888; dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; + ctx->panel.prepare_prev_first = true; drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, DRM_MODE_CONNECTOR_DSI);
Due to a recent introduction of the pre_enable_prev_first bridge flag [1], the panel driver will be probed before the DSI is enabled, causing the DCS commands to fail to send. Ensure that DSI is enabled before panel probe by setting the prepare_prev_first flag for the panel. [1] commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel driver") Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 + 1 file changed, 1 insertion(+) --- base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f Best regards,