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

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

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, 
but this doesn't look like a format really needed on that bridge). A bit 
more logic seems to be needed there to make it working properly.

I suggest to move all this bus format related changes into a separate 
patchset and adjust other bridges/panels/etc as needed in it.

>
>> +
>> +    *num_input_fmts = 1;
>> +
>> +    input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>> +    if (!input_fmts)
>> +        return NULL;
>> +
>> +    input_fmts[0] = output_fmt;
>> +
>> +    return input_fmts;
>> +}
>> +
>>   static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>>                        struct drm_bridge_state *bridge_state,
>>                        struct drm_crtc_state *crtc_state,
>> @@ -1385,6 +1437,7 @@ static const struct drm_bridge_funcs 
>> samsung_dsim_bridge_funcs = {
>>       .atomic_duplicate_state        = 
>> drm_atomic_helper_bridge_duplicate_state,
>>       .atomic_destroy_state        = 
>> drm_atomic_helper_bridge_destroy_state,
>>       .atomic_reset            = drm_atomic_helper_bridge_reset,
>> +    .atomic_get_input_bus_fmts    = 
>> samsung_dsim_atomic_get_input_bus_fmts,
>>       .atomic_check            = samsung_dsim_atomic_check,
>>       .atomic_pre_enable        = samsung_dsim_atomic_pre_enable,
>>       .atomic_enable            = samsung_dsim_atomic_enable,
>
> Best regards

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