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

[Marek Vasut]

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

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 ?