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

[Jagan Teki <jagan@amarulasolutions.com>]

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?

On the other side, MEDIA_BUS_FMT_RGB888_1X7X4_SPWG is 24-bit samples
transferred over an LVDS bus with four differential data pairs,
serialized into 7-time slots using SPWG which indeed a
MEDIA_BUS_FMT_RGB888_1X24 input_fmt for the bridge. so handling in the
supported list and reassigning the RGB888_1X24 would be the proper way
to handle this format.

Jagan.