Hi Jagan,
On 15.11.2022 10:20, Jagan Teki wrote:
> On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf
> <frieder.schrempf@xxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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@xxxxxxxxxxx>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
