Re: [PATCH v8 09/14] drm: bridge: samsung-dsim: Add atomic_get_input_bus_fmts

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 16.11.2022 11:49, Marek Vasut wrote:
> On 11/16/22 09:07, Marek Szyprowski wrote:
>> On 15.11.2022 13:00, Marek Vasut wrote:
>>> On 11/14/22 08:49, Jagan Teki wrote:
>>>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 11/10/22 19:38, Jagan Teki wrote:
>>>>>> Finding the right input bus format throughout the pipeline is hard
>>>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>>>> proper input format from list of supported output formats.
>>>>>>
>>>>>> This format can be used in pipeline for negotiating bus format 
>>>>>> between
>>>>>> the DSI-end of this bridge and the other component closer to 
>>>>>> pipeline
>>>>>> components.
>>>>>>
>>>>>> List of Pixel formats are taken from,
>>>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>>> 3.7.4 Pixel formats
>>>>>> Table 14. DSI pixel packing formats
>>>>>>
>>>>>> v8:
>>>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI
>>>>>> DSI/CSI-2
>>>>>>
>>>>>> v7, v6, v5, v4:
>>>>>> * none
>>>>>>
>>>>>> v3:
>>>>>> * include media-bus-format.h
>>>>>>
>>>>>> v2:
>>>>>> * none
>>>>>>
>>>>>> v1:
>>>>>> * new patch
>>>>>>
>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/bridge/samsung-dsim.c | 53
>>>>>> +++++++++++++++++++++++++++
>>>>>>     1 file changed, 53 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>     #include <linux/clk.h>
>>>>>>     #include <linux/delay.h>
>>>>>>     #include <linux/irq.h>
>>>>>> +#include <linux/media-bus-format.h>
>>>>>>     #include <linux/of_device.h>
>>>>>>     #include <linux/phy/phy.h>
>>>>>>
>>>>>> @@ -1321,6 +1322,57 @@ static void
>>>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>>>         pm_runtime_put_sync(dsi->dev);
>>>>>>     }
>>>>>>
>>>>>> +/*
>>>>>> + * This pixel output formats list referenced from,
>>>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>>> + * 3.7.4 Pixel formats
>>>>>> + * Table 14. DSI pixel packing formats
>>>>>> + */
>>>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>>>
>>>>> You can also add :
>>>>>
>>>>> MEDIA_BUS_FMT_YUYV10_1X20
>>>>>
>>>>> MEDIA_BUS_FMT_YUYV12_1X24
>>>>
>>>> Are these for the below formats?
>>>>
>>>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
>>>>    Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
>>>>>
>>>>>> +     MEDIA_BUS_FMT_UYVY8_1X16,
>>>>>> +     MEDIA_BUS_FMT_RGB101010_1X30,
>>>>>> +     MEDIA_BUS_FMT_RGB121212_1X36,
>>>>>> +     MEDIA_BUS_FMT_RGB565_1X16,
>>>>>> +     MEDIA_BUS_FMT_RGB666_1X18,
>>>>>> +     MEDIA_BUS_FMT_RGB888_1X24,
>>>>>> +};
>>>>>> +
>>>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>>>> +{
>>>>>> +     int i;
>>>>>> +
>>>>>> +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts);
>>>>>> i++) {
>>>>>> +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>>>> +                     return true;
>>>>>> +     }
>>>>>> +
>>>>>> +     return false;
>>>>>> +}
>>>>>> +
>>>>>> +static u32 *
>>>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>>>> +                                    struct drm_bridge_state
>>>>>> *bridge_state,
>>>>>> +                                    struct drm_crtc_state
>>>>>> *crtc_state,
>>>>>> +                                    struct drm_connector_state
>>>>>> *conn_state,
>>>>>> +                                    u32 output_fmt,
>>>>>> +                                    unsigned int *num_input_fmts)
>>>>>> +{
>>>>>> +     u32 *input_fmts;
>>>>>> +
>>>>>> +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>>>> +             return NULL;
>>>>>> +
>>>>>> +     *num_input_fmts = 1;
>>>>>
>>>>> Shouldn't this be 6 ?
>>>>>
>>>>>> +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>>>>>> +     if (!input_fmts)
>>>>>> +             return NULL;
>>>>>> +
>>>>>> +     input_fmts[0] = output_fmt;
>>>>>
>>>>> Shouldn't this be a list of all 6 supported pixel formats ?
>>>>
>>>> Negotiation would settle to return one input_fmt from the list of
>>>> supporting output_fmts. so the num_input_fmts would be 1 rather than
>>>> the number of fmts in the supporting list. This is what I understood
>>>> from the atomic_get_input_bus_fmts API. let me know if I miss
>>>> something here.
>>>
>>> How does the negotiation work for this kind of pipeline:
>>>
>>> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector
>>>
>>> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either
>>> RGB888 or packed YUV422 ?
>>>
>>> Who decides the format used by such pipeline ?
>>>
>>> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the
>>> LCDIFv3 ?
>>
>>
>> If I got it right, the drivers reports their preference for the returned
>> formats. The format that is first in the reported array is the most
>> preferred one. This probably means that in your case the HDMI bridge
>> decides by reporting RGB or YUV first (if all elements supports both).
>
> But in that case, we need to list input_fmts[0]...input_fmts[n-1] in 
> samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts 
> = n, correct ?
>
Yes, if I got the bus format negotiation code right, but I didn't check 
the details.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland