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

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

Hi Jagan,

On 15.11.2022 10:20, Jagan Teki wrote:
> On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>> On 15.11.22 09:09, Marek Szyprowski wrote:
>>> On 14.11.2022 18:07, Jagan Teki wrote:
>>>> On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
>>>> <m.szyprowski@samsung.com> wrote:
>>>>> On 14.11.2022 11:57, Marek Szyprowski wrote:
>>>>>> On 10.11.2022 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[] = {
>>>>>>> +    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;
>>>>>> Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
>>>>>> MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
>>>>>>
>>>>>> Otherwise the above check breaks all current clients of the Samsung
>>>>>> DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
>>>>>> DSI panels requests such format on my test systems...
>>>>> I've checked a bit more the bus format related code and it looks that
>>>>> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
>>>>> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
>>>>> them. On Arndale board with Toshiba tc358764 bridge the
>>>>> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
>>>>> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,
>>>> dsim => tc358764 => panel-simple
>>>>
>>>> If I understand the bridge format negotiation correctly - If
>>>> atomic_get_input_bus_fmts is not implemented in tc358764 then
>>>> MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
>>>> MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.
>>>>
>>>> from include/drm/drm_bridge.h:
>>>>
>>>>            * This method is called on all elements of the bridge chain as part of
>>>>            * the bus format negotiation process that happens in
>>>>            * drm_atomic_bridge_chain_select_bus_fmts().
>>>>            * This method is optional. When not implemented, the core will bypass
>>>>            * bus format negotiation on this element of the bridge without
>>>>            * failing, and the previous element in the chain will be passed
>>>>            * MEDIA_BUS_FMT_FIXED as its output bus format.
>>>>
>>>> As I can see tc358764 is not implemented either
>>>> atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
>>>> MEDIA_BUS_FMT_FIXED bridge pipeline. I have tested sn65dsi without
>>>> atomic_get_input_bus_fmts I can see the dsim is getting
>>>> MEDIA_BUS_FMT_FIXED.
>>>>
>>>> Can you check the same from your side?
>>> Here in case of Arndale 5250 with the following pipeline:
>>>
>>> dsim => tc358764 => panel-simple (boe,hv070wsa-100 panel)
>>>
>>> the DRM core requests MEDIA_BUS_FMT_RGB888_1X7X4_SPWG format, taken from the boe_hv070wsa panel (see from drivers/gpu/drm/panel/panel-simple.c). Please note that in case of Exynos, the reversed bridge chain order is used for dsim, so this is another nasty consequence of that hack. :/
>>>
>>> Maybe if no compatible bus format is found, the driver should force
>>> MEDIA_BUS_FMT_RGB888_1X24 until a proper format negotiation is
>>> implemented and hacks removed?
>> For this specific case, wouldn't it be better to just fix the format
>> negotiation for tc358764 using atomic_get_input_bus_fmts()? It should
>> probably do the same as sn65dsi83 and request MEDIA_BUS_FMT_RGB888_1X24
>> from the DSI.
>>
>> Forwarding the LVDS-specific format to the input is obviously the wrong
>> thing for the tc358764 driver to do.
>>
>> But I agree, if there are other problematic pipelines with other
>> bridge/display drivers that don't pass a correct format, we should
>> accept them for now and fall back to a sane default
>> (MEDIA_BUS_FMT_RGB888_1X24) and fix the other drivers afterwards.
>>
>> Let's not delay this series any further and better work on how to get it
>> merged before we miss another merge window.
> Agreed this point, here is the updated diff (on top of this patch) for
> falling back to the default format for those.
>
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1387,6 +1387,8 @@ static const u32 samsung_dsim_pixel_output_fmts[] = {
>          MEDIA_BUS_FMT_RGB565_1X16,
>          MEDIA_BUS_FMT_RGB666_1X18,
>          MEDIA_BUS_FMT_RGB888_1X24,
> +
> +       MEDIA_BUS_FMT_FIXED,
>   };
>
>   static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> @@ -1412,7 +1414,13 @@ samsung_dsim_atomic_get_input_bus_fmts(struct
> drm_bridge *bridge,
>          u32 *input_fmts;
>
>          if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> -               return NULL;
> +               /*
> +                * Some bridge/display drivers are still not able to pass
> +                * the correct format, so handle those pipelines by falling
> +                * back to the default format till the supported format list
> +                * gets finalized.
> +                */
> +               output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
>
>          *num_input_fmts = 1;
>
> @@ -1420,7 +1428,14 @@ samsung_dsim_atomic_get_input_bus_fmts(struct
> drm_bridge *bridge,
>          if (!input_fmts)
>                  return NULL;
>
> -       input_fmts[0] = output_fmt;
> +       switch (output_fmt) {
> +       case MEDIA_BUS_FMT_FIXED:
> +               input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +               break;
> +       default:
> +               input_fmts[0] = output_fmt;
> +               break;
> +       }
>
>          return input_fmts;
>
> If all is okay, I will send v9 which would probably test it for the
> final version to merge.

This works for me (I've manually applied the above changes, due to 
malformed diff).

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

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