[PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Marek Vasut]

In rare cases, the bridge may not start up correctly, which usually
leads to no display output. In case this happens, warn about it in
the kernel log.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: dri-devel@lists.freedesktop.org
---
NOTE: See the following:
https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index a32f70bc68ea4..4ea71d7f0bfbc 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
+
+	usleep_range(10000, 12000);
+	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
+	if (pval)
+		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
 }
 
 static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
-- 
2.33.0

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Andrzej Hajda]

On 07.09.2021 04:39, Marek Vasut wrote:
> In rare cases, the bridge may not start up correctly, which usually
> leads to no display output. In case this happens, warn about it in
> the kernel log.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: dri-devel@lists.freedesktop.org
> ---
> NOTE: See the following:
> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
> ---
>   drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index a32f70bc68ea4..4ea71d7f0bfbc 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>   	/* Clear all errors that got asserted during initialization. */
>   	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>   	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);


It does not look as correct error handling, maybe it would be good to 
analyze and optionally report 'unexpected' errors here as well.


> +
> +	usleep_range(10000, 12000);
> +	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> +	if (pval)
> +		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);


I am not sure what is the case here but it looks like 'we do not know 
what is going on, so let's add some diagnostic messages to gather info 
and figure it out later'.

Whole driver lacks IRQ handler which IMO could perform better diagnosis, 
and I guess it could also help in recovery, but this is just my guess.
So if this patch is enough for now you can add:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Regards
Andrzej


>   }
>   
>   static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
> 

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Marek Vasut]

On 9/7/21 9:31 AM, Andrzej Hajda wrote:
> On 07.09.2021 04:39, Marek Vasut wrote:
>> In rare cases, the bridge may not start up correctly, which usually
>> leads to no display output. In case this happens, warn about it in
>> the kernel log.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>> NOTE: See the following:
>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
>> ---
>>    drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
>>    1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>>    	/* Clear all errors that got asserted during initialization. */
>>    	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>    	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
> 
> 
> It does not look as correct error handling, maybe it would be good to
> analyze and optionally report 'unexpected' errors here as well.

The above is correct -- it clears the status register because the setup 
might've set random bits in that register. Then we wait a bit, let the 
link run, and read them again to get the real link status in this new 
piece of code below, hence the usleep_range there. And then if the link 
indicates a problem, we know it is a problem.

>> +
>> +	usleep_range(10000, 12000);
>> +	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>> +	if (pval)
>> +		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
> 
> 
> I am not sure what is the case here but it looks like 'we do not know
> what is going on, so let's add some diagnostic messages to gather info
> and figure it out later'.

That's pretty much the case, see the two links above in the NOTE 
section. If something goes wrong, we print the value for the user 
(usually developer) so they can fix their problems. We cannot do much 
better in the attach callback.

The issue I ran into (and where this would be helpful information to me 
during debugging, since the issue happened real seldom, see also the 
NOTE links above) is that the DSI controller driver started streaming 
video on the data lanes before the DSI83 had a chance to initialize. 
This worked most of the time, except for a few exceptions here and 
there, where the video didn't start. This does set link status bits 
consistently. In the meantime, I fixed the controller driver (so far 
downstream, due to ongoing discussion).

> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
> and I guess it could also help in recovery, but this is just my guess.
> So if this patch is enough for now you can add:

No, IRQ won't help you here, because by the time you get the IRQ, the 
DSI host already started streaming video on data lanes and you won't be 
able to correctly reinit the DSI83 unless you communicate to the DSI 
host that it should switch the data lanes back to LP11.

And for that, there is a bigger chunk missing really. What needs to be 
added is a way for the DSI bridge / panel to communicate its needs to 
the DSI host -- things like "I need DSI clock lane frequency f MHz, I 
need clock lane in HS mode and data lanes in LP11 mode". If you look at 
the way DSI hosts and bridges/panels work out the DSI link parameters, 
you will notice they basically do it each on their own, there is no such 
API or communication channel.

With the above in place, you could have an interrupt which would use it, 
notify the host to configure the DSI bus in required way, then you would 
be able to reinit the DSI83, and then again notify the DSI host to start 
streaming video.

> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> Regards
> Andrzej
> 
> 
>>    }
>>    
>>    static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
>>
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Andrzej Hajda]

W dniu 07.09.2021 o 16:25, Marek Vasut pisze:
> On 9/7/21 9:31 AM, Andrzej Hajda wrote:
>> On 07.09.2021 04:39, Marek Vasut wrote:
>>> In rare cases, the bridge may not start up correctly, which usually
>>> leads to no display output. In case this happens, warn about it in
>>> the kernel log.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Robert Foss <robert.foss@linaro.org>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: dri-devel@lists.freedesktop.org
>>> ---
>>> NOTE: See the following:
>>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video 
>>>
>>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533 
>>>
>>> ---
>>>    drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
>>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct 
>>> drm_bridge *bridge,
>>>        /* Clear all errors that got asserted during initialization. */
>>>        regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>>        regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
>>
>>
>> It does not look as correct error handling, maybe it would be good to
>> analyze and optionally report 'unexpected' errors here as well.
>
> The above is correct -- it clears the status register because the 
> setup might've set random bits in that register. Then we wait a bit, 
> let the link run, and read them again to get the real link status in 
> this new piece of code below, hence the usleep_range there. And then 
> if the link indicates a problem, we know it is a problem.


Usually such registers are cleared on very beginning of the 
initialization, and tested (via irq handler, or via reading), during 
initalization, if initialization phase goes well. If it is not the case 
forgive me.


>
>>> +
>>> +    usleep_range(10000, 12000);
>>> +    regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>> +    if (pval)
>>> +        dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
>>
>>
>> I am not sure what is the case here but it looks like 'we do not know
>> what is going on, so let's add some diagnostic messages to gather info
>> and figure it out later'.
>
> That's pretty much the case, see the two links above in the NOTE 
> section. If something goes wrong, we print the value for the user 
> (usually developer) so they can fix their problems. We cannot do much 
> better in the attach callback.
>
> The issue I ran into (and where this would be helpful information to 
> me during debugging, since the issue happened real seldom, see also 
> the NOTE links above) is that the DSI controller driver started 
> streaming video on the data lanes before the DSI83 had a chance to 
> initialize. This worked most of the time, except for a few exceptions 
> here and there, where the video didn't start. This does set link 
> status bits consistently. In the meantime, I fixed the controller 
> driver (so far downstream, due to ongoing discussion).


Maybe drm_connector_set_link_status_property(conn, 
DRM_MODE_LINK_STATUS_BAD) would be usefule here.


