Hi Tomi,
On 17/07/25 15:59, Tomi Valkeinen wrote:
> Hi,
>
> On 17/07/2025 12:36, Devarsh Thakkar wrote:
>> Hi Tomi
>>
>> Thanks for the patch.
>>
>> On 18/06/25 15:29, Tomi Valkeinen wrote:
>>> While the cdns-dsi does not support DSI burst mode, the burst mode is
>>> essentially DSI event mode with more versatile clocking and timings.
>> I don't fully agree with this statement, DSI burst mode and DSI event
>> mode are two different things having separate requirements. DSI burst
>> mode maps to MIPI_DSI_MODE_VIDEO_BURST. I don't see a separate flag for
>> event mode but I guess,
>
> Well, what does DSI burst mode mean? Signal-wise it's the same as DSI
> event mode. "burst" just means that the DSI TX is allowed to send the
> data much faster than the pixel clock. But there's no strict requirement
> that it _must_ be faster.
>
> So, DSI burst mode is basically DSI event mode with more freedom on the
> timings. Thus, afaics, DSI TX in DSI event mode should always work if
> the DSI RX expects DSI burst mode.
>
Yes this is true, although there are subtle differences between event
mode and burst mode and I guess that's why DSI specification 8.11.1
Transmission Packet Sequences mentions for video mode lists DSI event
mode sequence as differently then burst mode w.r.t packet sequence viz
listing 3 different sequences i.e. non-burst with sync pulse, non-burst
with sync event and burst mode. But seems like we do not have 3 separate
flags for each of those.
And I see most drivers using MIPI_DSI_MODE_VIDEO both in context of
selecting video mode as opposed to command mode and also subtly
inferring it as event mode in case MIPI_DSI_MODE_VIDEO_BURST and
MIPI_DSI_MODE_VIDEO_PULSE are not set. So there is no accurate way to
differentiate between the three or for the bridge driver to enforce
burst mode over event mode and I guess decision is left to host driver.
So, I think this patch looks fine considering these aspects.
>>> Thus cdns-dsi doesn't need to fail if the DSI peripheral driver requests
>>> MIPI_DSI_MODE_VIDEO_BURST.
>>
>> MIPI_DSI_MODE_VIDEO_BURST is currently not supported by the cadence DSI
>> host driver, so only if DSI peripheral driver is saying that burst mode
>> is the only one it supports in that case only we should fail.
>>
>>>
>>> In my particular use case, this allows the use of ti-sn65dsi83 driver.
>>>
>>> Tested-by: Parth Pancholi <parth.pancholi@toradex.com>
>>> Tested-by: Jayesh Choudhary <j-choudhary@ti.com> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>
Regards
Devarsh
>>> ---
>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 4 ----
>>> 1 file changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/
>>> gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> index 114d883c65dc..09b289f0fcbf 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> @@ -1052,10 +1052,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host
>>> *host,
>>> if (output->dev)
>>> return -EBUSY;
>>> - /* We do not support burst mode yet. */
>>> - if (dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
>>> - return -ENOTSUPP;
>>> -
>>
>> Removing this check also gives a false impression that burst mode is
>> supported by the driver and can also lead to failures too in case device
>> is only supporting burst mode.
>>
>> I think it makes sense to fail only if burst mode is the only one being
>> supported by the device, something like below should work I believe,
>>
>> if (dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST ==
>> MIPI_DSI_MODE_VIDEO_BURST)
>> return -ENOTSUPP;
>>
>> Regards
>> Devarsh
>>
>>> /*
>>> * The host <-> device link might be described using an OF-graph
>>> * representation, in this case we extract the device of_node from
>>>
>