drm/bridge: ti-sn65dsi86: Add necessary DSI flags

[PATCH] drm/bridge: ti-sn65dsi86: Add necessary DSI flags

Posted by Jayesh Choudhary 10 months ago

Enable NO_EOT and SYNC flags for DSI to use VIDEO_SYNC_PULSE_MODE
with EOT disabled.

Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f72675766e01..8e9a7eb927da 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -707,7 +707,8 @@ static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86
 	/* TODO: setting to 4 MIPI lanes always for now */
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET |
+			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
 
 	/* check if continuous dsi clock is required or not */
 	pm_runtime_get_sync(dev);
-- 
2.34.1

Re: [PATCH] drm/bridge: ti-sn65dsi86: Add necessary DSI flags

Posted by Doug Anderson 10 months ago

Hi,

On Fri, Apr 11, 2025 at 2:23 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> Enable NO_EOT and SYNC flags for DSI to use VIDEO_SYNC_PULSE_MODE
> with EOT disabled.

Any chance you could add some details to this commit message? Your
subject says that these flags are "necessary", but people have been
using this driver successfully for many years now. Why did these flags
suddenly become necessary and why were things working before?

I'm not saying that we shouldn't use these flags, just trying to
understand. I actually don't know a ton about these details in MIPI,
so it would help me :-).

> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f72675766e01..8e9a7eb927da 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -707,7 +707,8 @@ static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86
>         /* TODO: setting to 4 MIPI lanes always for now */
>         dsi->lanes = 4;
>         dsi->format = MIPI_DSI_FMT_RGB888;
> -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET |
> +                         MIPI_DSI_MODE_VIDEO_SYNC_PULSE;

FWIW, I can confirm that on my board the screen still seems to light
up after this change. ...so I'd be OK w/

Tested-by: Douglas Anderson <dianders@chromium.org>

...before giving a Reviewed-by I'd want a description that helps me
understand it better.

-Doug

Re: [PATCH] drm/bridge: ti-sn65dsi86: Add necessary DSI flags

Posted by Jayesh Choudhary 9 months, 3 weeks ago

Hello Doug,

On 13/04/25 07:22, Doug Anderson wrote:
> Hi,
> 
> On Fri, Apr 11, 2025 at 2:23 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> Enable NO_EOT and SYNC flags for DSI to use VIDEO_SYNC_PULSE_MODE
>> with EOT disabled.
> 
> Any chance you could add some details to this commit message? Your
> subject says that these flags are "necessary", but people have been
> using this driver successfully for many years now. Why did these flags
> suddenly become necessary and why were things working before?
> 
> I'm not saying that we shouldn't use these flags, just trying to
> understand. I actually don't know a ton about these details in MIPI,
> so it would help me :-).
> 

Definitely.
I will add more details for the commit message.

For more context here, I was working with cadence dsi driver for TI
SoCs. So to be more accurate, this is required for CDNS_DSI

I observed other bridges like lt-9211, where I have seen such flags
being set for dsi-controller by vendors.

> 
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index f72675766e01..8e9a7eb927da 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -707,7 +707,8 @@ static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86
>>          /* TODO: setting to 4 MIPI lanes always for now */
>>          dsi->lanes = 4;
>>          dsi->format = MIPI_DSI_FMT_RGB888;
>> -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
>> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET |
>> +                         MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> 
> FWIW, I can confirm that on my board the screen still seems to light
> up after this change. ...so I'd be OK w/
> 
> Tested-by: Douglas Anderson <dianders@chromium.org>
> 
> ...before giving a Reviewed-by I'd want a description that helps me
> understand it better.

> 
> -Doug

Re: [PATCH] drm/bridge: ti-sn65dsi86: Add necessary DSI flags

Posted by Jayesh Choudhary 9 months, 2 weeks ago

Hello Doug,