>
>> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
>> and I guess it could also help in recovery, but this is just my guess.
>> So if this patch is enough for now you can add:
>
> No, IRQ won't help you here, because by the time you get the IRQ, the 
> DSI host already started streaming video on data lanes and you won't 
> be able to correctly reinit the DSI83 unless you communicate to the 
> DSI host that it should switch the data lanes back to LP11.
>
> And for that, there is a bigger chunk missing really. What needs to be 
> added is a way for the DSI bridge / panel to communicate its needs to 
> the DSI host -- things like "I need DSI clock lane frequency f MHz, I 
> need clock lane in HS mode and data lanes in LP11 mode". If you look 
> at the way DSI hosts and bridges/panels work out the DSI link 
> parameters, you will notice they basically do it each on their own, 
> there is no such API or communication channel.


There is one-time communication channel via mipi_dsi_attach, it allows 
to set max frequency i HS and LP, choose mode of operation (HS/LPM) and 
few more things. If it is necessary to extend it please propse sth.

Regarding requesting LP11 I am not sure if we really should have such 
low level communication. LP11, as I remember ,is initial state in HS so 
it should be set anyway, before starting video transmission.


And maybe this is the main problem:

DRM core calls:

crtc->enable

bridges->pre_enable,

encoder->enable,

bridges->enable.


Usually video transmission starts in crtc->enable (CRTC->Encoder), and 
in encoder->enable (encoder->bridge), so in bridges->enable it would be 
too late for LP11 state - transmission can be already in progress.

It shows well that this order of calls does not fit well to DSI, and 
probably many other protocols.

Maybe moving most of the bridge->enable code to bridge->pre_enable would 
help, but I am not sur if it will not pose another issues.

This is quick analysis, so please fix me if I am wrong.


Regards

Andrzej

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Marek Vasut]

On 9/7/21 7:29 PM, Andrzej Hajda wrote:
> 
> W dniu 07.09.2021 o 16:25, Marek Vasut pisze:
>> On 9/7/21 9:31 AM, Andrzej Hajda wrote:
>>> On 07.09.2021 04:39, Marek Vasut wrote:
>>>> In rare cases, the bridge may not start up correctly, which usually
>>>> leads to no display output. In case this happens, warn about it in
>>>> the kernel log.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: Robert Foss <robert.foss@linaro.org>
>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> ---
>>>> NOTE: See the following:
>>>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
>>>>
>>>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
>>>>
>>>> ---
>>>>     drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
>>>>     1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct
>>>> drm_bridge *bridge,
>>>>         /* Clear all errors that got asserted during initialization. */
>>>>         regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>>>         regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
>>>
>>>
>>> It does not look as correct error handling, maybe it would be good to
>>> analyze and optionally report 'unexpected' errors here as well.
>>
>> The above is correct -- it clears the status register because the
>> setup might've set random bits in that register. Then we wait a bit,
>> let the link run, and read them again to get the real link status in
>> this new piece of code below, hence the usleep_range there. And then
>> if the link indicates a problem, we know it is a problem.
> 
> 
> Usually such registers are cleared on very beginning of the
> initialization, and tested (via irq handler, or via reading), during
> initalization, if initialization phase goes well. If it is not the case
> forgive me.

The init just flips the bit at random in the IRQ_STAT register, so no, 
that's not really viable here. That's why we clear them at the end, and 
then wait a bit, and then check whether something new appeared in them.

If not, all is great.

Sure, we could generate an IRQ, but then IRQ line is not always 
connected to this chip on all hardware I have available. So this gives 
the user at least some indication that something is wrong with their HW.

>>>> +
>>>> +    usleep_range(10000, 12000);
>>>> +    regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>>> +    if (pval)
>>>> +        dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
>>>
>>>
>>> I am not sure what is the case here but it looks like 'we do not know
>>> what is going on, so let's add some diagnostic messages to gather info
>>> and figure it out later'.
>>
>> That's pretty much the case, see the two links above in the NOTE
>> section. If something goes wrong, we print the value for the user
>> (usually developer) so they can fix their problems. We cannot do much
>> better in the attach callback.
>>
>> The issue I ran into (and where this would be helpful information to
>> me during debugging, since the issue happened real seldom, see also
>> the NOTE links above) is that the DSI controller driver started
>> streaming video on the data lanes before the DSI83 had a chance to
>> initialize. This worked most of the time, except for a few exceptions
>> here and there, where the video didn't start. This does set link
>> status bits consistently. In the meantime, I fixed the controller
>> driver (so far downstream, due to ongoing discussion).
> 
> 
> Maybe drm_connector_set_link_status_property(conn,
> DRM_MODE_LINK_STATUS_BAD) would be usefule here.

Hmm, this works on connector, the dsi83 is a bridge and it can be stuck 
between two other bridges. That doesn't seem like the right tool, no ?

>>> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
>>> and I guess it could also help in recovery, but this is just my guess.
>>> So if this patch is enough for now you can add:
>>
>> No, IRQ won't help you here, because by the time you get the IRQ, the
>> DSI host already started streaming video on data lanes and you won't
>> be able to correctly reinit the DSI83 unless you communicate to the
>> DSI host that it should switch the data lanes back to LP11.
>>
>> And for that, there is a bigger chunk missing really. What needs to be
>> added is a way for the DSI bridge / panel to communicate its needs to
>> the DSI host -- things like "I need DSI clock lane frequency f MHz, I
>> need clock lane in HS mode and data lanes in LP11 mode". If you look
>> at the way DSI hosts and bridges/panels work out the DSI link
>> parameters, you will notice they basically do it each on their own,
>> there is no such API or communication channel.
> 
> 
> There is one-time communication channel via mipi_dsi_attach, it allows
> to set max frequency i HS and LP, choose mode of operation (HS/LPM) and
> few more things. If it is necessary to extend it please propse sth.

Well, take for example the drivers/gpu/drm/exynos/exynos_drm_dsi.c , 
there is this:

static void exynos_dsi_enable(struct drm_encoder *encoder)
...
                 list_for_each_entry_reverse(iter, &dsi->bridge_chain,
                                             chain_node) {
                         if (iter->funcs->pre_enable)
                                 iter->funcs->pre_enable(iter);
...
         exynos_dsi_set_display_mode(dsi);
         exynos_dsi_set_display_enable(dsi, true);
...
                 list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
                         if (iter->funcs->enable)
                                 iter->funcs->enable(iter);
                 }
...

So the bridge enable callback is called with clock lane already in HS 
mode, and data lanes streaming video. This doesn't work with the DSI83, 
which would need clock lane in HS and providing clock , but data lanes 
in LP11 with no video.

Sure, I could probably move exynos_dsi_set_display_enable(dsi, true); 
after the enable call, but is that really the right solution ? What 
about bridges which need some other specific configuration of the data 
lanes during init ?

> Regarding requesting LP11 I am not sure if we really should have such
> low level communication. LP11, as I remember ,is initial state in HS so
> it should be set anyway, before starting video transmission.

Well, see above, that's the problem I ran across recently.

> And maybe this is the main problem:
> 
> DRM core calls:
> 
> crtc->enable
> 
> bridges->pre_enable,
> 
> encoder->enable,
> 
> bridges->enable.
> 
> 
> Usually video transmission starts in crtc->enable (CRTC->Encoder), and
> in encoder->enable (encoder->bridge), so in bridges->enable it would be
> too late for LP11 state - transmission can be already in progress.
> 
> It shows well that this order of calls does not fit well to DSI, and
> probably many other protocols.
> 
> Maybe moving most of the bridge->enable code to bridge->pre_enable would
> help, but I am not sur if it will not pose another issues.

Yep, that won't work e.g. with the exynos DSIM, because 
exynos_dsi_set_display_mode() sets the data lanes to LP11.

> This is quick analysis, so please fix me if I am wrong.

I pretty much agree that the current state of things does not fit with 
DSI too well.

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Dave Stevenson]

Hi Marek and Andrzej

On Tue, 7 Sept 2021 at 22:24, Marek Vasut <marex@denx.de> wrote:
>
> On 9/7/21 7:29 PM, Andrzej Hajda wrote:
> >
> > W dniu 07.09.2021 o 16:25, Marek Vasut pisze:
> >> On 9/7/21 9:31 AM, Andrzej Hajda wrote:
> >>> On 07.09.2021 04:39, Marek Vasut wrote:
> >>>> In rare cases, the bridge may not start up correctly, which usually
> >>>> leads to no display output. In case this happens, warn about it in
> >>>> the kernel log.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
> >>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Cc: Linus Walleij <linus.walleij@linaro.org>
> >>>> Cc: Robert Foss <robert.foss@linaro.org>
> >>>> Cc: Sam Ravnborg <sam@ravnborg.org>
> >>>> Cc: dri-devel@lists.freedesktop.org
> >>>> ---
> >>>> NOTE: See the following:
> >>>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
> >>>>
> >>>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
> >>>>
> >>>> ---
> >>>>     drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
> >>>>     1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
> >>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct
> >>>> drm_bridge *bridge,
> >>>>         /* Clear all errors that got asserted during initialization. */
> >>>>         regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> >>>>         regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
> >>>
> >>>
> >>> It does not look as correct error handling, maybe it would be good to
> >>> analyze and optionally report 'unexpected' errors here as well.
> >>
> >> The above is correct -- it clears the status register because the
> >> setup might've set random bits in that register. Then we wait a bit,
> >> let the link run, and read them again to get the real link status in
> >> this new piece of code below, hence the usleep_range there. And then
> >> if the link indicates a problem, we know it is a problem.
> >
> >
> > Usually such registers are cleared on very beginning of the
> > initialization, and tested (via irq handler, or via reading), during
> > initalization, if initialization phase goes well. If it is not the case
> > forgive me.
>
> The init just flips the bit at random in the IRQ_STAT register, so no,
> that's not really viable here. That's why we clear them at the end, and
> then wait a bit, and then check whether something new appeared in them.
>
> If not, all is great.
>
> Sure, we could generate an IRQ, but then IRQ line is not always
> connected to this chip on all hardware I have available. So this gives
> the user at least some indication that something is wrong with their HW.
>
> >>>> +
> >>>> +    usleep_range(10000, 12000);
> >>>> +    regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> >>>> +    if (pval)
> >>>> +        dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
> >>>
> >>>
> >>> I am not sure what is the case here but it looks like 'we do not know
> >>> what is going on, so let's add some diagnostic messages to gather info
> >>> and figure it out later'.
> >>
> >> That's pretty much the case, see the two links above in the NOTE
> >> section. If something goes wrong, we print the value for the user
> >> (usually developer) so they can fix their problems. We cannot do much
> >> better in the attach callback.
> >>
> >> The issue I ran into (and where this would be helpful information to
> >> me during debugging, since the issue happened real seldom, see also
> >> the NOTE links above) is that the DSI controller driver started
> >> streaming video on the data lanes before the DSI83 had a chance to
> >> initialize. This worked most of the time, except for a few exceptions
> >> here and there, where the video didn't start. This does set link
> >> status bits consistently. In the meantime, I fixed the controller
> >> driver (so far downstream, due to ongoing discussion).
> >
> >
> > Maybe drm_connector_set_link_status_property(conn,
> > DRM_MODE_LINK_STATUS_BAD) would be usefule here.
>
> Hmm, this works on connector, the dsi83 is a bridge and it can be stuck
> between two other bridges. That doesn't seem like the right tool, no ?
>
> >>> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
> >>> and I guess it could also help in recovery, but this is just my guess.
> >>> So if this patch is enough for now you can add:
> >>
> >> No, IRQ won't help you here, because by the time you get the IRQ, the
> >> DSI host already started streaming video on data lanes and you won't
> >> be able to correctly reinit the DSI83 unless you communicate to the
> >> DSI host that it should switch the data lanes back to LP11.
> >>
> >> And for that, there is a bigger chunk missing really. What needs to be
> >> added is a way for the DSI bridge / panel to communicate its needs to
> >> the DSI host -- things like "I need DSI clock lane frequency f MHz, I
> >> need clock lane in HS mode and data lanes in LP11 mode". If you look
> >> at the way DSI hosts and bridges/panels work out the DSI link
> >> parameters, you will notice they basically do it each on their own,
> >> there is no such API or communication channel.
> >
> >
> > There is one-time communication channel via mipi_dsi_attach, it allows
> > to set max frequency i HS and LP, choose mode of operation (HS/LPM) and
> > few more things. If it is necessary to extend it please propse sth.
>
> Well, take for example the drivers/gpu/drm/exynos/exynos_drm_dsi.c ,
> there is this:
>
> static void exynos_dsi_enable(struct drm_encoder *encoder)
> ...
>                  list_for_each_entry_reverse(iter, &dsi->bridge_chain,
>                                              chain_node) {
>                          if (iter->funcs->pre_enable)
>                                  iter->funcs->pre_enable(iter);
> ...
>          exynos_dsi_set_display_mode(dsi);
>          exynos_dsi_set_display_enable(dsi, true);
> ...
>                  list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
>                          if (iter->funcs->enable)
>                                  iter->funcs->enable(iter);
>                  }
> ...
>
> So the bridge enable callback is called with clock lane already in HS
> mode, and data lanes streaming video. This doesn't work with the DSI83,
> which would need clock lane in HS and providing clock , but data lanes
> in LP11 with no video.
>
> Sure, I could probably move exynos_dsi_set_display_enable(dsi, true);
> after the enable call, but is that really the right solution ? What
> about bridges which need some other specific configuration of the data
> lanes during init ?

I hadn't noticed that Exynos was doing that.
vc4 DSI is doing the same thing in deliberately breaking the
panel/bridge chain so that it gets a chance to do some initialisation
before panel/bridge pre_enable.

Another issue I've noted in doing this is that it breaks calls to the
bridge's mode_set, mode_valid, and mode_fixup hooks. The framework
doesn't know about the bridges/panels beyond the encoder, and the
encoder doesn't get all the information required in order to replicate
those calls.
I'm about to look into whether switching the DSI host to being a
bridge instead of an encoder allows me to overcome that one.
Doing so doesn't solve the issue of the DSI host bridge pre_enable
being called after the panel/bridge pre_enable though.

