| Message ID | 20221110183853.3678209-10-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: bridge: Add Samsung MIPI DSIM bridge | expand |
On 11/10/22 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[] = { You can also add : MEDIA_BUS_FMT_YUYV10_1X20 MEDIA_BUS_FMT_YUYV12_1X24 > + 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; > + > + *num_input_fmts = 1; Shouldn't this be 6 ? > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + input_fmts[0] = output_fmt; Shouldn't this be a list of all 6 supported pixel formats ? (replace 6 with 8 with the two new YUYV10/YUYV12 pixel formats above)
On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote: > > On 11/10/22 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[] = { > > You can also add : > > MEDIA_BUS_FMT_YUYV10_1X20 > > MEDIA_BUS_FMT_YUYV12_1X24 Are these for the below formats? "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2 Packed Pixel Stream, 24-bit YCbCr, 4:2:2" > > > + 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; > > + > > + *num_input_fmts = 1; > > Shouldn't this be 6 ? > > > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > + if (!input_fmts) > > + return NULL; > > + > > + input_fmts[0] = output_fmt; > > Shouldn't this be a list of all 6 supported pixel formats ? Negotiation would settle to return one input_fmt from the list of supporting output_fmts. so the num_input_fmts would be 1 rather than the number of fmts in the supporting list. This is what I understood from the atomic_get_input_bus_fmts API. let me know if I miss something here. Jagan.
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... > + > + *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
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
On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski <m.szyprowski@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@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? On the other side, MEDIA_BUS_FMT_RGB888_1X7X4_SPWG is 24-bit samples transferred over an LVDS bus with four differential data pairs, serialized into 7-time slots using SPWG which indeed a MEDIA_BUS_FMT_RGB888_1X24 input_fmt for the bridge. so handling in the supported list and reassigning the RGB888_1X24 would be the proper way to handle this format. Jagan.
Hi Jagan, On 14.11.2022 18:07, Jagan Teki wrote: > On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski > <m.szyprowski@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@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? Best regards
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@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@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.
On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf <frieder.schrempf@kontron.de> 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@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@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. 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.
On 11/14/22 08:49, Jagan Teki wrote: > On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote: >> >> On 11/10/22 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[] = { >> >> You can also add : >> >> MEDIA_BUS_FMT_YUYV10_1X20 >> >> MEDIA_BUS_FMT_YUYV12_1X24 > > Are these for the below formats? > > "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2 > Packed Pixel Stream, 24-bit YCbCr, 4:2:2" >> >>> + 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; >>> + >>> + *num_input_fmts = 1; >> >> Shouldn't this be 6 ? >> >>> + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); >>> + if (!input_fmts) >>> + return NULL; >>> + >>> + input_fmts[0] = output_fmt; >> >> Shouldn't this be a list of all 6 supported pixel formats ? > > Negotiation would settle to return one input_fmt from the list of > supporting output_fmts. so the num_input_fmts would be 1 rather than > the number of fmts in the supporting list. This is what I understood > from the atomic_get_input_bus_fmts API. let me know if I miss > something here. How does the negotiation work for this kind of pipeline: LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector where all elements (LCDIFv3, DSIM, HDMI bridge) can support either RGB888 or packed YUV422 ? Who decides the format used by such pipeline ? Why should it be the DSIM bridge and not e.g. the HDMI bridge or the LCDIFv3 ?
Hi Jagan, On 15.11.2022 10:20, Jagan Teki wrote: > On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf > <frieder.schrempf@kontron.de> 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@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@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. > 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@samsung.com> Best regards
On 15.11.2022 13:00, Marek Vasut wrote: > On 11/14/22 08:49, Jagan Teki wrote: >> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote: >>> >>> On 11/10/22 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[] = { >>> >>> You can also add : >>> >>> MEDIA_BUS_FMT_YUYV10_1X20 >>> >>> MEDIA_BUS_FMT_YUYV12_1X24 >> >> Are these for the below formats? >> >> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2 >> Packed Pixel Stream, 24-bit YCbCr, 4:2:2" >>> >>>> + 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; >>>> + >>>> + *num_input_fmts = 1; >>> >>> Shouldn't this be 6 ? >>> >>>> + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); >>>> + if (!input_fmts) >>>> + return NULL; >>>> + >>>> + input_fmts[0] = output_fmt; >>> >>> Shouldn't this be a list of all 6 supported pixel formats ? >> >> Negotiation would settle to return one input_fmt from the list of >> supporting output_fmts. so the num_input_fmts would be 1 rather than >> the number of fmts in the supporting list. This is what I understood >> from the atomic_get_input_bus_fmts API. let me know if I miss >> something here. > > How does the negotiation work for this kind of pipeline: > > LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector > > where all elements (LCDIFv3, DSIM, HDMI bridge) can support either > RGB888 or packed YUV422 ? > > Who decides the format used by such pipeline ? > > Why should it be the DSIM bridge and not e.g. the HDMI bridge or the > LCDIFv3 ? If I got it right, the drivers reports their preference for the returned formats. The format that is first in the reported array is the most preferred one. This probably means that in your case the HDMI bridge decides by reporting RGB or YUV first (if all elements supports both). Best regards
On 11/16/22 09:07, Marek Szyprowski wrote: > On 15.11.2022 13:00, Marek Vasut wrote: >> On 11/14/22 08:49, Jagan Teki wrote: >>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 11/10/22 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[] = { >>>> >>>> You can also add : >>>> >>>> MEDIA_BUS_FMT_YUYV10_1X20 >>>> >>>> MEDIA_BUS_FMT_YUYV12_1X24 >>> >>> Are these for the below formats? >>> >>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2 >>> Packed Pixel Stream, 24-bit YCbCr, 4:2:2" >>>> >>>>> + 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; >>>>> + >>>>> + *num_input_fmts = 1; >>>> >>>> Shouldn't this be 6 ? >>>> >>>>> + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); >>>>> + if (!input_fmts) >>>>> + return NULL; >>>>> + >>>>> + input_fmts[0] = output_fmt; >>>> >>>> Shouldn't this be a list of all 6 supported pixel formats ? >>> >>> Negotiation would settle to return one input_fmt from the list of >>> supporting output_fmts. so the num_input_fmts would be 1 rather than >>> the number of fmts in the supporting list. This is what I understood >>> from the atomic_get_input_bus_fmts API. let me know if I miss >>> something here. >> >> How does the negotiation work for this kind of pipeline: >> >> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector >> >> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either >> RGB888 or packed YUV422 ? >> >> Who decides the format used by such pipeline ? >> >> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the >> LCDIFv3 ? > > > If I got it right, the drivers reports their preference for the returned > formats. The format that is first in the reported array is the most > preferred one. This probably means that in your case the HDMI bridge > decides by reporting RGB or YUV first (if all elements supports both). But in that case, we need to list input_fmts[0]...input_fmts[n-1] in samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts = n, correct ?
On 16.11.2022 11:49, Marek Vasut wrote: > On 11/16/22 09:07, Marek Szyprowski wrote: >> On 15.11.2022 13:00, Marek Vasut wrote: >>> On 11/14/22 08:49, Jagan Teki wrote: >>>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote: >>>>> >>>>> On 11/10/22 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[] = { >>>>> >>>>> You can also add : >>>>> >>>>> MEDIA_BUS_FMT_YUYV10_1X20 >>>>> >>>>> MEDIA_BUS_FMT_YUYV12_1X24 >>>> >>>> Are these for the below formats? >>>> >>>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2 >>>> Packed Pixel Stream, 24-bit YCbCr, 4:2:2" >>>>> >>>>>> + 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; >>>>>> + >>>>>> + *num_input_fmts = 1; >>>>> >>>>> Shouldn't this be 6 ? >>>>> >>>>>> + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); >>>>>> + if (!input_fmts) >>>>>> + return NULL; >>>>>> + >>>>>> + input_fmts[0] = output_fmt; >>>>> >>>>> Shouldn't this be a list of all 6 supported pixel formats ? >>>> >>>> Negotiation would settle to return one input_fmt from the list of >>>> supporting output_fmts. so the num_input_fmts would be 1 rather than >>>> the number of fmts in the supporting list. This is what I understood >>>> from the atomic_get_input_bus_fmts API. let me know if I miss >>>> something here. >>> >>> How does the negotiation work for this kind of pipeline: >>> >>> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector >>> >>> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either >>> RGB888 or packed YUV422 ? >>> >>> Who decides the format used by such pipeline ? >>> >>> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the >>> LCDIFv3 ? >> >> >> If I got it right, the drivers reports their preference for the returned >> formats. The format that is first in the reported array is the most >> preferred one. This probably means that in your case the HDMI bridge >> decides by reporting RGB or YUV first (if all elements supports both). > > But in that case, we need to list input_fmts[0]...input_fmts[n-1] in > samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts > = n, correct ? > Yes, if I got the bus format negotiation code right, but I didn't check the details. Best regards
On Wed, Nov 16, 2022 at 4:37 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 16.11.2022 11:49, Marek Vasut wrote: > > On 11/16/22 09:07, Marek Szyprowski wrote: > >> On 15.11.2022 13:00, Marek Vasut wrote: > >>> On 11/14/22 08:49, Jagan Teki wrote: > >>>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote: > >>>>> > >>>>> On 11/10/22 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[] = { > >>>>> > >>>>> You can also add : > >>>>> > >>>>> MEDIA_BUS_FMT_YUYV10_1X20 > >>>>> > >>>>> MEDIA_BUS_FMT_YUYV12_1X24 > >>>> > >>>> Are these for the below formats? > >>>> > >>>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2 > >>>> Packed Pixel Stream, 24-bit YCbCr, 4:2:2" > >>>>> > >>>>>> + 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; > >>>>>> + > >>>>>> + *num_input_fmts = 1; > >>>>> > >>>>> Shouldn't this be 6 ? > >>>>> > >>>>>> + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > >>>>>> + if (!input_fmts) > >>>>>> + return NULL; > >>>>>> + > >>>>>> + input_fmts[0] = output_fmt; > >>>>> > >>>>> Shouldn't this be a list of all 6 supported pixel formats ? > >>>> > >>>> Negotiation would settle to return one input_fmt from the list of > >>>> supporting output_fmts. so the num_input_fmts would be 1 rather than > >>>> the number of fmts in the supporting list. This is what I understood > >>>> from the atomic_get_input_bus_fmts API. let me know if I miss > >>>> something here. > >>> > >>> How does the negotiation work for this kind of pipeline: > >>> > >>> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector > >>> > >>> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either > >>> RGB888 or packed YUV422 ? > >>> > >>> Who decides the format used by such pipeline ? > >>> > >>> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the > >>> LCDIFv3 ? > >> > >> > >> If I got it right, the drivers reports their preference for the returned > >> formats. The format that is first in the reported array is the most > >> preferred one. This probably means that in your case the HDMI bridge > >> decides by reporting RGB or YUV first (if all elements supports both). > > > > But in that case, we need to list input_fmts[0]...input_fmts[n-1] in > > samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts > > = n, correct ? > > > Yes, if I got the bus format negotiation code right, but I didn't check > the details. It Looks like if the number of formats returned by the array is more than 1, then the input_fmts array needs to return by listing formats in decreasing preference order so that the bridge core will try all formats until it finds one that works. But the question here is how much is the array number, how to decide? I can see dw-hdmi return a maximum of 3 possible input formats for an output format so the array seems to be 3 here, but the rest of drivers like imx8qxp-pixel-link do return 1 from the list of supported - dsim doing the same here. If we are sure dsim can support or feature 2 then order RGB888 and YUV422 decreasing preference and return them otherwise stick with a single return of checking supported list and return desired one. Jagan.
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; + + *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,
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(+)