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 <[email protected]>
---
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,
--
Jessica Zhang <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>>> ---
>>>> 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 Tue, 29 Aug 2023 at 12:22, <[email protected]> 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, [email protected] wrote:
> >> Hi Jessica,
> >>
> >> On 25/08/2023 20:37, Jessica Zhang wrote:
> >>>
> >>>
> >>> On 8/21/2023 3:01 AM, [email protected] wrote:
> >>>> Hi Maxime,
> >>>>
> >>>> On 21/08/2023 10:17, Maxime Ripard wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, [email protected] wrote:
> >>>>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote:
> >>>>>>> On 16/08/2023 10:51, [email protected] 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.
--
With best wishes
Dmitry