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

[Marek Szyprowski]

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 at 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 at 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).


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