On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf
<frieder.schrempf@xxxxxxxxxx> wrote:
>
> 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@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.
Thanks,
Jagan.