On 17/04/25 02:40, Jayesh Choudhary wrote:
> Hello Doug,
> 
> On 13/04/25 07:22, Doug Anderson wrote:
>> Hi,
>>
>> On Fri, Apr 11, 2025 at 2:23 AM Jayesh Choudhary <j-choudhary@ti.com> 
>> wrote:
>>>
>>> Enable NO_EOT and SYNC flags for DSI to use VIDEO_SYNC_PULSE_MODE
>>> with EOT disabled.
>>
>> Any chance you could add some details to this commit message? Your
>> subject says that these flags are "necessary", but people have been
>> using this driver successfully for many years now. Why did these flags
>> suddenly become necessary and why were things working before?
>>
>> I'm not saying that we shouldn't use these flags, just trying to
>> understand. I actually don't know a ton about these details in MIPI,
>> so it would help me :-).
>>
> 
> Definitely.
> I will add more details for the commit message.
> 
> For more context here, I was working with cadence dsi driver for TI
> SoCs. So to be more accurate, this is required for CDNS_DSI
> 
> I observed other bridges like lt-9211, where I have seen such flags
> being set for dsi-controller by vendors.

Upon testing with modes other than video sync pulse mode,
I found that with the upcoming fixes in cdns-dsi-core driver made by
Tomi[0], this patch is no longer necessary.

[0]: 
https://lore.kernel.org/all/20250414-cdns-dsi-impro-v3-0-4e52551d4f07@ideasonboard.com/

I apologies for the noise in the mailing list.

Warm Regards,
Jayesh

> 
>>
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>> ---
>>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
>>> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>> index f72675766e01..8e9a7eb927da 100644
>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>> @@ -707,7 +707,8 @@ static int ti_sn_attach_host(struct 
>>> auxiliary_device *adev, struct ti_sn65dsi86
>>>          /* TODO: setting to 4 MIPI lanes always for now */
>>>          dsi->lanes = 4;
>>>          dsi->format = MIPI_DSI_FMT_RGB888;
>>> -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
>>> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
>>> MIPI_DSI_MODE_NO_EOT_PACKET |
>>> +                         MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>>
>> FWIW, I can confirm that on my board the screen still seems to light
>> up after this change. ...so I'd be OK w/
>>
>> Tested-by: Douglas Anderson <dianders@chromium.org>
>>
>> ...before giving a Reviewed-by I'd want a description that helps me
>> understand it better.
> 
>>
>> -Doug

Re: [PATCH] drm/bridge: ti-sn65dsi86: Add necessary DSI flags

Posted by Doug Anderson 9 months, 2 weeks ago

Hi,

On Thu, Apr 24, 2025 at 3:47 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> Hello Doug,
>
> On 17/04/25 02:40, Jayesh Choudhary wrote:
> > Hello Doug,
> >
> > On 13/04/25 07:22, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Fri, Apr 11, 2025 at 2:23 AM Jayesh Choudhary <j-choudhary@ti.com>
> >> wrote:
> >>>
> >>> Enable NO_EOT and SYNC flags for DSI to use VIDEO_SYNC_PULSE_MODE
> >>> with EOT disabled.
> >>
> >> Any chance you could add some details to this commit message? Your
> >> subject says that these flags are "necessary", but people have been
> >> using this driver successfully for many years now. Why did these flags
> >> suddenly become necessary and why were things working before?
> >>
> >> I'm not saying that we shouldn't use these flags, just trying to
> >> understand. I actually don't know a ton about these details in MIPI,
> >> so it would help me :-).
> >>
> >
> > Definitely.
> > I will add more details for the commit message.
> >
> > For more context here, I was working with cadence dsi driver for TI
> > SoCs. So to be more accurate, this is required for CDNS_DSI
> >
> > I observed other bridges like lt-9211, where I have seen such flags
> > being set for dsi-controller by vendors.
>
> Upon testing with modes other than video sync pulse mode,
> I found that with the upcoming fixes in cdns-dsi-core driver made by
> Tomi[0], this patch is no longer necessary.
>
> [0]:
> https://lore.kernel.org/all/20250414-cdns-dsi-impro-v3-0-4e52551d4f07@ideasonboard.com/
>
> I apologies for the noise in the mailing list.

No worries. If folks are convinced that adding these flags makes
things "more correct" I still don't have an objection to landing the
patch, though the commit message would still need to be clear about
why these flags make things more correct. I'll leave it up to you. ;-)

-Doug