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

[Jagan Teki <jagan@amarulasolutions.com>]

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:
> > Hi Jagan,
> >
> > 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.

Thanks,
Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel