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

Tue Nov 15 00:48:18 PST 2022

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 at 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 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[] = {
>>>>> +    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.