Hi Jagan,
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?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