> > Regarding requesting LP11 I am not sure if we really should have such
> > low level communication. LP11, as I remember ,is initial state in HS so
> > it should be set anyway, before starting video transmission.

LP-11 is the idle LP state. Both P and N lines of the pair are at
LP-high (~1.2V).
You then have an escape sequence to shift to HS mode (LP-01 for
T(lpx), LP-00 for T(hs-prepare), enable HS driver) when you are
wishing to send data bursts. The signalling levels for HS drop to
~100mV for low and ~300mV for high.

Add in ULPS which is effectively powered off but has a specific entry
and escape sequence to sleep/wake up the receiving device. ULPS seems
to be totally ignored in DRM as it seems to rely on regulator or other
control of the panel/bridge to power down.

> Well, see above, that's the problem I ran across recently.
>
> > And maybe this is the main problem:
> >
> > DRM core calls:
> >
> > crtc->enable
> >
> > bridges->pre_enable,
> >
> > encoder->enable,
> >
> > bridges->enable.
> >
> >
> > Usually video transmission starts in crtc->enable (CRTC->Encoder), and
> > in encoder->enable (encoder->bridge), so in bridges->enable it would be
> > too late for LP11 state - transmission can be already in progress.
> >
> > It shows well that this order of calls does not fit well to DSI, and
> > probably many other protocols.
> >
> > Maybe moving most of the bridge->enable code to bridge->pre_enable would
> > help, but I am not sur if it will not pose another issues.
>
> Yep, that won't work e.g. with the exynos DSIM, because
> exynos_dsi_set_display_mode() sets the data lanes to LP11.

Isn't the bigger question for SN65DSI8[3|4|5] whether the clock lane
is running or not in pre_enable?

> > This is quick analysis, so please fix me if I am wrong.
>
> I pretty much agree that the current state of things does not fit with
> DSI too well.

That was why I was questioning how DSI was meant to be implemented in
https://lore.kernel.org/dri-devel/CAPY8ntBUKRkSam59Y+72dW_6XOeKVswPWffzPj3uvgE6pV4ZGQ@mail.gmail.com/

The need to have the DSI host in a defined idle state (often LP-11,
but varying whether the clock lane is in HS) before powering up the
panel/bridge is incredibly common, but currently undefined in DRM.

Taking the SN65DSI83 as an example, the datasheet [1] section 7.4.2
states that the clock lane must be in HS mode, and data lanes in LP-11
when coming out of reset. That means that we can't be "enable" as that
will have the data lanes in HS mode and sending video, and as we can't
be in "pre_enable" as the DSI PHY will be powered down and so we won't
have the clock lanes in HS mode.

I've hit a similar one with the Toshiba TC358762 where it seems to get
upset if it is receiving video data when it gets configured.
panel-raspberrypi-touchscreen[2] which drives that chip is
intermittent when using panel enable, whereas panel prepare is
significantly more reliable but relies on the DSI host being
initialised to LP-11 by breaking the chain.

  Dave

