[RESEND,v11,11/18] drm: exynos: dsi: Add atomic_get_input_bus_fmts
diff mbox series

Message ID 20230123151212.269082-12-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki Jan. 23, 2023, 3:12 p.m. UTC 
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

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v11:
- collect RB from Frieder
- drop extra line
Changes for v10:
- none
Changes for v9:
- added MEDIA_BUS_FMT_FIXED
- return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt
  list is unsupported.
- added MEDIA_BUS_FMT_YUYV10_1X20, MEDIA_BUS_FMT_YUYV12_1X24
Changes for v8:
- added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
Changes for v7 - v4:
- none
Changes for v3:
- include media-bus-format.h
Changes for v2:
- none
Changes for v1:
- new patch

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 68 +++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

Comments

Marek Vasut Jan. 24, 2023, 8:45 p.m. UTC | #1 
On 1/23/23 16:12, Jagan Teki wrote:

[...]

> +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt)
> +{
> +	int i;

if (fmt == MEDIA_BUS_FMT_FIXED)
  return false;

> +	for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) {
> +		if (exynos_dsi_pixel_output_fmts[i] == fmt)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static u32 *
> +exynos_dsi_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 (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
> +		/*
> +		 * 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 formats finalized.
> +		 */
> +		output_fmt = MEDIA_BUS_FMT_RGB888_1X24;

You can move this ^ past the kmalloc() call, so in unlikely case the 
kmalloc() fails, it would fail first.

> +	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;

Delete from here ...

> +	switch (output_fmt) {
> +	case MEDIA_BUS_FMT_FIXED:
> +		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +		break;
> +	default:
> +		input_fmts[0] = output_fmt;
> +		break;
> +	}

... until here, and do simple:

input_fmts[0] = output_fmt;

The effect should be the same, the code should be simpler and faster.

> +	*num_input_fmts = 1;

[...]
Jagan Teki Jan. 24, 2023, 9:16 p.m. UTC | #2 
On Wed, Jan 25, 2023 at 2:15 AM Marek Vasut <marex@denx.de> wrote:
>
> On 1/23/23 16:12, Jagan Teki wrote:
>
> [...]
>
> > +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt)
> > +{
> > +     int i;
>
> if (fmt == MEDIA_BUS_FMT_FIXED)
>   return false;
>
> > +     for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) {
> > +             if (exynos_dsi_pixel_output_fmts[i] == fmt)
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static u32 *
> > +exynos_dsi_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 (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
> > +             /*
> > +              * 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 formats finalized.
> > +              */
> > +             output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
>
> You can move this ^ past the kmalloc() call, so in unlikely case the
> kmalloc() fails, it would fail first.

I didn't get this point, what do we need to do if
exynos_dsi_pixel_output_fmt_supported returns false?

Jagan.
Marek Vasut Jan. 24, 2023, 9:19 p.m. UTC | #3 
On 1/24/23 22:16, Jagan Teki wrote:
> On Wed, Jan 25, 2023 at 2:15 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/23/23 16:12, Jagan Teki wrote:
>>
>> [...]
>>
>>> +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt)
>>> +{
>>> +     int i;
>>
>> if (fmt == MEDIA_BUS_FMT_FIXED)
>>    return false;
>>
>>> +     for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) {
>>> +             if (exynos_dsi_pixel_output_fmts[i] == fmt)
>>> +                     return true;
>>> +     }
>>> +
>>> +     return false;
>>> +}
>>> +
>>> +static u32 *
>>> +exynos_dsi_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 (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
>>> +             /*
>>> +              * 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 formats finalized.
>>> +              */
>>> +             output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
>>
>> You can move this ^ past the kmalloc() call, so in unlikely case the
>> kmalloc() fails, it would fail first.
> 
> I didn't get this point, what do we need to do if
> exynos_dsi_pixel_output_fmt_supported returns false?

{
	u32 *input_fmts;

	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
	if (!input_fmts)
		return NULL;

	if (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
		/* ... the comment ... */
		output_fmt = MEDIA_BUS_FMT_RGB888_1X24;

	input_fmts[0] = output_fmt;
	*num_input_fmts = 1;

	return input_fmts;
}
Jagan Teki Jan. 24, 2023, 9:22 p.m. UTC | #4 
On Wed, Jan 25, 2023 at 2:49 AM Marek Vasut <marex@denx.de> wrote:
>
> On 1/24/23 22:16, Jagan Teki wrote:
> > On Wed, Jan 25, 2023 at 2:15 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/23/23 16:12, Jagan Teki wrote:
> >>
> >> [...]
> >>
> >>> +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt)
> >>> +{
> >>> +     int i;
> >>
> >> if (fmt == MEDIA_BUS_FMT_FIXED)
> >>    return false;
> >>
> >>> +     for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) {
> >>> +             if (exynos_dsi_pixel_output_fmts[i] == fmt)
> >>> +                     return true;
> >>> +     }
> >>> +
> >>> +     return false;
> >>> +}
> >>> +
> >>> +static u32 *
> >>> +exynos_dsi_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 (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
> >>> +             /*
> >>> +              * 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 formats finalized.
> >>> +              */
> >>> +             output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
> >>
> >> You can move this ^ past the kmalloc() call, so in unlikely case the
> >> kmalloc() fails, it would fail first.
> >
> > I didn't get this point, what do we need to do if
> > exynos_dsi_pixel_output_fmt_supported returns false?
>
> {
>         u32 *input_fmts;
>
>         input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>         if (!input_fmts)
>                 return NULL;
>
>         if (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
>                 /* ... the comment ... */
>                 output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
>
>         input_fmts[0] = output_fmt;
>         *num_input_fmts = 1;
>
>         return input_fmts;
> }

Got it, thanks!

Patch
diff mbox series 

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 5518d92c8455..7afbbe30d1d3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -12,6 +12,7 @@ 
 #include <linux/component.h>
 #include <linux/gpio/consumer.h>
 #include <linux/irq.h>
+#include <linux/media-bus-format.h>
 #include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/phy/phy.h>
@@ -1466,6 +1467,72 @@  static void exynos_dsi_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 exynos_dsi_pixel_output_fmts[] = {
+	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,
+	MEDIA_BUS_FMT_FIXED,
+};
+
+static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) {
+		if (exynos_dsi_pixel_output_fmts[i] == fmt)
+			return true;
+	}
+
+	return false;
+}
+
+static u32 *
+exynos_dsi_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 (!exynos_dsi_pixel_output_fmt_supported(output_fmt))
+		/*
+		 * 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 formats finalized.
+		 */
+		output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
+
+	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	switch (output_fmt) {
+	case MEDIA_BUS_FMT_FIXED:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+		break;
+	default:
+		input_fmts[0] = output_fmt;
+		break;
+	}
+
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
 static int exynos_dsi_atomic_check(struct drm_bridge *bridge,
 				   struct drm_bridge_state *bridge_state,
 				   struct drm_crtc_state *crtc_state,
@@ -1514,6 +1581,7 @@  static const struct drm_bridge_funcs exynos_dsi_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	= exynos_dsi_atomic_get_input_bus_fmts,
 	.atomic_check			= exynos_dsi_atomic_check,
 	.atomic_pre_enable		= exynos_dsi_atomic_pre_enable,
 	.atomic_enable			= exynos_dsi_atomic_enable,