[1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf
[2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Marek Vasut]

On 9/8/21 1:11 PM, Dave Stevenson wrote:
> Hi Marek and Andrzej

Hello Dave,

skipping the protocol discussion, which I hope Andrej will pick up.

[...]

>>> Usually video transmission starts in crtc->enable (CRTC->Encoder), and
>>> in encoder->enable (encoder->bridge), so in bridges->enable it would be
>>> too late for LP11 state - transmission can be already in progress.
>>>
>>> It shows well that this order of calls does not fit well to DSI, and
>>> probably many other protocols.
>>>
>>> Maybe moving most of the bridge->enable code to bridge->pre_enable would
>>> help, but I am not sur if it will not pose another issues.
>>
>> Yep, that won't work e.g. with the exynos DSIM, because
>> exynos_dsi_set_display_mode() sets the data lanes to LP11.
> 
> Isn't the bigger question for SN65DSI8[3|4|5] whether the clock lane
> is running or not in pre_enable?

I think the bigger question really is -- how do we cater for all the 
different bridges with different init-time requirements.

>>> This is quick analysis, so please fix me if I am wrong.
>>
>> I pretty much agree that the current state of things does not fit with
>> DSI too well.
> 
> That was why I was questioning how DSI was meant to be implemented in
> https://lore.kernel.org/dri-devel/CAPY8ntBUKRkSam59Y+72dW_6XOeKVswPWffzPj3uvgE6pV4ZGQ@mail.gmail.com/
> 
> The need to have the DSI host in a defined idle state (often LP-11,
> but varying whether the clock lane is in HS) before powering up the
> panel/bridge is incredibly common, but currently undefined in DRM.
> 
> Taking the SN65DSI83 as an example, the datasheet [1] section 7.4.2
> states that the clock lane must be in HS mode, and data lanes in LP-11
> when coming out of reset. That means that we can't be "enable" as that
> will have the data lanes in HS mode and sending video, and as we can't
> be in "pre_enable" as the DSI PHY will be powered down and so we won't
> have the clock lanes in HS mode.
> 
> I've hit a similar one with the Toshiba TC358762 where it seems to get
> upset if it is receiving video data when it gets configured.
> panel-raspberrypi-touchscreen[2] which drives that chip is
> intermittent when using panel enable, whereas panel prepare is
> significantly more reliable but relies on the DSI host being
> initialised to LP-11 by breaking the chain.

Right

To make it worse, not initing the DSI bridge exactly per spec leads to 
intermittent failures, not consistently occuring ones.

>    Dave
> 
> [1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf
> [2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

Unrelated to this discussion -- there is a tc358762 driver, driver for 
that attiny88 regulator, and driver for the touchscreen chip, on that 
rpi 7" display, in upstream. You can use those to replace the composite 
panel driver (it works at least against stm32mp1 DSI host with the rpi 
7" panel). Sadly there is little documentation for that attiny88 
protocol or firmware, that's what I don't like about that panel.

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Dave Stevenson]

On Wed, 8 Sept 2021 at 16:26, Marek Vasut <marex@denx.de> wrote:
>
> On 9/8/21 1:11 PM, Dave Stevenson wrote:
> > Hi Marek and Andrzej
>
> Hello Dave,
>
> skipping the protocol discussion, which I hope Andrej will pick up.
>
> [...]
>
> >>> Usually video transmission starts in crtc->enable (CRTC->Encoder), and
> >>> in encoder->enable (encoder->bridge), so in bridges->enable it would be
> >>> too late for LP11 state - transmission can be already in progress.
> >>>
> >>> It shows well that this order of calls does not fit well to DSI, and
> >>> probably many other protocols.
> >>>
> >>> Maybe moving most of the bridge->enable code to bridge->pre_enable would
> >>> help, but I am not sur if it will not pose another issues.
> >>
> >> Yep, that won't work e.g. with the exynos DSIM, because
> >> exynos_dsi_set_display_mode() sets the data lanes to LP11.
> >
> > Isn't the bigger question for SN65DSI8[3|4|5] whether the clock lane
> > is running or not in pre_enable?
>
> I think the bigger question really is -- how do we cater for all the
> different bridges with different init-time requirements.
>
> >>> This is quick analysis, so please fix me if I am wrong.
> >>
> >> I pretty much agree that the current state of things does not fit with
> >> DSI too well.
> >
> > That was why I was questioning how DSI was meant to be implemented in
> > https://lore.kernel.org/dri-devel/CAPY8ntBUKRkSam59Y+72dW_6XOeKVswPWffzPj3uvgE6pV4ZGQ@mail.gmail.com/
> >
> > The need to have the DSI host in a defined idle state (often LP-11,
> > but varying whether the clock lane is in HS) before powering up the
> > panel/bridge is incredibly common, but currently undefined in DRM.
> >
> > Taking the SN65DSI83 as an example, the datasheet [1] section 7.4.2
> > states that the clock lane must be in HS mode, and data lanes in LP-11
> > when coming out of reset. That means that we can't be "enable" as that
> > will have the data lanes in HS mode and sending video, and as we can't
> > be in "pre_enable" as the DSI PHY will be powered down and so we won't
> > have the clock lanes in HS mode.
> >
> > I've hit a similar one with the Toshiba TC358762 where it seems to get
> > upset if it is receiving video data when it gets configured.
> > panel-raspberrypi-touchscreen[2] which drives that chip is
> > intermittent when using panel enable, whereas panel prepare is
> > significantly more reliable but relies on the DSI host being
> > initialised to LP-11 by breaking the chain.
>
> Right
>
> To make it worse, not initing the DSI bridge exactly per spec leads to
> intermittent failures, not consistently occuring ones.

Yes, I suspect it's been just down to timing as to whether the display
side starts producing video data before or after all the configuration
has been sent, and I get random LP commands timing out. (We're only
dropping to LP in vertical blanking, so there isn't a huge amount of
time).

> >    Dave
> >
> > [1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf
> > [2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
>
> Unrelated to this discussion -- there is a tc358762 driver, driver for
> that attiny88 regulator, and driver for the touchscreen chip, on that
> rpi 7" display, in upstream. You can use those to replace the composite
> panel driver (it works at least against stm32mp1 DSI host with the rpi
> 7" panel). Sadly there is little documentation for that attiny88
> protocol or firmware, that's what I don't like about that panel.

Thank you, I know they exist, and I'm looking at exactly that problem
at the moment!

panel-raspberrypi-touchscreen doesn't expose any form of regulator
control, so trying to hook edt-ft54x6 on for the touchscreen sees it
getting the power yanked from under it. I'm trying to switch to those
drivers so that the two play nicely.

The Atmel is a bit nasty in trying to initialise the bridge, panel,
and touch all at the same time. The edt-ft54x6 driver generally probes
first and powers everything up when the DSI host isn't initialised.
This seems to upset the TC358762 and it then won't initialise.
It is possible to poke most things manually through the PORTA, PORTB
and PORTC commands, but I'm currently failing to create a reliable
mechanism :-( I have the advantage that I have the source code for the
Atmel (it's not nice)

  Dave

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Andrzej Hajda]

W dniu 08.09.2021 o 13:11, Dave Stevenson pisze:
> Hi Marek and Andrzej
>
> On Tue, 7 Sept 2021 at 22:24, Marek Vasut <marex@denx.de> wrote:
>> On 9/7/21 7:29 PM, Andrzej Hajda wrote:
>>> W dniu 07.09.2021 o 16:25, Marek Vasut pisze:
>>>> On 9/7/21 9:31 AM, Andrzej Hajda wrote:
>>>>> On 07.09.2021 04:39, Marek Vasut wrote:
>>>>>> In rare cases, the bridge may not start up correctly, which usually
>>>>>> leads to no display output. In case this happens, warn about it in
>>>>>> the kernel log.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>>>> Cc: Robert Foss <robert.foss@linaro.org>
>>>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>> ---
>>>>>> NOTE: See the following:
>>>>>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
>>>>>>
>>>>>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
>>>>>>
>>>>>> ---
>>>>>>      drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
>>>>>>      1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>>>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>>>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
>>>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>>>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct
>>>>>> drm_bridge *bridge,
>>>>>>          /* Clear all errors that got asserted during initialization. */
>>>>>>          regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>>>>>          regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
>>>>>
>>>>> It does not look as correct error handling, maybe it would be good to
>>>>> analyze and optionally report 'unexpected' errors here as well.
>>>> The above is correct -- it clears the status register because the
>>>> setup might've set random bits in that register. Then we wait a bit,
>>>> let the link run, and read them again to get the real link status in
>>>> this new piece of code below, hence the usleep_range there. And then
>>>> if the link indicates a problem, we know it is a problem.
>>>
>>> Usually such registers are cleared on very beginning of the
>>> initialization, and tested (via irq handler, or via reading), during
>>> initalization, if initialization phase goes well. If it is not the case
>>> forgive me.
>> The init just flips the bit at random in the IRQ_STAT register, so no,
>> that's not really viable here. That's why we clear them at the end, and
>> then wait a bit, and then check whether something new appeared in them.
>>
>> If not, all is great.
>>
>> Sure, we could generate an IRQ, but then IRQ line is not always
>> connected to this chip on all hardware I have available. So this gives
>> the user at least some indication that something is wrong with their HW.
>>
>>>>>> +
>>>>>> +    usleep_range(10000, 12000);
>>>>>> +    regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>>>>>> +    if (pval)
>>>>>> +        dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
>>>>>
>>>>> I am not sure what is the case here but it looks like 'we do not know
>>>>> what is going on, so let's add some diagnostic messages to gather info
>>>>> and figure it out later'.
>>>> That's pretty much the case, see the two links above in the NOTE
>>>> section. If something goes wrong, we print the value for the user
>>>> (usually developer) so they can fix their problems. We cannot do much
>>>> better in the attach callback.
>>>>
>>>> The issue I ran into (and where this would be helpful information to
>>>> me during debugging, since the issue happened real seldom, see also
>>>> the NOTE links above) is that the DSI controller driver started
>>>> streaming video on the data lanes before the DSI83 had a chance to
>>>> initialize. This worked most of the time, except for a few exceptions
>>>> here and there, where the video didn't start. This does set link
>>>> status bits consistently. In the meantime, I fixed the controller
>>>> driver (so far downstream, due to ongoing discussion).
>>>
>>> Maybe drm_connector_set_link_status_property(conn,
>>> DRM_MODE_LINK_STATUS_BAD) would be usefule here.
>> Hmm, this works on connector, the dsi83 is a bridge and it can be stuck
>> between two other bridges. That doesn't seem like the right tool, no ?
>>
>>>>> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
>>>>> and I guess it could also help in recovery, but this is just my guess.
>>>>> So if this patch is enough for now you can add:
>>>> No, IRQ won't help you here, because by the time you get the IRQ, the
>>>> DSI host already started streaming video on data lanes and you won't
>>>> be able to correctly reinit the DSI83 unless you communicate to the
>>>> DSI host that it should switch the data lanes back to LP11.
>>>>
>>>> And for that, there is a bigger chunk missing really. What needs to be
>>>> added is a way for the DSI bridge / panel to communicate its needs to
>>>> the DSI host -- things like "I need DSI clock lane frequency f MHz, I
>>>> need clock lane in HS mode and data lanes in LP11 mode". If you look
>>>> at the way DSI hosts and bridges/panels work out the DSI link
>>>> parameters, you will notice they basically do it each on their own,
>>>> there is no such API or communication channel.
>>>
>>> There is one-time communication channel via mipi_dsi_attach, it allows
>>> to set max frequency i HS and LP, choose mode of operation (HS/LPM) and
>>> few more things. If it is necessary to extend it please propse sth.
>> Well, take for example the drivers/gpu/drm/exynos/exynos_drm_dsi.c ,
>> there is this:
>>
>> static void exynos_dsi_enable(struct drm_encoder *encoder)
>> ...
>>                   list_for_each_entry_reverse(iter, &dsi->bridge_chain,
>>                                               chain_node) {
>>                           if (iter->funcs->pre_enable)
>>                                   iter->funcs->pre_enable(iter);
>> ...
>>           exynos_dsi_set_display_mode(dsi);
>>           exynos_dsi_set_display_enable(dsi, true);
>> ...
>>                   list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
>>                           if (iter->funcs->enable)
>>                                   iter->funcs->enable(iter);
>>                   }
>> ...
>>
>> So the bridge enable callback is called with clock lane already in HS
>> mode, and data lanes streaming video. This doesn't work with the DSI83,
>> which would need clock lane in HS and providing clock , but data lanes
>> in LP11 with no video.
>>
>> Sure, I could probably move exynos_dsi_set_display_enable(dsi, true);
>> after the enable call, but is that really the right solution ? What
>> about bridges which need some other specific configuration of the data
>> lanes during init ?
> I hadn't noticed that Exynos was doing that.
> vc4 DSI is doing the same thing in deliberately breaking the
> panel/bridge chain so that it gets a chance to do some initialisation
> before panel/bridge pre_enable.

Initially ExynosDSI was written with panel support only, in such case 
developer can explicitly control time of calling panel ops - and that 
was good.

Later, adding bridge support showed that bridge chain has fixed call 
order which is incompatible with Exynos, so the driver needs to calls 
bridge ops explicitly - flexibility was scarified for simplicity.

For me, fixed order of calls in the whole chain 
(crtc->encoder->bridges...->panel) seems incorrect. Crtc starts 
transmission but the encoder is not yet ready, the same with encoder and 
bridges, later is slightly better - bridges have two ops (pre_enable, 
enable) but since they are not well defined developers are confused what 
should be performed where, as a result we have incompatible approaches.

Only panels have well defined opses: .prepare is for getting panel ready 
for video transmission, .enable is called after starting transmission to 
start showing the image (backlight-on or MIPI_DCS_SET_DISPLAY_ON).

Apparently this model somehow works, probably due to nice hardware and 
custom hacks, but as we see more complicated protocols like DSI or more 
delicate devices cannot be handled with such callbacks.

In case of Exynos DSI and s6e8aa0 panel we need to implement complicated 
sequence, which I have implemented this way:

1. Power on DSI host, start clocks, enable DSI PHY: 
pm_runtime_resume_and_get->exynos_dsi_resume.

2. Power on DSI device: 
drm_panel_prepare->s6e8aa0_prepare->s6e8aa0_power_on.

3. Initialize DSI host: 
drm_panel_prepare->s6e8aa0_prepare->s6e8aa0_set_sequence->...mipi_dsi_device_transfer->...->exynos_dsi_init.

4. Initialize DSI device: 
drm_panel_prepare->s6e8aa0_prepare->s6e8aa0_set_sequence (bulk of MIPI 
DCS/MCS commands).

5. Configure and start video stream on host: 
exynos_dsi_set_display_mode, exynos_dsi_set_display_enable.

6. Show the image: drm_panel_enable


I guess LP-11 state is after DSI host init (3).


>
> Another issue I've noted in doing this is that it breaks calls to the
> bridge's mode_set, mode_valid, and mode_fixup hooks. The framework
> doesn't know about the bridges/panels beyond the encoder, and the
> encoder doesn't get all the information required in order to replicate
> those calls.
If you put such calls into dsi host it will work, this is minus of the 
flexibility - you must do on your own.
> I'm about to look into whether switching the DSI host to being a
> bridge instead of an encoder allows me to overcome that one.
> Doing so doesn't solve the issue of the DSI host bridge pre_enable
> being called after the panel/bridge pre_enable though.


The latter is rather blocking issue, maybe you can overcome it by adding 
mipi_host callbacks: power_on, init - this way you can call them from 
device's pre_enable

This would solve the issues described later.

It seems little bit hacky, but quite easy to implement, what do you think.


Regards

Andrzej


>>> Regarding requesting LP11 I am not sure if we really should have such
>>> low level communication. LP11, as I remember ,is initial state in HS so
>>> it should be set anyway, before starting video transmission.
> LP-11 is the idle LP state. Both P and N lines of the pair are at
> LP-high (~1.2V).
> You then have an escape sequence to shift to HS mode (LP-01 for
> T(lpx), LP-00 for T(hs-prepare), enable HS driver) when you are
> wishing to send data bursts. The signalling levels for HS drop to
> ~100mV for low and ~300mV for high.
>
> Add in ULPS which is effectively powered off but has a specific entry
> and escape sequence to sleep/wake up the receiving device. ULPS seems
> to be totally ignored in DRM as it seems to rely on regulator or other
> control of the panel/bridge to power down.


Nodoby tried to implement it yet, if you want patches are welcome.


>
>> Well, see above, that's the problem I ran across recently.
>>
>>> And maybe this is the main problem:
>>>
>>> DRM core calls:
>>>
>>> crtc->enable
>>>
>>> bridges->pre_enable,
>>>
>>> encoder->enable,
>>>
>>> bridges->enable.
>>>
>>>
>>> Usually video transmission starts in crtc->enable (CRTC->Encoder), and
>>> in encoder->enable (encoder->bridge), so in bridges->enable it would be
>>> too late for LP11 state - transmission can be already in progress.
>>>
>>> It shows well that this order of calls does not fit well to DSI, and
>>> probably many other protocols.
>>>
>>> Maybe moving most of the bridge->enable code to bridge->pre_enable would
>>> help, but I am not sur if it will not pose another issues.
>> Yep, that won't work e.g. with the exynos DSIM, because
>> exynos_dsi_set_display_mode() sets the data lanes to LP11.
> Isn't the bigger question for SN65DSI8[3|4|5] whether the clock lane
> is running or not in pre_enable?
>
>>> This is quick analysis, so please fix me if I am wrong.
>> I pretty much agree that the current state of things does not fit with
>> DSI too well.
> That was why I was questioning how DSI was meant to be implemented in
> https://protect2.fireeye.com/v1/url?k=8518e60b-da83df6e-85196d44-000babff32e3-efd6ff7a2d2163dc&q=1&e=0ab51aa1-fbca-44d3-b10d-56f32a581aa5&u=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2FCAPY8ntBUKRkSam59Y%2B72dW_6XOeKVswPWffzPj3uvgE6pV4ZGQ%40mail.gmail.com%2F
>
> The need to have the DSI host in a defined idle state (often LP-11,
> but varying whether the clock lane is in HS) before powering up the
> panel/bridge is incredibly common, but currently undefined in DRM.
>
> Taking the SN65DSI83 as an example, the datasheet [1] section 7.4.2
> states that the clock lane must be in HS mode, and data lanes in LP-11
> when coming out of reset. That means that we can't be "enable" as that
> will have the data lanes in HS mode and sending video, and as we can't
> be in "pre_enable" as the DSI PHY will be powered down and so we won't
> have the clock lanes in HS mode.
>
> I've hit a similar one with the Toshiba TC358762 where it seems to get
> upset if it is receiving video data when it gets configured.
> panel-raspberrypi-touchscreen[2] which drives that chip is
> intermittent when using panel enable, whereas panel prepare is
> significantly more reliable but relies on the DSI host being
> initialised to LP-11 by breaking the chain.
>
>    Dave
>
> [1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf
> [2] https://protect2.fireeye.com/v1/url?k=2f04bd31-709f8454-2f05367e-000babff32e3-b90973c6593e81b3&q=1&e=0ab51aa1-fbca-44d3-b10d-56f32a581aa5&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fgpu%2Fdrm%2Fpanel%2Fpanel-raspberrypi-touchscreen.c
>

Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge

[Dave Stevenson]

Hi Andrzej

Sorry, I'm just coming back to this. I'd started this reply a while
back, but got sidetracked onto other priorities and not sent it.

On Wed, 8 Sept 2021 at 22:14, Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>
> W dniu 08.09.2021 o 13:11, Dave Stevenson pisze:
> > Hi Marek and Andrzej
> >
> > On Tue, 7 Sept 2021 at 22:24, Marek Vasut <marex@denx.de> wrote:
> >> On 9/7/21 7:29 PM, Andrzej Hajda wrote:
> >>> W dniu 07.09.2021 o 16:25, Marek Vasut pisze:
> >>>> On 9/7/21 9:31 AM, Andrzej Hajda wrote:
> >>>>> On 07.09.2021 04:39, Marek Vasut wrote:
> >>>>>> In rare cases, the bridge may not start up correctly, which usually
> >>>>>> leads to no display output. In case this happens, warn about it in
> >>>>>> the kernel log.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
> >>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
> >>>>>> Cc: Robert Foss <robert.foss@linaro.org>
> >>>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
> >>>>>> Cc: dri-devel@lists.freedesktop.org
> >>>>>> ---
> >>>>>> NOTE: See the following:
> >>>>>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video
> >>>>>>
> >>>>>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533
> >>>>>>
> >>>>>> ---
> >>>>>>      drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 +++++
> >>>>>>      1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>>>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>>>> index a32f70bc68ea4..4ea71d7f0bfbc 100644
> >>>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >>>>>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct
> >>>>>> drm_bridge *bridge,
> >>>>>>          /* Clear all errors that got asserted during initialization. */
> >>>>>>          regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> >>>>>>          regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
> >>>>>
> >>>>> It does not look as correct error handling, maybe it would be good to
> >>>>> analyze and optionally report 'unexpected' errors here as well.
> >>>> The above is correct -- it clears the status register because the
> >>>> setup might've set random bits in that register. Then we wait a bit,
> >>>> let the link run, and read them again to get the real link status in
> >>>> this new piece of code below, hence the usleep_range there. And then
> >>>> if the link indicates a problem, we know it is a problem.
> >>>
> >>> Usually such registers are cleared on very beginning of the
> >>> initialization, and tested (via irq handler, or via reading), during
> >>> initalization, if initialization phase goes well. If it is not the case
> >>> forgive me.
> >> The init just flips the bit at random in the IRQ_STAT register, so no,
> >> that's not really viable here. That's why we clear them at the end, and
> >> then wait a bit, and then check whether something new appeared in them.
> >>
> >> If not, all is great.
> >>
> >> Sure, we could generate an IRQ, but then IRQ line is not always
> >> connected to this chip on all hardware I have available. So this gives
> >> the user at least some indication that something is wrong with their HW.
> >>
> >>>>>> +
> >>>>>> +    usleep_range(10000, 12000);
> >>>>>> +    regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> >>>>>> +    if (pval)
> >>>>>> +        dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
> >>>>>
> >>>>> I am not sure what is the case here but it looks like 'we do not know
> >>>>> what is going on, so let's add some diagnostic messages to gather info
> >>>>> and figure it out later'.
> >>>> That's pretty much the case, see the two links above in the NOTE
> >>>> section. If something goes wrong, we print the value for the user
> >>>> (usually developer) so they can fix their problems. We cannot do much
> >>>> better in the attach callback.
> >>>>
> >>>> The issue I ran into (and where this would be helpful information to
> >>>> me during debugging, since the issue happened real seldom, see also
> >>>> the NOTE links above) is that the DSI controller driver started
> >>>> streaming video on the data lanes before the DSI83 had a chance to
> >>>> initialize. This worked most of the time, except for a few exceptions
> >>>> here and there, where the video didn't start. This does set link
> >>>> status bits consistently. In the meantime, I fixed the controller
> >>>> driver (so far downstream, due to ongoing discussion).
> >>>
> >>> Maybe drm_connector_set_link_status_property(conn,
> >>> DRM_MODE_LINK_STATUS_BAD) would be usefule here.
> >> Hmm, this works on connector, the dsi83 is a bridge and it can be stuck
> >> between two other bridges. That doesn't seem like the right tool, no ?
> >>
> >>>>> Whole driver lacks IRQ handler which IMO could perform better diagnosis,
> >>>>> and I guess it could also help in recovery, but this is just my guess.
> >>>>> So if this patch is enough for now you can add:
> >>>> No, IRQ won't help you here, because by the time you get the IRQ, the
> >>>> DSI host already started streaming video on data lanes and you won't
> >>>> be able to correctly reinit the DSI83 unless you communicate to the
> >>>> DSI host that it should switch the data lanes back to LP11.
> >>>>
> >>>> And for that, there is a bigger chunk missing really. What needs to be
> >>>> added is a way for the DSI bridge / panel to communicate its needs to
> >>>> the DSI host -- things like "I need DSI clock lane frequency f MHz, I
> >>>> need clock lane in HS mode and data lanes in LP11 mode". If you look
> >>>> at the way DSI hosts and bridges/panels work out the DSI link
> >>>> parameters, you will notice they basically do it each on their own,
> >>>> there is no such API or communication channel.
> >>>
> >>> There is one-time communication channel via mipi_dsi_attach, it allows
> >>> to set max frequency i HS and LP, choose mode of operation (HS/LPM) and
> >>> few more things. If it is necessary to extend it please propse sth.
> >> Well, take for example the drivers/gpu/drm/exynos/exynos_drm_dsi.c ,
> >> there is this:
> >>
> >> static void exynos_dsi_enable(struct drm_encoder *encoder)
> >> ...
> >>                   list_for_each_entry_reverse(iter, &dsi->bridge_chain,
> >>                                               chain_node) {
> >>                           if (iter->funcs->pre_enable)
> >>                                   iter->funcs->pre_enable(iter);
> >> ...
> >>           exynos_dsi_set_display_mode(dsi);
> >>           exynos_dsi_set_display_enable(dsi, true);
> >> ...
> >>                   list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
> >>                           if (iter->funcs->enable)
> >>                                   iter->funcs->enable(iter);
> >>                   }
> >> ...
> >>
> >> So the bridge enable callback is called with clock lane already in HS
> >> mode, and data lanes streaming video. This doesn't work with the DSI83,
> >> which would need clock lane in HS and providing clock , but data lanes
> >> in LP11 with no video.
> >>
> >> Sure, I could probably move exynos_dsi_set_display_enable(dsi, true);
> >> after the enable call, but is that really the right solution ? What
> >> about bridges which need some other specific configuration of the data
> >> lanes during init ?
> > I hadn't noticed that Exynos was doing that.
> > vc4 DSI is doing the same thing in deliberately breaking the
> > panel/bridge chain so that it gets a chance to do some initialisation
> > before panel/bridge pre_enable.
>
> Initially ExynosDSI was written with panel support only, in such case
> developer can explicitly control time of calling panel ops - and that
> was good.
>
> Later, adding bridge support showed that bridge chain has fixed call
> order which is incompatible with Exynos, so the driver needs to calls
> bridge ops explicitly - flexibility was scarified for simplicity.
>
> For me, fixed order of calls in the whole chain
> (crtc->encoder->bridges...->panel) seems incorrect. Crtc starts
> transmission but the encoder is not yet ready, the same with encoder and
> bridges, later is slightly better - bridges have two ops (pre_enable,
> enable) but since they are not well defined developers are confused what
> should be performed where, as a result we have incompatible approaches.

I can't comment on how other platforms work, but on the Broadcom chips
the clock to read pixel data out of the pipeline comes from the
encoder, whether that is HDMI, DSI, DPI, or other. Therefore until the
encoder is enabled no data actually flows.

I'm not in a position to discuss whether the ordering is correct or
not - there are many others who know the subsystem far better than me
in that regard.

> Only panels have well defined opses: .prepare is for getting panel ready
> for video transmission, .enable is called after starting transmission to
> start showing the image (backlight-on or MIPI_DCS_SET_DISPLAY_ON).
>
> Apparently this model somehow works, probably due to nice hardware and
> custom hacks, but as we see more complicated protocols like DSI or more
> delicate devices cannot be handled with such callbacks.
>
> In case of Exynos DSI and s6e8aa0 panel we need to implement complicated
> sequence, which I have implemented this way:
>
> 1. Power on DSI host, start clocks, enable DSI PHY:
> pm_runtime_resume_and_get->exynos_dsi_resume.
>
> 2. Power on DSI device:
> drm_panel_prepare->s6e8aa0_prepare->s6e8aa0_power_on.
>
> 3. Initialize DSI host:
> drm_panel_prepare->s6e8aa0_prepare->s6e8aa0_set_sequence->...mipi_dsi_device_transfer->...->exynos_dsi_init.
>
> 4. Initialize DSI device:
> drm_panel_prepare->s6e8aa0_prepare->s6e8aa0_set_sequence (bulk of MIPI
> DCS/MCS commands).
>
> 5. Configure and start video stream on host:
> exynos_dsi_set_display_mode, exynos_dsi_set_display_enable.
>
> 6. Show the image: drm_panel_enable
>
>
> I guess LP-11 state is after DSI host init (3).

Possibly, but seeing as you have phy_power_on called from
exynos_dsi_resume in step 1 that's also possibly setting LP-11, but
may be ULPS (LP-00).

Then again exynos_dsi_init_link called from exynos_dsi_init configures
which lanes are required, and it would seem a bit odd to power up
lanes that weren't wanted.
Oddly that only seems to be called from exynos_dsi_host_transfer (your
step 3), so that'll never be called for something like the SN65DSI83
where it's configured via I2C rather than MIPI commands.

> >
> > Another issue I've noted in doing this is that it breaks calls to the
> > bridge's mode_set, mode_valid, and mode_fixup hooks. The framework
> > doesn't know about the bridges/panels beyond the encoder, and the
> > encoder doesn't get all the information required in order to replicate
> > those calls.
> If you put such calls into dsi host it will work, this is minus of the
> flexibility - you must do on your own.

You can't for mode_valid - the drm_bridge_funcs version has a "const
struct drm_display_info *info" parameter which the
drm_encoder_helper_funcs version doesn't have.

> > I'm about to look into whether switching the DSI host to being a
> > bridge instead of an encoder allows me to overcome that one.
> > Doing so doesn't solve the issue of the DSI host bridge pre_enable
> > being called after the panel/bridge pre_enable though.

I've just been working through converting our driver to being a bridge
as the current intended way of modelling DSI, and so that we can get
access to the relevant parameters to be able to implement mode_valid
and call down the chain.

It largely works, except for drm_atomic_add_encoder_bridges adds state
for all the bridges that the framework believes are attached to the
encoder. That stops at our DSI encoder/bridge as we've split the
chain, and there doesn't seem to be a simple way to replicate the
effect down the split section. AFAICT we can't replicate that from our
atomic_duplicate_state as we don't get given the full state to add our
state to.

Keeping this split chain approach seems flawed, so I'll look at either
a pre_pre_enable, or a phy state DSI call.

> The latter is rather blocking issue, maybe you can overcome it by adding
> mipi_host callbacks: power_on, init - this way you can call them from
> device's pre_enable
>
> This would solve the issues described later.
>
> It seems little bit hacky, but quite easy to implement, what do you think.

I started a discussion back in July as to whether a new function was
sensible[1]. Laurent's just resurrected it, and you've posted to it
too, so I'll shift the discussion there.

  Dave

[1] https://lists.freedesktop.org/archives/dri-devel/2021-July/313576.html