[PATCH v2 0/5] allow override of bus format in bridges

[PATCH v2 0/5] allow override of bus format in bridges

[Peter Rosin]

Hi!

[I got to v2 sooner than expected]

I have an Atmel sama5d31 hooked up to an lvds encoder and then
on to an lvds panel. Which seems like something that has been
done one or two times before...

The problem is that the bus_format of the SoC and the panel do
not agree. The SoC driver (atmel-hlcdc) can handle the
rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
wired for the rgb565 case. The lvds encoder supports rgb888 on
its input side with the LSB wires for each color simply pulled
down internally in the encoder in my case which means that the
rgb565 bus_format is the format that works best. And the panel
is expecting lvds (vesa-24), which is what the encoder outputs.

The reason I "blame" the bus_format of the drm_connector is that
with the below DT snippet, things do not work *exactly* due to
that. At least, it starts to work if I hack the panel-lvds driver
to report the rgb565 bus_format instead of vesa-24.

	panel: panel {
		compatible = "panel-lvds";

		width-mm = <304>;
		height-mm = <228;

		data-mapping = "vesa-24";

		panel-timing {
			// 1024x768 @ 60Hz (typical)
			clock-frequency = <52140000 65000000 71100000>;
			hactive = <1024>;
			vactive = <768>;
			hfront-porch = <48 88 88>;
			hback-porch = <96 168 168>;
			hsync-len = <32 64 64>;
			vfront-porch = <8 13 14>;
			vback-porch = <8 13 14>;
			vsync-len = <8 12 14>;
		};

		port {
			panel_input: endpoint {
				remote-endpoint = <&lvds_encoder_output>;
			};
		};
	};

	lvds-encoder {
		compatible = "ti,ds90c185", "lvds-encoder";

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				reg = <0>;

				lvds_encoder_input: endpoint {
					remote-endpoint = <&hlcdc_output>;
				};
			};

			port@1 {
				reg = <1>;

				lvds_encoder_output: endpoint {
					remote-endpoint = <&panel_input>;
				};
			};
		};
	};

But, instead of perverting the panel-lvds driver with support
for a totally fake non-lvds bus_format, I intruduce an API that allows
display controller drivers to query the required bus_format of any
intermediate bridges, and match up with that instead of the formats
given by the drm_connector. I trigger this with this addition to the
lvds-encoder DT node:

		interface-pix-fmt = "rgb565";

Naming is hard though, so I'm not sure if that's good?

I threw in the first patch, since that is the actual lvds encoder
I have in this case.

Suggestions welcome.

Cheers,
Peter

Changes since v1 https://lkml.org/lkml/2018/3/17/221
- Add a proper bridge API to query the bus_format instead of abusing
  the ->get_modes part of the code. This is cleaner but requires
  changes to all display controller drivers wishing to participate.
- Add patch to adjust the atmel-hlcdc driver according to the above.
- Hook the new info into the bridge local to the lvds-encoder instead
  of messing about with new interfaces for the panel-bridge driver.
- Add patch with a DT parsing function of bus_formats in a central place.
- Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.

Peter Rosin (5):
  dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
  drm: bridge: add API to query the expected input formats of bridges
  drm: of: add display bus-format parser
  drm: bridge: lvds-encoder: allow specifying the input bus format
  drm/atmel-hlcdc: take bridges into account when selecting output
    format

 .../bindings/display/bridge/lvds-transmitter.txt   | 14 ++++-
 .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 49 ++++++++++++++++--
 drivers/gpu/drm/bridge/lvds-encoder.c              | 25 +++++++++
 drivers/gpu/drm/drm_bridge.c                       | 32 ++++++++++++
 drivers/gpu/drm/drm_of.c                           | 59 ++++++++++++++++++++++
 include/drm/drm_bridge.h                           | 18 +++++++
 include/drm/drm_of.h                               |  9 ++++
 8 files changed, 237 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bus-format.txt

-- 
2.11.0

[PATCH v2 1/5] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185

[Peter Rosin]

Start list of actual chips compatible with "lvds-encoder".

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/bridge/lvds-transmitter.txt       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..50220190c203 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,7 +22,13 @@ among others.
 
 Required properties:
 
-- compatible: Must be "lvds-encoder"
+- compatible: Must be one or more of the following
+  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
+  - "lvds-encoder" for a generic LVDS encoder device
+
+  When compatible with the generic version, nodes must list the
+  device-specific version corresponding to the device first
+  followed by the generic version.
 
 Required nodes:
 
-- 
2.11.0

Re: [PATCH v2 1/5] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Tuesday, 27 March 2018 00:24:43 EEST Peter Rosin wrote:
> Start list of actual chips compatible with "lvds-encoder".
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../devicetree/bindings/display/bridge/lvds-transmitter.txt       | 8 ++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> index fd39ad34c383..50220190c203 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> @@ -22,7 +22,13 @@ among others.
> 
>  Required properties:
> 
> -- compatible: Must be "lvds-encoder"
> +- compatible: Must be one or more of the following
> +  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
> +  - "lvds-encoder" for a generic LVDS encoder device
> +
> +  When compatible with the generic version, nodes must list the
> +  device-specific version corresponding to the device first
> +  followed by the generic version.
> 
>  Required nodes:


-- 
Regards,

Laurent Pinchart

[PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[Peter Rosin]

Bridges may affect the required bus output format of the encoder, in
which case it may be wrong to use the output format of the panel or
connector as is. Add infrastructure to address this problem.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     | 18 ++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..f85e61b7416e 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -348,6 +348,38 @@ void drm_bridge_enable(struct drm_bridge *bridge)
 }
 EXPORT_SYMBOL(drm_bridge_enable);
 
+/**
+ * drm_bridge_input_formats - get the expected bus input format of the bridge
+ * @bridge: bridge control structure
+ * @bus_formats: where to store a pointer to the bus input formats
+ *
+ * Calls the &drm_bridge_funcs.input_formats op for the frist bridge in the
+ * chain that has registered this op.
+ *
+ * Note that the bridge passed should normally be the bridge closest to the
+ * encoder, but possibly the bridge closest to an intermediate bridge in
+ * convoluted cases.
+ *
+ * RETURNS:
+ * The number of bus input formats the bridge accepts. Zero means that
+ * the chain of bridges are not converting the bus format and that the
+ * format of the drm_connector should be used.
+ */
+int drm_bridge_input_formats(struct drm_bridge *bridge,
+			     const u32 **bus_formats)
+{
+	int ret = 0;
+
+	if (!bridge)
+		return 0;
+
+	if (bridge->funcs->input_formats)
+		ret = bridge->funcs->input_formats(bridge, bus_formats);
+
+	return ret ?: drm_bridge_input_formats(bridge->next, bus_formats);
+}
+EXPORT_SYMBOL(drm_bridge_input_formats);
+
 #ifdef CONFIG_OF
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..ae8d3c4af0b8 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -220,6 +220,22 @@ struct drm_bridge_funcs {
 	 * The enable callback is optional.
 	 */
 	void (*enable)(struct drm_bridge *bridge);
+
+	/**
+	 * @input_formats:
+	 *
+	 * The callback reports the expected bus input formats of the bridge.
+	 *
+	 * The @input_formats callback is optional. The bridge is assumed to
+	 * not convert the bus format if the callback is not installed.
+	 *
+	 * RETURNS:
+	 *
+	 * Zero if the bridge does not convert the bus format, otherwise the
+	 * number of bus input formats returned in &bus_formats.
+	 */
+	int (*input_formats)(struct drm_bridge *bridge,
+			     const u32 **bus_formats);
 };
 
 /**
@@ -263,6 +279,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
 			struct drm_display_mode *adjusted_mode);
 void drm_bridge_pre_enable(struct drm_bridge *bridge);
 void drm_bridge_enable(struct drm_bridge *bridge);
+int drm_bridge_input_formats(struct drm_bridge *bridge,
+			     const u32 **bus_formats);
 
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
-- 
2.11.0

Re: [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5584 bytes --]

Hi Peter,
   thanks for the patches

On Mon, Mar 26, 2018 at 11:24:44PM +0200, Peter Rosin wrote:
> Bridges may affect the required bus output format of the encoder, in
> which case it may be wrong to use the output format of the panel or
> connector as is. Add infrastructure to address this problem.

Bridges not only affect the format expected by the connector at the
end of the display pipeline, they may perform encoding/decoding
between protocols and their accepted input formats may be unrelated to
the connector at the end of the pipeline as there may an arbitrary
number of bridges in between.

Eg.

ENCODER                                                CONNECTOR
  |                                                         |
|DU LVDS| ->lvds-> |THC63| ->rgb-> |ADV7511| ->hdmi-> |HDMI connector|

The fact that THC63 wants a specific LVDS input format is unrelated to
the format required by the HDMI connector at the end of the pipeline.

I would just state here that bridges need a way to report their
accepted media bus formats, and this patch extends the DRM Bridge APIs
to implement that.

>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     | 18 ++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..f85e61b7416e 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -348,6 +348,38 @@ void drm_bridge_enable(struct drm_bridge *bridge)
>  }
>  EXPORT_SYMBOL(drm_bridge_enable);
>
> +/**
> + * drm_bridge_input_formats - get the expected bus input format of the bridge

I may be biased by the V4L2 APIs, but this seems to me very much like
similar to g_fmt/s_fmt callbacks we have there. Bridges have an input and
an output formats, and that calls for something that takes that into
account, as well as they may have different input ports with different
accepted formats but I would for now simplify this to just 'g_fmt()'

> + * @bridge: bridge control structure
> + * @bus_formats: where to store a pointer to the bus input formats
> + *
> + * Calls the &drm_bridge_funcs.input_formats op for the frist bridge in the
> + * chain that has registered this op.

I'm not sure passing the call down the pipeline is desirable. Each
component should make sure its output format is accepted as the next
bridge input format, passing the call to the next bridge is not
different that getting to the connector at the end of the pipeline and
return to the initial caller its supported format. Do you agree with this?

> + *
> + * Note that the bridge passed should normally be the bridge closest to the
> + * encoder, but possibly the bridge closest to an intermediate bridge in
> + * convoluted cases.
> + *

As I see this, any bridge in the arbitrary long pipeline should call
this operation on next bridge if it supports different output formats.
Ie. I would not name here the encoder nor refer to the bridge position
in the pipeline.

> + * RETURNS:
> + * The number of bus input formats the bridge accepts. Zero means that
> + * the chain of bridges are not converting the bus format and that the
> + * format of the drm_connector should be used.

How do we get to the connector format from a bridge that has maybe
other components in between in the pipeline?

If a bridge do not report any supported format, it won't implement
this callback and things will work as they work today.

> + */
> +int drm_bridge_input_formats(struct drm_bridge *bridge,
> +			     const u32 **bus_formats)
> +{
> +	int ret = 0;
> +
> +	if (!bridge)
> +		return 0;
> +
> +	if (bridge->funcs->input_formats)
> +		ret = bridge->funcs->input_formats(bridge, bus_formats);
> +
> +	return ret ?: drm_bridge_input_formats(bridge->next, bus_formats);

See my comment on the call propagation down in the pipeline.

> +}
> +EXPORT_SYMBOL(drm_bridge_input_formats);
> +
>  #ifdef CONFIG_OF
>  /**
>   * of_drm_find_bridge - find the bridge corresponding to the device node in
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 682d01ba920c..ae8d3c4af0b8 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -220,6 +220,22 @@ struct drm_bridge_funcs {
>  	 * The enable callback is optional.
>  	 */
>  	void (*enable)(struct drm_bridge *bridge);
> +
> +	/**
> +	 * @input_formats:
> +	 *
> +	 * The callback reports the expected bus input formats of the bridge.
> +	 *
> +	 * The @input_formats callback is optional. The bridge is assumed to
> +	 * not convert the bus format if the callback is not installed.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * Zero if the bridge does not convert the bus format, otherwise the
> +	 * number of bus input formats returned in &bus_formats.
> +	 */
> +	int (*input_formats)(struct drm_bridge *bridge,
> +			     const u32 **bus_formats);

Consider g_fmt() here as well, or a function name that captures that we want
to know the supported format (and possibly configure it as well one
day, if ever possible).

Thanks
   j

>  };
>
>  /**
> @@ -263,6 +279,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>  			struct drm_display_mode *adjusted_mode);
>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>  void drm_bridge_enable(struct drm_bridge *bridge);
> +int drm_bridge_input_formats(struct drm_bridge *bridge,
> +			     const u32 **bus_formats);
>
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> --
> 2.11.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[Peter Rosin]

Hi Jacopo,

Thanks for the feedback!

On 2018-03-27 11:47, jacopo mondi wrote:
> Hi Peter,
>    thanks for the patches
> 
> On Mon, Mar 26, 2018 at 11:24:44PM +0200, Peter Rosin wrote:
>> Bridges may affect the required bus output format of the encoder, in
>> which case it may be wrong to use the output format of the panel or
>> connector as is. Add infrastructure to address this problem.
> 
> Bridges not only affect the format expected by the connector at the
> end of the display pipeline, they may perform encoding/decoding
> between protocols and their accepted input formats may be unrelated to
> the connector at the end of the pipeline as there may an arbitrary
> number of bridges in between.
> 
> Eg.
> 
> ENCODER                                                CONNECTOR
>   |                                                         |
> |DU LVDS| ->lvds-> |THC63| ->rgb-> |ADV7511| ->hdmi-> |HDMI connector|
> 
> The fact that THC63 wants a specific LVDS input format is unrelated to
> the format required by the HDMI connector at the end of the pipeline.
> 
> I would just state here that bridges need a way to report their
> accepted media bus formats, and this patch extends the DRM Bridge APIs
> to implement that.

Yes, I can add some words, but I would like to clear up the naming
before re-spinning.

>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++++++++++++++++++++++
>>  include/drm/drm_bridge.h     | 18 ++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 1638bfe9627c..f85e61b7416e 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -348,6 +348,38 @@ void drm_bridge_enable(struct drm_bridge *bridge)
>>  }
>>  EXPORT_SYMBOL(drm_bridge_enable);
>>
>> +/**
>> + * drm_bridge_input_formats - get the expected bus input format of the bridge
> 
> I may be biased by the V4L2 APIs, but this seems to me very much like
> similar to g_fmt/s_fmt callbacks we have there. Bridges have an input and

g_fmt/s_fmt says nothing to me. Graphics format? Source Format? I have
no idea if those guesses are even close? So, that seems like poor naming
to me. The only reason I see to go that way is for the sake of consistency.

> an output formats, and that calls for something that takes that into
> account, as well as they may have different input ports with different
> accepted formats but I would for now simplify this to just 'g_fmt()'

You mean rename the function to drm_bridge_g_fmt(), right?

As indicated above, I'm not that fond of it, but don't really care
either. Maintainers?

>> + * @bridge: bridge control structure
>> + * @bus_formats: where to store a pointer to the bus input formats
>> + *
>> + * Calls the &drm_bridge_funcs.input_formats op for the frist bridge in the
>> + * chain that has registered this op.
> 
> I'm not sure passing the call down the pipeline is desirable. Each
> component should make sure its output format is accepted as the next
> bridge input format, passing the call to the next bridge is not
> different that getting to the connector at the end of the pipeline and
> return to the initial caller its supported format. Do you agree with this?

Not passing down the call does one of two things. Either all bridges have
to implement the call or all users have to walk the pipeline "manually".
I don't like either or those options, so I still think it is good to
pass the call down the pipeline.

*time passes*

Oh, you do not think it is useful for the bridge to have a callback but
still report "no format change", and that the call down to pipeline
should only happen for bridges that have not implemented the op. But I
do think that is useful, see below.

>> + *
>> + * Note that the bridge passed should normally be the bridge closest to the
>> + * encoder, but possibly the bridge closest to an intermediate bridge in
>> + * convoluted cases.
>> + *
> 
> As I see this, any bridge in the arbitrary long pipeline should call
> this operation on next bridge if it supports different output formats.
> Ie. I would not name here the encoder nor refer to the bridge position
> in the pipeline.

Ok, I can change that, it just seemed like a convoluted case to me.
I mean, if this was a real issue and complicated pipelines were
common, something along the lines of this patch series would be
available already. Right? I guess not, but how the &#/%# have people
been coping?

>> + * RETURNS:
>> + * The number of bus input formats the bridge accepts. Zero means that
>> + * the chain of bridges are not converting the bus format and that the
>> + * format of the drm_connector should be used.
> 
> How do we get to the connector format from a bridge that has maybe
> other components in between in the pipeline?
> 
> If a bridge do not report any supported format, it won't implement
> this callback and things will work as they work today.

Unless the bridge optionally changes the format. If the bridge has no
way to say "no format change" even with the callback in place, it
has to juggle different ops structs, which seems like a big hammer.
So, I'm in favor of calling down the pipeline in two cases. A) when
a bridge does not implement the op and B) when the op returns zero.

Maintainers?

>> + */
>> +int drm_bridge_input_formats(struct drm_bridge *bridge,
>> +			     const u32 **bus_formats)
>> +{
>> +	int ret = 0;
>> +
>> +	if (!bridge)
>> +		return 0;
>> +
>> +	if (bridge->funcs->input_formats)
>> +		ret = bridge->funcs->input_formats(bridge, bus_formats);
>> +
>> +	return ret ?: drm_bridge_input_formats(bridge->next, bus_formats);
> 
> See my comment on the call propagation down in the pipeline.
> 
>> +}
>> +EXPORT_SYMBOL(drm_bridge_input_formats);
>> +
>>  #ifdef CONFIG_OF
>>  /**
>>   * of_drm_find_bridge - find the bridge corresponding to the device node in
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 682d01ba920c..ae8d3c4af0b8 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -220,6 +220,22 @@ struct drm_bridge_funcs {
>>  	 * The enable callback is optional.
>>  	 */
>>  	void (*enable)(struct drm_bridge *bridge);
>> +
>> +	/**
>> +	 * @input_formats:
>> +	 *
>> +	 * The callback reports the expected bus input formats of the bridge.
>> +	 *
>> +	 * The @input_formats callback is optional. The bridge is assumed to
>> +	 * not convert the bus format if the callback is not installed.
>> +	 *
>> +	 * RETURNS:
>> +	 *
>> +	 * Zero if the bridge does not convert the bus format, otherwise the
>> +	 * number of bus input formats returned in &bus_formats.
>> +	 */
>> +	int (*input_formats)(struct drm_bridge *bridge,
>> +			     const u32 **bus_formats);
> 
> Consider g_fmt() here as well, or a function name that captures that we want
> to know the supported format (and possibly configure it as well one
> day, if ever possible).

The naming here should obviously follow the naming of the exported function.

Cheers,
Peter

> Thanks
>    j
> 
>>  };
>>
>>  /**
>> @@ -263,6 +279,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>>  			struct drm_display_mode *adjusted_mode);
>>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>>  void drm_bridge_enable(struct drm_bridge *bridge);
>> +int drm_bridge_input_formats(struct drm_bridge *bridge,
>> +			     const u32 **bus_formats);
>>
>>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>> --
>> 2.11.0
>>

Re: [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 9273 bytes --]

Hi Peter,

On Tue, Mar 27, 2018 at 02:12:42PM +0200, Peter Rosin wrote:
> Hi Jacopo,
>
> Thanks for the feedback!
>
> On 2018-03-27 11:47, jacopo mondi wrote:
> > Hi Peter,
> >    thanks for the patches
> >
> > On Mon, Mar 26, 2018 at 11:24:44PM +0200, Peter Rosin wrote:
> >> Bridges may affect the required bus output format of the encoder, in
> >> which case it may be wrong to use the output format of the panel or
> >> connector as is. Add infrastructure to address this problem.
> >
> > Bridges not only affect the format expected by the connector at the
> > end of the display pipeline, they may perform encoding/decoding
> > between protocols and their accepted input formats may be unrelated to
> > the connector at the end of the pipeline as there may an arbitrary
> > number of bridges in between.
> >
> > Eg.
> >
> > ENCODER                                                CONNECTOR
> >   |                                                         |
> > |DU LVDS| ->lvds-> |THC63| ->rgb-> |ADV7511| ->hdmi-> |HDMI connector|
> >
> > The fact that THC63 wants a specific LVDS input format is unrelated to
> > the format required by the HDMI connector at the end of the pipeline.
> >
> > I would just state here that bridges need a way to report their
> > accepted media bus formats, and this patch extends the DRM Bridge APIs
> > to implement that.
>
> Yes, I can add some words, but I would like to clear up the naming
> before re-spinning.
>
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++++++++++++++++++++++
> >>  include/drm/drm_bridge.h     | 18 ++++++++++++++++++
> >>  2 files changed, 50 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 1638bfe9627c..f85e61b7416e 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -348,6 +348,38 @@ void drm_bridge_enable(struct drm_bridge *bridge)
> >>  }
> >>  EXPORT_SYMBOL(drm_bridge_enable);
> >>
> >> +/**
> >> + * drm_bridge_input_formats - get the expected bus input format of the bridge
> >
> > I may be biased by the V4L2 APIs, but this seems to me very much like
> > similar to g_fmt/s_fmt callbacks we have there. Bridges have an input and
>
> g_fmt/s_fmt says nothing to me. Graphics format? Source Format? I have
> no idea if those guesses are even close? So, that seems like poor naming
> to me. The only reason I see to go that way is for the sake of consistency.
>

Sorry, I wasn't clear here.

To me this operation seems like a "get format" and I see space for
future implementation of "set format" operations  and also
"enum_formats" to implement format negotiation between bridges...


> > an output formats, and that calls for something that takes that into
> > account, as well as they may have different input ports with different
> > accepted formats but I would for now simplify this to just 'g_fmt()'
>
> You mean rename the function to drm_bridge_g_fmt(), right?

Yeah, something like "drm_bridge_get_format(next_bridge)"

>
> As indicated above, I'm not that fond of it, but don't really care
> either. Maintainers?
>

I do not care much about the name too ofc, I think the real meat is
below...

> >> + * @bridge: bridge control structure
> >> + * @bus_formats: where to store a pointer to the bus input formats
> >> + *
> >> + * Calls the &drm_bridge_funcs.input_formats op for the frist bridge in the
> >> + * chain that has registered this op.
> >
> > I'm not sure passing the call down the pipeline is desirable. Each
> > component should make sure its output format is accepted as the next
> > bridge input format, passing the call to the next bridge is not
> > different that getting to the connector at the end of the pipeline and
> > return to the initial caller its supported format. Do you agree with this?
>
> Not passing down the call does one of two things. Either all bridges have
> to implement the call or all users have to walk the pipeline "manually".
> I don't like either or those options, so I still think it is good to
> pass the call down the pipeline.
>
> *time passes*
>
> Oh, you do not think it is useful for the bridge to have a callback but
> still report "no format change", and that the call down to pipeline
> should only happen for bridges that have not implemented the op. But I
> do think that is useful, see below.
>
> >> + *
> >> + * Note that the bridge passed should normally be the bridge closest to the
> >> + * encoder, but possibly the bridge closest to an intermediate bridge in
> >> + * convoluted cases.
> >> + *
> >
> > As I see this, any bridge in the arbitrary long pipeline should call
> > this operation on next bridge if it supports different output formats.
> > Ie. I would not name here the encoder nor refer to the bridge position
> > in the pipeline.
>
> Ok, I can change that, it just seemed like a convoluted case to me.
> I mean, if this was a real issue and complicated pipelines were
> common, something along the lines of this patch series would be
> available already. Right? I guess not, but how the &#/%# have people
> been coping?
>
> >> + * RETURNS:
> >> + * The number of bus input formats the bridge accepts. Zero means that
> >> + * the chain of bridges are not converting the bus format and that the
> >> + * format of the drm_connector should be used.
> >
> > How do we get to the connector format from a bridge that has maybe
> > other components in between in the pipeline?
> >
> > If a bridge do not report any supported format, it won't implement
> > this callback and things will work as they work today.
>
> Unless the bridge optionally changes the format. If the bridge has no
> way to say "no format change" even with the callback in place, it
> has to juggle different ops structs, which seems like a big hammer.
> So, I'm in favor of calling down the pipeline in two cases. A) when
> a bridge does not implement the op and B) when the op returns zero.
>

Why do you think the bridge format conversion plays a role here?

Maybe here's where I lost you, possibly because of my limited
knowledge of the DRM realm and its actual use cases.

As I see it, this new API allows two bridges to synchronize on which
image format or LVDS mode 'bridge' should output to 'next_bridge'
input.

The "mode_set" callback walks all element of the display pipeline,
and when one component has to select which image format or LVDS mode
to output to its next kin, if the next one is a panel it inspects the
display_info field, it it's a bridge it uses this new API to get the
format it accepts.

If the next bridge does not implement this callback, things work as they
do today (in our specific use case, by chance :)

> Maintainers?
>

Yes, let's scale to more knowledgeable people than me, as I gave my
opinion already but it is based on the single use case I know for
real.

Thanks
   j


> >> + */
> >> +int drm_bridge_input_formats(struct drm_bridge *bridge,
> >> +			     const u32 **bus_formats)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +	if (!bridge)
> >> +		return 0;
> >> +
> >> +	if (bridge->funcs->input_formats)
> >> +		ret = bridge->funcs->input_formats(bridge, bus_formats);
> >> +
> >> +	return ret ?: drm_bridge_input_formats(bridge->next, bus_formats);
> >
> > See my comment on the call propagation down in the pipeline.
> >
> >> +}
> >> +EXPORT_SYMBOL(drm_bridge_input_formats);
> >> +
> >>  #ifdef CONFIG_OF
> >>  /**
> >>   * of_drm_find_bridge - find the bridge corresponding to the device node in
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index 682d01ba920c..ae8d3c4af0b8 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -220,6 +220,22 @@ struct drm_bridge_funcs {
> >>  	 * The enable callback is optional.
> >>  	 */
> >>  	void (*enable)(struct drm_bridge *bridge);
> >> +
> >> +	/**
> >> +	 * @input_formats:
> >> +	 *
> >> +	 * The callback reports the expected bus input formats of the bridge.
> >> +	 *
> >> +	 * The @input_formats callback is optional. The bridge is assumed to
> >> +	 * not convert the bus format if the callback is not installed.
> >> +	 *
> >> +	 * RETURNS:
> >> +	 *
> >> +	 * Zero if the bridge does not convert the bus format, otherwise the
> >> +	 * number of bus input formats returned in &bus_formats.
> >> +	 */
> >> +	int (*input_formats)(struct drm_bridge *bridge,
> >> +			     const u32 **bus_formats);
> >
> > Consider g_fmt() here as well, or a function name that captures that we want
> > to know the supported format (and possibly configure it as well one
> > day, if ever possible).
>
> The naming here should obviously follow the naming of the exported function.
>
> Cheers,
> Peter
>
> > Thanks
> >    j
> >
> >>  };
> >>
> >>  /**
> >> @@ -263,6 +279,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >>  			struct drm_display_mode *adjusted_mode);
> >>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >>  void drm_bridge_enable(struct drm_bridge *bridge);
> >> +int drm_bridge_input_formats(struct drm_bridge *bridge,
> >> +			     const u32 **bus_formats);
> >>
> >>  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >> --
> >> 2.11.0
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[Laurent Pinchart]

Hello,

First of all, thank you for the patch. This generates more discussion than I 
had anticipated, which is both good and bad. I'll comment through the e-mail, 
and try to explain both my initial idea, and also where it could lead us.

On Tuesday, 27 March 2018 16:02:31 EEST jacopo mondi wrote:
> On Tue, Mar 27, 2018 at 02:12:42PM +0200, Peter Rosin wrote:
> > On 2018-03-27 11:47, jacopo mondi wrote:
> >> On Mon, Mar 26, 2018 at 11:24:44PM +0200, Peter Rosin wrote:
> >>> Bridges may affect the required bus output format of the encoder, in
> >>> which case it may be wrong to use the output format of the panel or
> >>> connector as is. Add infrastructure to address this problem.
> >> 
> >> Bridges not only affect the format expected by the connector at the
> >> end of the display pipeline, they may perform encoding/decoding
> >> between protocols and their accepted input formats may be unrelated to
> >> the connector at the end of the pipeline as there may an arbitrary
> >> number of bridges in between.
> >> 
> >> Eg.
> >> 
> >> ENCODER                                                CONNECTOR
> >> 
> >> |DU LVDS| ->lvds-> |THC63| ->rgb-> |ADV7511| ->hdmi-> |HDMI connector|
> >> 
> >> The fact that THC63 wants a specific LVDS input format is unrelated to
> >> the format required by the HDMI connector at the end of the pipeline.
> >> 
> >> I would just state here that bridges need a way to report their
> >> accepted media bus formats, and this patch extends the DRM Bridge APIs
> >> to implement that.
> > 
> > Yes, I can add some words, but I would like to clear up the naming
> > before re-spinning.
> > 
> >>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++++++++++++++++++++++
> >>>  include/drm/drm_bridge.h     | 18 ++++++++++++++++++
> >>  2 files changed, 50 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/drm_bridge.c
> >>> b/drivers/gpu/drm/drm_bridge.c
> >>> index 1638bfe9627c..f85e61b7416e 100644
> >>> --- a/drivers/gpu/drm/drm_bridge.c
> >>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>> @@ -348,6 +348,38 @@ void drm_bridge_enable(struct drm_bridge *bridge)
> >>> 
> >>>  }
> >>>  EXPORT_SYMBOL(drm_bridge_enable);
> >>> 
> >>> +/**
> >>> + * drm_bridge_input_formats - get the expected bus input format of the
> >>> bridge
> >> 
> >> I may be biased by the V4L2 APIs, but this seems to me very much like
> >> similar to g_fmt/s_fmt callbacks we have there. Bridges have an input
> >> and
> > 
> > g_fmt/s_fmt says nothing to me. Graphics format? Source Format? I have
> > no idea if those guesses are even close? So, that seems like poor naming
> > to me. The only reason I see to go that way is for the sake of
> > consistency.
> 
> Sorry, I wasn't clear here.
> 
> To me this operation seems like a "get format" and I see space for
> future implementation of "set format" operations  and also
> "enum_formats" to implement format negotiation between bridges...

A bit of context is probably needed here to bridge (no pun intended) DRM and 
V4L2.

On the V4L2 side we have an object, named v4l2_subdev, that is analogous to 
drm_bridge. A v4l2_subdev models a component in a video pipeline, using 
abstract operations in a similar fashion to the drm_bridge_funcs. There are, 
however, several major differences between drm_bridge and v4l2_subdev.

drm_bridge models a particular kind of component, namely video encoders, 
decoders or transcoders. A bridge is thus based on the implicit notion of 
having a single input and a single output, neither of them exposed explicitly.

v4l2_subdev models any type of component in a video pipeline and for that 
reason has a variable number of inputs and outputs. The inputs and outputs are 
exposed explicitly through the v4l2_subdev API (in terms of the number of 
inputs/outputs, their type, and how they interact with the v4l2_subdev 
operations).

As drm_bridge models one particular kind of component, the bridge operations 
are tailored to the feature of those components. Not all operations are 
mandatory (pre_enable or post_disable are optional for instance), but they all 
apply to all bridges conceptually.

As v4l2_subdev models any type of component, the subdev operations have to 
support a wide variety of device features. There are thus many more 
operations, and not all of them are applicable to all type of v4l2_subdev. 
Operations that don't make sense for a particular v4l2_subdev instance (think 
about video tuner control for a camera sensor for instance) are simply not 
implemented.

The drm_bridge framework hardcodes an operational model for bridges. 
Operations are called in a particular order on all bridges in a chain. For 
instance the enable operation is called from source to sink (bridge closest to 
the display controller to bridge closest to the connector), while the disable 
operation is called from sink to source.

The v4l2_subdev framework doesn't hardcode an operation model. It is up to the 
top-level V4L2 driver (equivalent to the display controller driver) to call 
subdev operations in order applicable for the use cases it needs to support.

This being said, let's talk about formats. In V4L2 a v4l2_subdev exposes 
operations to explicitly configure formats on all inputs and outputs 
(generically called pads in V4L2). Three operations are supported: .get_fmt() 
to retrieve the current format, .set_fmt() to set a format, and 
enum_mbus_code() and .enum_frame_size() to enumerate supported formats (I 
won't go into details regarding why enumeration is split in two operations, 
that's irrelevant here). It is important to note that format enumeration is 
mostly static (it tells what a v4l2_subdev supports), while get and set 
operations are dynamic (getting and setting, and more generically negotiating, 
formats).

DRM has an API to enumerate the formats supported by connectors (through the 
drm_connector->display_info.bus_formats field). There is no API exposed to 
userspace to pick the desired format, neither at the connector level, or for 
bridges in the pipeline (bridges are not exposed to userspace at all). There 
is also no in-kernel API to enumerate formats supported by bridges or to get/
set a bridge format. Display controller are expected to pick an output format 
suitable for the output pipeline based on the formats supported by the 
connector and the selected display mode, and bridges are expected to just work 
without needing to care about formats.

This model is starting to show its shortcomings now that we need to support 
bridges that convert formats. In Jacopo's case, an LVDS decoder transforms 
LVDS RGB to parallel RGB, and the display controller needs to know which LVDS 
format to output based on what the bridge expects. In Peter's case, an LVDS 
encoder transfer 24-bit RGB to LVDS RGB, but has the LSBs tied to ground on 
the input side, and thus requires the display controller to output RGB565.

Now that we're done with the introduction, let's move on, please see below.

> >> an output formats, and that calls for something that takes that into
> >> account, as well as they may have different input ports with different
> >> accepted formats but I would for now simplify this to just 'g_fmt()'
> > 
> > You mean rename the function to drm_bridge_g_fmt(), right?
> 
> Yeah, something like "drm_bridge_get_format(next_bridge)"
> 
> > As indicated above, I'm not that fond of it, but don't really care
> > either. Maintainers?
> 
> I do not care much about the name too ofc, I think the real meat is
> below...
> 
> >>> + * @bridge: bridge control structure
> >>> + * @bus_formats: where to store a pointer to the bus input formats
> >>> + *
> >>> + * Calls the &drm_bridge_funcs.input_formats op for the frist bridge
> >>> in the
> >>> + * chain that has registered this op.
> >> 
> >> I'm not sure passing the call down the pipeline is desirable. Each
> >> component should make sure its output format is accepted as the next
> >> bridge input format, passing the call to the next bridge is not
> >> different that getting to the connector at the end of the pipeline and
> >> return to the initial caller its supported format. Do you agree with
> >> this?
> > 
> > Not passing down the call does one of two things. Either all bridges have
> > to implement the call or all users have to walk the pipeline "manually".
> > I don't like either or those options, so I still think it is good to
> > pass the call down the pipeline.
> > 
> > *time passes*
> > 
> > Oh, you do not think it is useful for the bridge to have a callback but
> > still report "no format change", and that the call down to pipeline
> > should only happen for bridges that have not implemented the op. But I
> > do think that is useful, see below.
> > 
> >>> + *
> >>> + * Note that the bridge passed should normally be the bridge closest to
> >>> the
> >>> + * encoder, but possibly the bridge closest to an intermediate bridge
> >>> in
> >>> + * convoluted cases.
> >>> + *
> >> 
> >> As I see this, any bridge in the arbitrary long pipeline should call
> >> this operation on next bridge if it supports different output formats.
> >> Ie. I would not name here the encoder nor refer to the bridge position
> >> in the pipeline.
> > 
> > Ok, I can change that, it just seemed like a convoluted case to me.
> > I mean, if this was a real issue and complicated pipelines were
> > common, something along the lines of this patch series would be
> > available already. Right? I guess not, but how the &#/%# have people
> > been coping?
> > 
> >>> + * RETURNS:
> >>> + * The number of bus input formats the bridge accepts. Zero means that
> >>> + * the chain of bridges are not converting the bus format and that the
> >>> + * format of the drm_connector should be used.
> >> 
> >> How do we get to the connector format from a bridge that has maybe
> >> other components in between in the pipeline?
> >> 
> >> If a bridge do not report any supported format, it won't implement
> >> this callback and things will work as they work today.
> > 
> > Unless the bridge optionally changes the format. If the bridge has no
> > way to say "no format change" even with the callback in place, it
> > has to juggle different ops structs, which seems like a big hammer.
> > So, I'm in favor of calling down the pipeline in two cases. A) when
> > a bridge does not implement the op and B) when the op returns zero.
> 
> Why do you think the bridge format conversion plays a role here?
> 
> Maybe here's where I lost you, possibly because of my limited
> knowledge of the DRM realm and its actual use cases.
> 
> As I see it, this new API allows two bridges to synchronize on which
> image format or LVDS mode 'bridge' should output to 'next_bridge'
> input.
> 
> The "mode_set" callback walks all element of the display pipeline,
> and when one component has to select which image format or LVDS mode
> to output to its next kin, if the next one is a panel it inspects the
> display_info field, it it's a bridge it uses this new API to get the
> format it accepts.
> 
> If the next bridge does not implement this callback, things work as they
> do today (in our specific use case, by chance :)
> 
> > Maintainers?
> 
> Yes, let's scale to more knowledgeable people than me, as I gave my
> opinion already but it is based on the single use case I know for
> real.

I think it's important here to take a step back and look where we come from, 
what we need, and where we want to go.

The first part is easy. As explained above, the current state is that we know 
what formats a connector supports, and we have no other API whatsoever, 
neither to userspace or inside the kernel, to handle format in the display 
output pipeline.

What we need is at least static format information. In the two cases described 
above the bridge connected to the display controller output requires a 
specific format (RGB565 for Peter, LVDS JEIDA for Jacopo). There is no need to 
walk to pipeline and query all bridges, we can get the necessary information 
from the first bridge in the chain.

Where we want to go is the real question. We need at least static information, 
so we could expose it through a new bus_formats field in the drm_bridge 
structure, similar to the bus_formats field in the drm_display_info structure. 
This would solve the two problems at hand. However, such a solution wouldn't 
allow us to configure formats through the pipeline, and wouldn't allow us to 
query the format a bridge needs based on the format needed by the next bridge 
in the chain. It would thus be limited to specific cases, and one could argue 
that it would be too limited.

A more generic solution might be better, to support dynamic negotiation of 
formats through the pipeline. I believe this would be needed in the long run 
anyway, so it's tempting to give it a try today. However, as we don't have any 
real use case yet, one might argue that we would design the API incorrectly, 
which I can't completely disagree with. Nonetheless, if we were to design such 
an API, I agree with Daniel that it should be based on bridge states. Such a 
solution would allow negotiation of formats through the pipeline (using a yet 
to be designed process), and would then allow the display controller to 
retrieve the negotiated format expected by the first bridge from the bridge 
state.

I don't think we would design the API between the display controller and the 
first bridge wrong (it would just be a matter of reading the negotiated format 
from the first bridge's state, there's little room for design issues there), 
but the format negotiation through the pipeline would need more thoughts. One 
option (just thinking out loud) would be to start with the connector format, 
and then ask each bridge in the chain, from last to first, for the input 
format it needs to generate the expected output format. More complex schemes 
might be needed, especially when the connector supports multiple formats, or 
when a bridge can accept multiple input formats to generate a given output 
format. In those cases picking the first supported format could work, but it 
might not always lead to the optimal pipeline configuration.

Another point I want to mention is that in Peter's case the LVDS encoder 
requires RGB888 at its input, but the PCB routing transforms RGB565 to RGB888 
by shifting signals and tying the LSBs to ground. That information really 
belongs to DT as it isn't intrinsic to either the display controller or the 
LVDS encoder. I however wonder whether it should be expressed as a format, or 
in a different form. In V4L2 we have standardized bus-width and data-shift DT 
properties (see Documentation/devicetree/bindings/media/video-interfaces.txt) 
to expose how signals are connected in the PCB. This allows expressing 
transformations instead of hardcoding formats, which can be useful if the 
bridge can still accept multiple input formats. One example of this is raw 
camera sensors that can output multiple bayer patterns (BGGR, GBRG, GRBG or 
RGGB) with 10 bits per pixel, combined with a PCB that connects D[9:2] of the 
sensor to D[7:0] of the camera controller. BGGR10 would be turned into BGGR8, 
GBRG10 into GBRG8, and so on. The camera controller should thus not hardcode a 
particular format in DT as there are multiple options that can be selected at 
runtime. I can imagine a similar setup for the display, with a bridge 
configurable through I2C that could accept different formats, combined with 
the PCB performing some weird routing.

Finally, still regarding Peter's case, the decision to output RGB565 instead 
of RGB888 (which the LVDS encoder expects) is due to PCB routing between the 
display controller and the LVDS encoder. This isn't a property of the LVDS 
encoder or the display controller, but of their hardware connection. This 
patch series uses a DT property in the LVDS encoder DT node to convey that 
information, but wouldn't it be equally correct (or incorrect :-)) to instead 
use a DT property in the display controller DT node ?

> >>> + */
> >>> +int drm_bridge_input_formats(struct drm_bridge *bridge,
> >>> +			     const u32 **bus_formats)
> >>> +{
> >>> +	int ret = 0;
> >>> +
> >>> +	if (!bridge)
> >>> +		return 0;
> >>> +
> >>> +	if (bridge->funcs->input_formats)
> >>> +		ret = bridge->funcs->input_formats(bridge, bus_formats);
> >>> +
> >>> +	return ret ?: drm_bridge_input_formats(bridge->next, bus_formats);
> >> 
> >> See my comment on the call propagation down in the pipeline.
> >> 
> >>> +}
> >>> +EXPORT_SYMBOL(drm_bridge_input_formats);
> >>> +
> >>>  #ifdef CONFIG_OF
> >>>  /**
> >>>   * of_drm_find_bridge - find the bridge corresponding to the device
> >>>   node in
> >>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >>> index 682d01ba920c..ae8d3c4af0b8 100644
> >>> --- a/include/drm/drm_bridge.h
> >>> +++ b/include/drm/drm_bridge.h
> >>> @@ -220,6 +220,22 @@ struct drm_bridge_funcs {
> >>>  	 * The enable callback is optional.
> >>>  	 */
> >>>  	void (*enable)(struct drm_bridge *bridge);
> >>> +
> >>> +	/**
> >>> +	 * @input_formats:
> >>> +	 *
> >>> +	 * The callback reports the expected bus input formats of the
> >>> bridge.
> >>> +	 *
> >>> +	 * The @input_formats callback is optional. The bridge is assumed to
> >>> +	 * not convert the bus format if the callback is not installed.
> >>> +	 *
> >>> +	 * RETURNS:
> >>> +	 *
> >>> +	 * Zero if the bridge does not convert the bus format, otherwise the
> >>> +	 * number of bus input formats returned in &bus_formats.
> >>> +	 */
> >>> +	int (*input_formats)(struct drm_bridge *bridge,
> >>> +			     const u32 **bus_formats);
> >> 
> >> Consider g_fmt() here as well, or a function name that captures that we
> >> want to know the supported format (and possibly configure it as well
> >> one day, if ever possible).
> > 
> > The naming here should obviously follow the naming of the exported
> > function.
> > 
> >>>  };
> >>>  
> >>>  /**
> >>> @@ -263,6 +279,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >>>  			struct drm_display_mode *adjusted_mode);
> >>>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >>>  void drm_bridge_enable(struct drm_bridge *bridge);
> >>> +int drm_bridge_input_formats(struct drm_bridge *bridge,
> >>> +			     const u32 **bus_formats);
> >>> 
> >>>  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[Peter Rosin]

On 2018-04-04 15:07, Laurent Pinchart wrote:
> First of all, thank you for the patch. This generates more discussion than I 
> had anticipated, which is both good and bad. I'll comment through the e-mail, 
> and try to explain both my initial idea, and also where it could lead us.

*snip*

Thank you for the long interesting mail! I will read it again, but my
immediate reaction is that I am not the man to attempt anything heavier
than static format information in drm bridges, and that I will hold off
any further work until some consensus has been reached.

> Finally, still regarding Peter's case, the decision to output RGB565 instead 
> of RGB888 (which the LVDS encoder expects) is due to PCB routing between the 
> display controller and the LVDS encoder. This isn't a property of the LVDS 
> encoder or the display controller, but of their hardware connection. This 
> patch series uses a DT property in the LVDS encoder DT node to convey that 
> information, but wouldn't it be equally correct (or incorrect :-)) to instead 
> use a DT property in the display controller DT node ?

Right, it might even be more correct to do it there? Thinking about it, I
have this in the DT

#include "sama5d3_lcd.dtsi"

&hlcdc {
	hlcdc-display-controller {
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;

		port@0 {
			hlcdc_output: endpoint {
				remote-endpoint = <&lvds_encoder_input>;
			};
		};
	};
};

Where the &pinctrl_lcd_rgb565 handle tells the chip to not even bother
routing all of the rgb888 signals to pins and thus not waste pins that
are in fact used for other things. Maybe it is possible for the hlcdc
driver to look at that info and suggest rgb565 instead of rgb888 without
anything further in the DT? But maybe it's difficult to peek into pinctrl
bindings like that?

Cheers,
Peter

Re: [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges

[Peter Rosin]

On 2018-03-27 11:47, jacopo mondi wrote:
>> + * RETURNS:
>> + * The number of bus input formats the bridge accepts. Zero means that
>> + * the chain of bridges are not converting the bus format and that the
>> + * format of the drm_connector should be used.
> 
> How do we get to the connector format from a bridge that has maybe
> other components in between in the pipeline?

Forgot to write something here in my previous reply, sorry...

The chain of bridges do not end with the connector in the in-kernel
representation, IIUC. So, I think it is up to the caller to find the
format desired by the connector. See patch [5/5] for an example.

Maybe the connector should be passed in as an argument?

Cheers,
Peter

[PATCH v2 3/5] drm: of: add display bus-format parser

[Peter Rosin]

Add a common API to parse display bus format strings into fourcc codes.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
 drivers/gpu/drm/drm_of.c                           | 59 ++++++++++++++++++++++
 include/drm/drm_of.h                               |  9 ++++
 3 files changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bus-format.txt

diff --git a/Documentation/devicetree/bindings/display/bus-format.txt b/Documentation/devicetree/bindings/display/bus-format.txt
new file mode 100644
index 000000000000..590e6c73f3dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bus-format.txt
@@ -0,0 +1,35 @@
+Bus formats in the display pipe
+===============================
+
+Various encoders in display controllers output the pixels in different
+formats. Circuits handling display connectors and hardwired panels also
+expect pixel input in various formats. We call these formats bus formats.
+
+Some bus formats are:
+
+Parallel
+--------
+
+rgb888
+	8 parallel lines for red, green and blue respectively. There are
+	also lines for the pixel-clock, horizontal-sync, vertical-sync
+	and data-enable.
+
+rgb666
+	Same as rgb888, but with 6 lines per color.
+
+rgb565
+	Same as rgb888, but with 6 green lines and 5 red and blue lines.
+
+rgb444
+	Same as rgb888, but with 4 lines per color.
+
+
+LVDS
+----
+
+jeida-18
+jeida-24
+vesa-24
+	These are LVDS bus formats, see the data-mapping property in
+	panel/panel-lvds.txt for a description of these bus formats.
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 4c191c050e7d..5f65471225bb 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -262,3 +262,62 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+
+/*
+ * drm_of_bus_formats - parse list of bus format strings into drm fourcc
+ * @np: device tree node containing bus format property
+ * @propname: name of bus format property
+ * @bus_formats: array of parsed fourcc codes
+ *
+ * On success, @bus_formats points to a location where the actual drm
+ * fourcc codes are.
+ * WARNING: The caller is responsible for freeing this memory with kfree.
+ *
+ * Returns the number of parsed bus format entries, or one of the standard
+ * error codes on failure.
+ */
+int drm_of_bus_formats(const struct device_node *np, const char *propname,
+		       u32 **bus_formats)
+{
+	int num_bus_formats = of_property_count_strings(np, propname);
+	const char *fmt;
+	int ret;
+	int i;
+
+	if (num_bus_formats <= 0)
+		return num_bus_formats;
+
+	*bus_formats = kmalloc(num_bus_formats * sizeof(**bus_formats),
+			       GFP_KERNEL);
+	if (!*bus_formats)
+		return -ENOMEM;
+
+	for (i = 0; i < num_bus_formats; ++i) {
+		ret = of_property_read_string_index(np, propname, i, &fmt);
+		if (ret < 0)
+			return ret;
+
+		if (!strcmp(fmt, "rgb444")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB444_1X12;
+		} else if (!strcmp(fmt, "rgb565")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB565_1X16;
+		} else if (!strcmp(fmt, "rgb666")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB666_1X18;
+		} else if (!strcmp(fmt, "rgb888")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB888_1X24;
+		} else if (!strcmp(fmt, "jeida-18")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
+		} else if (!strcmp(fmt, "jeida-24")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
+		} else if (!strcmp(fmt, "vesa-24")) {
+			*bus_formats[i] = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
+		} else {
+			kfree(*bus_formats);
+			*bus_formats = NULL;
+			return -EINVAL;
+		}
+	}
+
+	return num_bus_formats;
+}
+EXPORT_SYMBOL_GPL(drm_of_bus_formats);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index b93c239afb60..ccacddc03a2e 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -33,6 +33,8 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 				int port, int endpoint,
 				struct drm_panel **panel,
 				struct drm_bridge **bridge);
+int drm_of_bus_formats(const struct device_node *np, const char *propname,
+		       u32 **bus_formats);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 						  struct device_node *port)
@@ -69,6 +71,13 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
 {
 	return -EINVAL;
 }
+
+static inline int drm_of_bus_formats(const struct device_node *np,
+				     const char *propname,
+				     u32 **bus_formats)
+{
+	return -EINVAL;
+}
 #endif
 
 /*
-- 
2.11.0

[PATCH v2 4/5] drm: bridge: lvds-encoder: allow specifying the input bus format

[Peter Rosin]

If the bridge changes the bus format, allow this to be described in
the bridge, instead of providing false information about the bus
format of the connector or panel.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../bindings/display/bridge/lvds-transmitter.txt   |  6 ++++++
 drivers/gpu/drm/bridge/lvds-encoder.c              | 25 ++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index 50220190c203..8d40a2069252 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -30,6 +30,12 @@ Required properties:
   device-specific version corresponding to the device first
   followed by the generic version.
 
+Optional properties:
+
+- interface-pix-fmt:
+  List of valid input bus formats of the encoder. Recognized bus formats
+  are listed in ../bus-format.txt
+
 Required nodes:
 
 This device has two video ports. Their connections are modeled using the OF
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..b78619b5560a 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -9,6 +9,7 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 
 #include <linux/of_graph.h>
@@ -16,6 +17,8 @@
 struct lvds_encoder {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
+	int num_bus_formats;
+	u32 *bus_formats;
 };
 
 static int lvds_encoder_attach(struct drm_bridge *bridge)
@@ -28,8 +31,22 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
 				 bridge);
 }
 
+static int lvds_encoder_input_formats(struct drm_bridge *bridge,
+				      const u32 **bus_formats)
+{
+	struct lvds_encoder *lvds_encoder = container_of(bridge,
+							 struct lvds_encoder,
+							 bridge);
+
+	if (lvds_encoder->num_bus_formats)
+		*bus_formats = lvds_encoder->bus_formats;
+
+	return lvds_encoder->num_bus_formats;
+}
+
 static struct drm_bridge_funcs funcs = {
 	.attach = lvds_encoder_attach,
+	.input_formats = lvds_encoder_input_formats,
 };
 
 static int lvds_encoder_probe(struct platform_device *pdev)
@@ -39,6 +56,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	struct device_node *panel_node;
 	struct drm_panel *panel;
 	struct lvds_encoder *lvds_encoder;
+	int ret;
 
 	lvds_encoder = devm_kzalloc(&pdev->dev, sizeof(*lvds_encoder),
 				    GFP_KERNEL);
@@ -79,6 +97,12 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	if (IS_ERR(lvds_encoder->panel_bridge))
 		return PTR_ERR(lvds_encoder->panel_bridge);
 
+	ret = drm_of_bus_formats(pdev->dev.of_node, "interface-pix-fmt",
+				 &lvds_encoder->bus_formats);
+	if (ret < 0)
+		return ret;
+	lvds_encoder->num_bus_formats = ret;
+
 	/* The panel_bridge bridge is attached to the panel's of_node,
 	 * but we need a bridge attached to our of_node for our user
 	 * to look up.
@@ -96,6 +120,7 @@ static int lvds_encoder_remove(struct platform_device *pdev)
 {
 	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
 
+	kfree(lvds_encoder->bus_formats);
 	drm_bridge_remove(&lvds_encoder->bridge);
 
 	return 0;
-- 
2.11.0

Re: [PATCH v2 4/5] drm: bridge: lvds-encoder: allow specifying the input bus format

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4339 bytes --]

Hi Peter, Laurent,
   thanks for the patches,

On Mon, Mar 26, 2018 at 11:24:46PM +0200, Peter Rosin wrote:
> If the bridge changes the bus format, allow this to be described in
> the bridge, instead of providing false information about the bus
> format of the connector or panel.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../bindings/display/bridge/lvds-transmitter.txt   |  6 ++++++
>  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 ++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> index 50220190c203..8d40a2069252 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> @@ -30,6 +30,12 @@ Required properties:
>    device-specific version corresponding to the device first
>    followed by the generic version.
>
> +Optional properties:
> +
> +- interface-pix-fmt:
> +  List of valid input bus formats of the encoder. Recognized bus formats
> +  are listed in ../bus-format.txt
> +

This comments applies here and to [3/5] as well.

Are we sure we want the supported bridge input format defined in DT
bindings?

Again, I may be biased, but as I see this, each bridge driver should
list its supported formats with MEDIA_BUS_FMT_ fourcc codes and return
the currently 'active' one when requested by the preceding bridge.

I understand for this driver, being compatible with both a generic lvds
encoder and a specific chip, the supported input formats are
different, but I would have used the 'of_device_id.data' field for
this and leave this out from DT bindings.

This makes the translation routine here implemented not required if
I'm not wrong...

Thanks
   j

>  Required nodes:
>
>  This device has two video ports. Their connections are modeled using the OF
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index 75b0d3f6e4de..b78619b5560a 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -9,6 +9,7 @@
>
>  #include <drm/drmP.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>
>  #include <linux/of_graph.h>
> @@ -16,6 +17,8 @@
>  struct lvds_encoder {
>  	struct drm_bridge bridge;
>  	struct drm_bridge *panel_bridge;
> +	int num_bus_formats;
> +	u32 *bus_formats;
>  };
>
>  static int lvds_encoder_attach(struct drm_bridge *bridge)
> @@ -28,8 +31,22 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
>  				 bridge);
>  }
>
> +static int lvds_encoder_input_formats(struct drm_bridge *bridge,
> +				      const u32 **bus_formats)
> +{
> +	struct lvds_encoder *lvds_encoder = container_of(bridge,
> +							 struct lvds_encoder,
> +							 bridge);
> +
> +	if (lvds_encoder->num_bus_formats)
> +		*bus_formats = lvds_encoder->bus_formats;
> +
> +	return lvds_encoder->num_bus_formats;
> +}
> +
>  static struct drm_bridge_funcs funcs = {
>  	.attach = lvds_encoder_attach,
> +	.input_formats = lvds_encoder_input_formats,
>  };
>
>  static int lvds_encoder_probe(struct platform_device *pdev)
> @@ -39,6 +56,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>  	struct device_node *panel_node;
>  	struct drm_panel *panel;
>  	struct lvds_encoder *lvds_encoder;
> +	int ret;
>
>  	lvds_encoder = devm_kzalloc(&pdev->dev, sizeof(*lvds_encoder),
>  				    GFP_KERNEL);
> @@ -79,6 +97,12 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>  	if (IS_ERR(lvds_encoder->panel_bridge))
>  		return PTR_ERR(lvds_encoder->panel_bridge);
>
> +	ret = drm_of_bus_formats(pdev->dev.of_node, "interface-pix-fmt",
> +				 &lvds_encoder->bus_formats);
> +	if (ret < 0)
> +		return ret;
> +	lvds_encoder->num_bus_formats = ret;
> +
>  	/* The panel_bridge bridge is attached to the panel's of_node,
>  	 * but we need a bridge attached to our of_node for our user
>  	 * to look up.
> @@ -96,6 +120,7 @@ static int lvds_encoder_remove(struct platform_device *pdev)
>  {
>  	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
>
> +	kfree(lvds_encoder->bus_formats);
>  	drm_bridge_remove(&lvds_encoder->bridge);
>
>  	return 0;
> --
> 2.11.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 4/5] drm: bridge: lvds-encoder: allow specifying the input bus format

[Peter Rosin]

Hi Jacopo,

Thanks for you feedback!

On 2018-03-27 12:27, jacopo mondi wrote:
> Hi Peter, Laurent,
>    thanks for the patches,
> 
> On Mon, Mar 26, 2018 at 11:24:46PM +0200, Peter Rosin wrote:
>> If the bridge changes the bus format, allow this to be described in
>> the bridge, instead of providing false information about the bus
>> format of the connector or panel.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  .../bindings/display/bridge/lvds-transmitter.txt   |  6 ++++++
>>  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 ++++++++++++++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> index 50220190c203..8d40a2069252 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> @@ -30,6 +30,12 @@ Required properties:
>>    device-specific version corresponding to the device first
>>    followed by the generic version.
>>
>> +Optional properties:
>> +
>> +- interface-pix-fmt:
>> +  List of valid input bus formats of the encoder. Recognized bus formats
>> +  are listed in ../bus-format.txt
>> +
> 
> This comments applies here and to [3/5] as well.

I'm not sure how the below comments apply to [3/5]? I guess you are
suggesting that [3/5] should be dropped?

> Are we sure we want the supported bridge input format defined in DT
> bindings?

Yes, I'm pretty sure. In my case, it has to be specified manually
*somewhere* (at least I think it has to, but what do I know?). The
bridge is the best position, if you ask me.

> Again, I may be biased, but as I see this, each bridge driver should
> list its supported formats with MEDIA_BUS_FMT_ fourcc codes and return
> the currently 'active' one when requested by the preceding bridge.

In my case this is not possible, the bridge supports maximum rgb888
input, but since it cannot know how much of that is actually wired,
the actual format needs to be provided manually somewhere. In my
case, I know that I want to send rgb565 to the bridge. Sure, the
wiring between the encoder and the bridge chip could be seen as
another "bridge" that goes from rgb565 to rgb888, but adding some
extra layer to the model for this step seems like over-engineering
to me. Especially when the binding for the generic lvds-transmitter
bridge needs to specify the input format anyway. At least, that's
the right thing to do if you ask me. How else should it know what
format to request?

> I understand for this driver, being compatible with both a generic lvds
> encoder and a specific chip, the supported input formats are
> different, but I would have used the 'of_device_id.data' field for
> this and leave this out from DT bindings.

No, the lvds-transmitter accepts parallel input and spits out lvds,
just like the specific chip I'm using. I do not *need* to specify the
specific chip, and the driver does nothing special for it. It is just
good practice to specify the details when they are readily available.

> This makes the translation routine here implemented not required if
> I'm not wrong...

I think you are.

Cheers,
Peter

> Thanks
>    j
> 
>>  Required nodes:
>>
>>  This device has two video ports. Their connections are modeled using the OF
>> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
>> index 75b0d3f6e4de..b78619b5560a 100644
>> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
>> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
>> @@ -9,6 +9,7 @@
>>
>>  #include <drm/drmP.h>
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_of.h>
>>  #include <drm/drm_panel.h>
>>
>>  #include <linux/of_graph.h>
>> @@ -16,6 +17,8 @@
>>  struct lvds_encoder {
>>  	struct drm_bridge bridge;
>>  	struct drm_bridge *panel_bridge;
>> +	int num_bus_formats;
>> +	u32 *bus_formats;
>>  };
>>
>>  static int lvds_encoder_attach(struct drm_bridge *bridge)
>> @@ -28,8 +31,22 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
>>  				 bridge);
>>  }
>>
>> +static int lvds_encoder_input_formats(struct drm_bridge *bridge,
>> +				      const u32 **bus_formats)
>> +{
>> +	struct lvds_encoder *lvds_encoder = container_of(bridge,
>> +							 struct lvds_encoder,
>> +							 bridge);
>> +
>> +	if (lvds_encoder->num_bus_formats)
>> +		*bus_formats = lvds_encoder->bus_formats;
>> +
>> +	return lvds_encoder->num_bus_formats;
>> +}
>> +
>>  static struct drm_bridge_funcs funcs = {
>>  	.attach = lvds_encoder_attach,
>> +	.input_formats = lvds_encoder_input_formats,
>>  };
>>
>>  static int lvds_encoder_probe(struct platform_device *pdev)
>> @@ -39,6 +56,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>>  	struct device_node *panel_node;
>>  	struct drm_panel *panel;
>>  	struct lvds_encoder *lvds_encoder;
>> +	int ret;
>>
>>  	lvds_encoder = devm_kzalloc(&pdev->dev, sizeof(*lvds_encoder),
>>  				    GFP_KERNEL);
>> @@ -79,6 +97,12 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>>  	if (IS_ERR(lvds_encoder->panel_bridge))
>>  		return PTR_ERR(lvds_encoder->panel_bridge);
>>
>> +	ret = drm_of_bus_formats(pdev->dev.of_node, "interface-pix-fmt",
>> +				 &lvds_encoder->bus_formats);
>> +	if (ret < 0)
>> +		return ret;
>> +	lvds_encoder->num_bus_formats = ret;
>> +
>>  	/* The panel_bridge bridge is attached to the panel's of_node,
>>  	 * but we need a bridge attached to our of_node for our user
>>  	 * to look up.
>> @@ -96,6 +120,7 @@ static int lvds_encoder_remove(struct platform_device *pdev)
>>  {
>>  	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
>>
>> +	kfree(lvds_encoder->bus_formats);
>>  	drm_bridge_remove(&lvds_encoder->bridge);
>>
>>  	return 0;
>> --
>> 2.11.0
>>

Re: [PATCH v2 4/5] drm: bridge: lvds-encoder: allow specifying the input bus format

[Rob Herring]

On Mon, Mar 26, 2018 at 11:24:46PM +0200, Peter Rosin wrote:
> If the bridge changes the bus format, allow this to be described in
> the bridge, instead of providing false information about the bus
> format of the connector or panel.

I guess this is dead, but I'll comment here anyway because this issue 
has come up several times and I think something is needed.

> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../bindings/display/bridge/lvds-transmitter.txt   |  6 ++++++
>  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 ++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> index 50220190c203..8d40a2069252 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> @@ -30,6 +30,12 @@ Required properties:
>    device-specific version corresponding to the device first
>    followed by the generic version.
>  
> +Optional properties:
> +
> +- interface-pix-fmt:
> +  List of valid input bus formats of the encoder. Recognized bus formats
> +  are listed in ../bus-format.txt

We already have bus interface properties defined in 
media/video-interfaces.txt. It's the same problem though the bus/pixel 
formats are somewhat different. "bus-width" is probably enough to handle 
your particular case, but I'd imagine that could easily not be the case. 
If so, we should extend the existing binding rather than diverge with 
something new.

Rob

[PATCH v2 5/5] drm/atmel-hlcdc: take bridges into account when selecting output format

[Peter Rosin]

Bridges may affect the required bus output format of the encoder, in
which case it may be wrong to use the output format of the panel or
connector as is. So, examine if any of the intermediate bridges needs
specific bus formats (if there are intermediate bridges).

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 49 ++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index d73281095fac..920eb16c0213 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -226,6 +226,45 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
 #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
 #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
 
+/* We know that our connectors will only ever have one encoder. Find it. */
+static struct drm_encoder *atmel_hlcdc_encoder(struct drm_connector *connector)
+{
+	struct drm_encoder *encoder;
+	int i;
+
+	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+		if (!connector->encoder_ids[i])
+			break;
+
+		encoder = drm_encoder_find(connector->dev, NULL,
+					   connector->encoder_ids[i]);
+		if (encoder)
+			return encoder;
+	}
+
+	return NULL;
+}
+
+static int atmel_hlcdc_output_formats(struct drm_connector *connector,
+				      const u32 **bus_formats)
+{
+	struct drm_encoder *encoder = atmel_hlcdc_encoder(connector);
+	int num_bus_formats;
+
+	if (!encoder)
+		return 0;
+
+	if (encoder->bridge) {
+		num_bus_formats = drm_bridge_input_formats(encoder->bridge,
+							   bus_formats);
+		if (num_bus_formats)
+			return num_bus_formats;
+	}
+
+	*bus_formats = connector->display_info.bus_formats;
+	return connector->display_info.num_bus_formats;
+}
+
 static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 {
 	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
@@ -238,15 +277,19 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
 
 	for_each_new_connector_in_state(state->state, connector, cstate, i) {
-		struct drm_display_info *info = &connector->display_info;
+		int num_bus_formats;
+		const u32 *bus_formats;
 		unsigned int supported_fmts = 0;
 		int j;
 
+		num_bus_formats = atmel_hlcdc_output_formats(connector,
+							     &bus_formats);
+
 		if (!cstate->crtc)
 			continue;
 
-		for (j = 0; j < info->num_bus_formats; j++) {
-			switch (info->bus_formats[j]) {
+		for (j = 0; j < num_bus_formats; j++) {
+			switch (bus_formats[j]) {
 			case MEDIA_BUS_FMT_RGB444_1X12:
 				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
 				break;
-- 
2.11.0

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Daniel Vetter]

On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
> Hi!
> 
> [I got to v2 sooner than expected]
> 
> I have an Atmel sama5d31 hooked up to an lvds encoder and then
> on to an lvds panel. Which seems like something that has been
> done one or two times before...
> 
> The problem is that the bus_format of the SoC and the panel do
> not agree. The SoC driver (atmel-hlcdc) can handle the
> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
> wired for the rgb565 case. The lvds encoder supports rgb888 on
> its input side with the LSB wires for each color simply pulled
> down internally in the encoder in my case which means that the
> rgb565 bus_format is the format that works best. And the panel
> is expecting lvds (vesa-24), which is what the encoder outputs.
> 
> The reason I "blame" the bus_format of the drm_connector is that
> with the below DT snippet, things do not work *exactly* due to
> that. At least, it starts to work if I hack the panel-lvds driver
> to report the rgb565 bus_format instead of vesa-24.
> 
> 	panel: panel {
> 		compatible = "panel-lvds";
> 
> 		width-mm = <304>;
> 		height-mm = <228;
> 
> 		data-mapping = "vesa-24";
> 
> 		panel-timing {
> 			// 1024x768 @ 60Hz (typical)
> 			clock-frequency = <52140000 65000000 71100000>;
> 			hactive = <1024>;
> 			vactive = <768>;
> 			hfront-porch = <48 88 88>;
> 			hback-porch = <96 168 168>;
> 			hsync-len = <32 64 64>;
> 			vfront-porch = <8 13 14>;
> 			vback-porch = <8 13 14>;
> 			vsync-len = <8 12 14>;
> 		};
> 
> 		port {
> 			panel_input: endpoint {
> 				remote-endpoint = <&lvds_encoder_output>;
> 			};
> 		};
> 	};
> 
> 	lvds-encoder {
> 		compatible = "ti,ds90c185", "lvds-encoder";
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				reg = <0>;
> 
> 				lvds_encoder_input: endpoint {
> 					remote-endpoint = <&hlcdc_output>;
> 				};
> 			};
> 
> 			port@1 {
> 				reg = <1>;
> 
> 				lvds_encoder_output: endpoint {
> 					remote-endpoint = <&panel_input>;
> 				};
> 			};
> 		};
> 	};
> 
> But, instead of perverting the panel-lvds driver with support
> for a totally fake non-lvds bus_format, I intruduce an API that allows
> display controller drivers to query the required bus_format of any
> intermediate bridges, and match up with that instead of the formats
> given by the drm_connector. I trigger this with this addition to the
> lvds-encoder DT node:
> 
> 		interface-pix-fmt = "rgb565";
> 
> Naming is hard though, so I'm not sure if that's good?
> 
> I threw in the first patch, since that is the actual lvds encoder
> I have in this case.
> 
> Suggestions welcome.

Took a quick look, feels rather un-atomic. And there's beend discussing
for other bridge related state that we might want to track (like the full
adjusted_mode that might need to be adjusted at each stage in the chain).
So here's my suggestions:

- Add an optional per-bridge internal state struct using the support in

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-private-state

  Yes it says "driver private", but since bridge is just helper stuff
  that's all included. "driver private" == "not exposed as uapi" here.
  Include all the usual convenience wrappers to get at the state for a
  bridge.

- Then stuff your bus_format into that new drm_bridge_state struct.

- Add a new bridge callback atomic_check, which gets that bridge state as
  parameter (similar to all the other atomic_check functions).

This way we can even handle the bus_format dynamically, through the atomic
framework your bridge's atomic_check callback can look at the entire
atomic state (both up and down the chain if needed), it all neatly fits
into atomic overall and it's much easier to extend.

Please also cc Laurent Pinchart on this.

Cheers, Daniel


> 
> Cheers,
> Peter
> 
> Changes since v1 https://lkml.org/lkml/2018/3/17/221
> - Add a proper bridge API to query the bus_format instead of abusing
>   the ->get_modes part of the code. This is cleaner but requires
>   changes to all display controller drivers wishing to participate.
> - Add patch to adjust the atmel-hlcdc driver according to the above.
> - Hook the new info into the bridge local to the lvds-encoder instead
>   of messing about with new interfaces for the panel-bridge driver.
> - Add patch with a DT parsing function of bus_formats in a central place.
> - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.
> 
> Peter Rosin (5):
>   dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
>   drm: bridge: add API to query the expected input formats of bridges
>   drm: of: add display bus-format parser
>   drm: bridge: lvds-encoder: allow specifying the input bus format
>   drm/atmel-hlcdc: take bridges into account when selecting output
>     format
> 
>  .../bindings/display/bridge/lvds-transmitter.txt   | 14 ++++-
>  .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 49 ++++++++++++++++--
>  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 +++++++++
>  drivers/gpu/drm/drm_bridge.c                       | 32 ++++++++++++
>  drivers/gpu/drm/drm_of.c                           | 59 ++++++++++++++++++++++
>  include/drm/drm_bridge.h                           | 18 +++++++
>  include/drm/drm_of.h                               |  9 ++++
>  8 files changed, 237 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bus-format.txt
> 
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Laurent Pinchart]

Hi Daniel,

On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
> > Hi!
> > 
> > [I got to v2 sooner than expected]
> > 
> > I have an Atmel sama5d31 hooked up to an lvds encoder and then
> > on to an lvds panel. Which seems like something that has been
> > done one or two times before...
> > 
> > The problem is that the bus_format of the SoC and the panel do
> > not agree. The SoC driver (atmel-hlcdc) can handle the
> > rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
> > wired for the rgb565 case. The lvds encoder supports rgb888 on
> > its input side with the LSB wires for each color simply pulled
> > down internally in the encoder in my case which means that the
> > rgb565 bus_format is the format that works best. And the panel
> > is expecting lvds (vesa-24), which is what the encoder outputs.
> > 
> > The reason I "blame" the bus_format of the drm_connector is that
> > with the below DT snippet, things do not work *exactly* due to
> > that. At least, it starts to work if I hack the panel-lvds driver
> > to report the rgb565 bus_format instead of vesa-24.
> > 
> > 	panel: panel {
> > 		compatible = "panel-lvds";
> > 		
> > 		width-mm = <304>;
> > 		height-mm = <228;
> > 		
> > 		data-mapping = "vesa-24";
> > 		
> > 		panel-timing {
> > 			// 1024x768 @ 60Hz (typical)
> > 			clock-frequency = <52140000 65000000 71100000>;
> > 			hactive = <1024>;
> > 			vactive = <768>;
> > 			hfront-porch = <48 88 88>;
> > 			hback-porch = <96 168 168>;
> > 			hsync-len = <32 64 64>;
> > 			vfront-porch = <8 13 14>;
> > 			vback-porch = <8 13 14>;
> > 			vsync-len = <8 12 14>;
> > 		};
> > 		
> > 		port {
> > 			panel_input: endpoint {
> > 				remote-endpoint = <&lvds_encoder_output>;
> > 			};
> > 		};
> > 	};
> > 	
> > 	lvds-encoder {
> > 		compatible = "ti,ds90c185", "lvds-encoder";
> > 		
> > 		ports {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			
> > 			port@0 {
> > 				reg = <0>;
> > 				
> > 				lvds_encoder_input: endpoint {
> > 					remote-endpoint = <&hlcdc_output>;
> > 				};
> > 			};
> > 			
> > 			port@1 {
> > 				reg = <1>;
> > 				
> > 				lvds_encoder_output: endpoint {
> > 					remote-endpoint = <&panel_input>;
> > 				};
> > 			};
> > 		};
> > 	};
> > 
> > But, instead of perverting the panel-lvds driver with support
> > for a totally fake non-lvds bus_format, I intruduce an API that allows
> > display controller drivers to query the required bus_format of any
> > intermediate bridges, and match up with that instead of the formats
> > given by the drm_connector. I trigger this with this addition to the
> > 
> > lvds-encoder DT node:
> > 		interface-pix-fmt = "rgb565";
> > 
> > Naming is hard though, so I'm not sure if that's good?
> > 
> > I threw in the first patch, since that is the actual lvds encoder
> > I have in this case.
> > 
> > Suggestions welcome.
> 
> Took a quick look, feels rather un-atomic. And there's beend discussing
> for other bridge related state that we might want to track (like the full
> adjusted_mode that might need to be adjusted at each stage in the chain).
> So here's my suggestions:
> 
> - Add an optional per-bridge internal state struct using the support in
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-privat
> e-state
> 
>   Yes it says "driver private", but since bridge is just helper stuff
>   that's all included. "driver private" == "not exposed as uapi" here.
>   Include all the usual convenience wrappers to get at the state for a
>   bridge.
> 
> - Then stuff your bus_format into that new drm_bridge_state struct.
> 
> - Add a new bridge callback atomic_check, which gets that bridge state as
>   parameter (similar to all the other atomic_check functions).
> 
> This way we can even handle the bus_format dynamically, through the atomic
> framework your bridge's atomic_check callback can look at the entire
> atomic state (both up and down the chain if needed), it all neatly fits
> into atomic overall and it's much easier to extend.

While I think we'll eventually need bridge states, I don't think that's need 
yet. The bus formats reported by this patch series are static. We're not 
talking about the currently configured format for a bridge, but about the list 
of supported formats. This is similar to the bus_formats field present in the 
drm_display_info structure. There is thus in my opinion no need to interface 
this with atomic until we need to track the current format (and I think that 
will indeed happen at some point, but I don't think Peter needs this feature 
for now). That's why I've told Peter that I would like a bridge API to report 
the information and haven't requested a state-based implementation.

> Please also cc Laurent Pinchart on this.
> 
> > Changes since v1 https://lkml.org/lkml/2018/3/17/221
> > - Add a proper bridge API to query the bus_format instead of abusing
> >   the ->get_modes part of the code. This is cleaner but requires
> >   changes to all display controller drivers wishing to participate.
> > - Add patch to adjust the atmel-hlcdc driver according to the above.
> > - Hook the new info into the bridge local to the lvds-encoder instead
> >   of messing about with new interfaces for the panel-bridge driver.
> > - Add patch with a DT parsing function of bus_formats in a central place.
> > - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.
> > 
> > Peter Rosin (5):
> >   dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
> >   drm: bridge: add API to query the expected input formats of bridges
> >   drm: of: add display bus-format parser
> >   drm: bridge: lvds-encoder: allow specifying the input bus format
> >   drm/atmel-hlcdc: take bridges into account when selecting output
> >     format
> >  
> >  .../bindings/display/bridge/lvds-transmitter.txt   | 14 ++++-
> >  .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 49 +++++++++++++++--
> >  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 +++++++++
> >  drivers/gpu/drm/drm_bridge.c                       | 32 ++++++++++++
> >  drivers/gpu/drm/drm_of.c                           | 59 +++++++++++++++++
> >  include/drm/drm_bridge.h                           | 18 +++++++
> >  include/drm/drm_of.h                               |  9 ++++
> >  8 files changed, 237 insertions(+), 4 deletions(-)
> >  create mode 100644
> >  Documentation/devicetree/bindings/display/bus-format.txt

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Laurent Pinchart]

Hi again,

On Wednesday, 4 April 2018 01:28:29 EEST Laurent Pinchart wrote:
> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
> > On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
> > > Hi!
> > > 
> > > [I got to v2 sooner than expected]
> > > 
> > > I have an Atmel sama5d31 hooked up to an lvds encoder and then
> > > on to an lvds panel. Which seems like something that has been
> > > done one or two times before...
> > > 
> > > The problem is that the bus_format of the SoC and the panel do
> > > not agree. The SoC driver (atmel-hlcdc) can handle the
> > > rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
> > > wired for the rgb565 case. The lvds encoder supports rgb888 on
> > > its input side with the LSB wires for each color simply pulled
> > > down internally in the encoder in my case which means that the
> > > rgb565 bus_format is the format that works best. And the panel
> > > is expecting lvds (vesa-24), which is what the encoder outputs.
> > > 
> > > The reason I "blame" the bus_format of the drm_connector is that
> > > with the below DT snippet, things do not work *exactly* due to
> > > that. At least, it starts to work if I hack the panel-lvds driver
> > > to report the rgb565 bus_format instead of vesa-24.
> > > 
> > > 	panel: panel {
> > > 		compatible = "panel-lvds";
> > > 		
> > > 		width-mm = <304>;
> > > 		height-mm = <228;
> > > 		
> > > 		data-mapping = "vesa-24";
> > > 		
> > > 		panel-timing {
> > > 			// 1024x768 @ 60Hz (typical)
> > > 			clock-frequency = <52140000 65000000 71100000>;
> > > 			hactive = <1024>;
> > > 			vactive = <768>;
> > > 			hfront-porch = <48 88 88>;
> > > 			hback-porch = <96 168 168>;
> > > 			hsync-len = <32 64 64>;
> > > 			vfront-porch = <8 13 14>;
> > > 			vback-porch = <8 13 14>;
> > > 			vsync-len = <8 12 14>;
> > > 		};
> > > 		
> > > 		port {
> > > 			panel_input: endpoint {
> > > 				remote-endpoint = <&lvds_encoder_output>;
> > > 			};
> > > 		};
> > > 	};
> > > 	
> > > 	lvds-encoder {
> > > 		compatible = "ti,ds90c185", "lvds-encoder";
> > > 		
> > > 		ports {
> > > 			#address-cells = <1>;
> > > 			#size-cells = <0>;
> > > 			
> > > 			port@0 {
> > > 				reg = <0>;
> > > 				
> > > 				lvds_encoder_input: endpoint {
> > > 					remote-endpoint = <&hlcdc_output>;
> > > 				};
> > > 			};
> > > 			
> > > 			port@1 {
> > > 				reg = <1>;
> > > 				
> > > 				lvds_encoder_output: endpoint {
> > > 					remote-endpoint = <&panel_input>;
> > > 				};
> > > 			};
> > > 		};
> > > 	};
> > > 
> > > But, instead of perverting the panel-lvds driver with support
> > > for a totally fake non-lvds bus_format, I intruduce an API that allows
> > > display controller drivers to query the required bus_format of any
> > > intermediate bridges, and match up with that instead of the formats
> > > given by the drm_connector. I trigger this with this addition to the
> > > 
> > > lvds-encoder DT node:
> > > 		interface-pix-fmt = "rgb565";
> > > 
> > > Naming is hard though, so I'm not sure if that's good?
> > > 
> > > I threw in the first patch, since that is the actual lvds encoder
> > > I have in this case.
> > > 
> > > Suggestions welcome.
> > 
> > Took a quick look, feels rather un-atomic. And there's beend discussing
> > for other bridge related state that we might want to track (like the full
> > adjusted_mode that might need to be adjusted at each stage in the chain).
> > So here's my suggestions:
> > 
> > - Add an optional per-bridge internal state struct using the support in
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-priv
> > at e-state
> > 
> >   Yes it says "driver private", but since bridge is just helper stuff
> >   that's all included. "driver private" == "not exposed as uapi" here.
> >   Include all the usual convenience wrappers to get at the state for a
> >   bridge.
> > 
> > - Then stuff your bus_format into that new drm_bridge_state struct.
> > 
> > - Add a new bridge callback atomic_check, which gets that bridge state as
> >   parameter (similar to all the other atomic_check functions).
> > 
> > This way we can even handle the bus_format dynamically, through the atomic
> > framework your bridge's atomic_check callback can look at the entire
> > atomic state (both up and down the chain if needed), it all neatly fits
> > into atomic overall and it's much easier to extend.
> 
> While I think we'll eventually need bridge states, I don't think that's need
> yet. The bus formats reported by this patch series are static. We're not
> talking about the currently configured format for a bridge, but about the
> list of supported formats. This is similar to the bus_formats field present
> in the drm_display_info structure. There is thus in my opinion no need to
> interface this with atomic until we need to track the current format (and I
> think that will indeed happen at some point, but I don't think Peter needs
> this feature for now). That's why I've told Peter that I would like a
> bridge API to report the information and haven't requested a state-based
> implementation.

Reading patch 5/5 I fear this might not be so simple. I'll sleep over it and 
try to comment tomorrow.

> > Please also cc Laurent Pinchart on this.
> > 
> > > Changes since v1 https://lkml.org/lkml/2018/3/17/221
> > > - Add a proper bridge API to query the bus_format instead of abusing
> > >   the ->get_modes part of the code. This is cleaner but requires
> > >   changes to all display controller drivers wishing to participate.
> > > - Add patch to adjust the atmel-hlcdc driver according to the above.
> > > - Hook the new info into the bridge local to the lvds-encoder instead
> > >   of messing about with new interfaces for the panel-bridge driver.
> > > - Add patch with a DT parsing function of bus_formats in a central
> > >   place.
> > > - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.
> > > 
> > > Peter Rosin (5):
> > >   dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
> > >   drm: bridge: add API to query the expected input formats of bridges
> > >   drm: of: add display bus-format parser
> > >   drm: bridge: lvds-encoder: allow specifying the input bus format
> > >   drm/atmel-hlcdc: take bridges into account when selecting output
> > >     format
> > >  
> > >  .../bindings/display/bridge/lvds-transmitter.txt   | 14 ++++-
> > >  .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
> > >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 49 +++++++++++++--
> > >  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 +++++++++
> > >  drivers/gpu/drm/drm_bridge.c                       | 32 ++++++++++++
> > >  drivers/gpu/drm/drm_of.c                           | 59 +++++++++++++++
> > >  include/drm/drm_bridge.h                           | 18 +++++++
> > >  include/drm/drm_of.h                               |  9 ++++
> > >  8 files changed, 237 insertions(+), 4 deletions(-)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/display/bus-format.txt

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Daniel Vetter]

On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Daniel,
>
> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
>> > Hi!
>> >
>> > [I got to v2 sooner than expected]
>> >
>> > I have an Atmel sama5d31 hooked up to an lvds encoder and then
>> > on to an lvds panel. Which seems like something that has been
>> > done one or two times before...
>> >
>> > The problem is that the bus_format of the SoC and the panel do
>> > not agree. The SoC driver (atmel-hlcdc) can handle the
>> > rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
>> > wired for the rgb565 case. The lvds encoder supports rgb888 on
>> > its input side with the LSB wires for each color simply pulled
>> > down internally in the encoder in my case which means that the
>> > rgb565 bus_format is the format that works best. And the panel
>> > is expecting lvds (vesa-24), which is what the encoder outputs.
>> >
>> > The reason I "blame" the bus_format of the drm_connector is that
>> > with the below DT snippet, things do not work *exactly* due to
>> > that. At least, it starts to work if I hack the panel-lvds driver
>> > to report the rgb565 bus_format instead of vesa-24.
>> >
>> >     panel: panel {
>> >             compatible = "panel-lvds";
>> >
>> >             width-mm = <304>;
>> >             height-mm = <228;
>> >
>> >             data-mapping = "vesa-24";
>> >
>> >             panel-timing {
>> >                     // 1024x768 @ 60Hz (typical)
>> >                     clock-frequency = <52140000 65000000 71100000>;
>> >                     hactive = <1024>;
>> >                     vactive = <768>;
>> >                     hfront-porch = <48 88 88>;
>> >                     hback-porch = <96 168 168>;
>> >                     hsync-len = <32 64 64>;
>> >                     vfront-porch = <8 13 14>;
>> >                     vback-porch = <8 13 14>;
>> >                     vsync-len = <8 12 14>;
>> >             };
>> >
>> >             port {
>> >                     panel_input: endpoint {
>> >                             remote-endpoint = <&lvds_encoder_output>;
>> >                     };
>> >             };
>> >     };
>> >
>> >     lvds-encoder {
>> >             compatible = "ti,ds90c185", "lvds-encoder";
>> >
>> >             ports {
>> >                     #address-cells = <1>;
>> >                     #size-cells = <0>;
>> >
>> >                     port@0 {
>> >                             reg = <0>;
>> >
>> >                             lvds_encoder_input: endpoint {
>> >                                     remote-endpoint = <&hlcdc_output>;
>> >                             };
>> >                     };
>> >
>> >                     port@1 {
>> >                             reg = <1>;
>> >
>> >                             lvds_encoder_output: endpoint {
>> >                                     remote-endpoint = <&panel_input>;
>> >                             };
>> >                     };
>> >             };
>> >     };
>> >
>> > But, instead of perverting the panel-lvds driver with support
>> > for a totally fake non-lvds bus_format, I intruduce an API that allows
>> > display controller drivers to query the required bus_format of any
>> > intermediate bridges, and match up with that instead of the formats
>> > given by the drm_connector. I trigger this with this addition to the
>> >
>> > lvds-encoder DT node:
>> >             interface-pix-fmt = "rgb565";
>> >
>> > Naming is hard though, so I'm not sure if that's good?
>> >
>> > I threw in the first patch, since that is the actual lvds encoder
>> > I have in this case.
>> >
>> > Suggestions welcome.
>>
>> Took a quick look, feels rather un-atomic. And there's beend discussing
>> for other bridge related state that we might want to track (like the full
>> adjusted_mode that might need to be adjusted at each stage in the chain).
>> So here's my suggestions:
>>
>> - Add an optional per-bridge internal state struct using the support in
>>
>> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-privat
>> e-state
>>
>>   Yes it says "driver private", but since bridge is just helper stuff
>>   that's all included. "driver private" == "not exposed as uapi" here.
>>   Include all the usual convenience wrappers to get at the state for a
>>   bridge.
>>
>> - Then stuff your bus_format into that new drm_bridge_state struct.
>>
>> - Add a new bridge callback atomic_check, which gets that bridge state as
>>   parameter (similar to all the other atomic_check functions).
>>
>> This way we can even handle the bus_format dynamically, through the atomic
>> framework your bridge's atomic_check callback can look at the entire
>> atomic state (both up and down the chain if needed), it all neatly fits
>> into atomic overall and it's much easier to extend.
>
> While I think we'll eventually need bridge states, I don't think that's need
> yet. The bus formats reported by this patch series are static. We're not
> talking about the currently configured format for a bridge, but about the list
> of supported formats. This is similar to the bus_formats field present in the
> drm_display_info structure. There is thus in my opinion no need to interface
> this with atomic until we need to track the current format (and I think that
> will indeed happen at some point, but I don't think Peter needs this feature
> for now). That's why I've told Peter that I would like a bridge API to report
> the information and haven't requested a state-based implementation.

If it's static, why a dynamic callback? Just add it to struct
drm_bridge, require it's filled out before registering the bridge,
done.
-Daniel

>
>> Please also cc Laurent Pinchart on this.
>>
>> > Changes since v1 https://lkml.org/lkml/2018/3/17/221
>> > - Add a proper bridge API to query the bus_format instead of abusing
>> >   the ->get_modes part of the code. This is cleaner but requires
>> >   changes to all display controller drivers wishing to participate.
>> > - Add patch to adjust the atmel-hlcdc driver according to the above.
>> > - Hook the new info into the bridge local to the lvds-encoder instead
>> >   of messing about with new interfaces for the panel-bridge driver.
>> > - Add patch with a DT parsing function of bus_formats in a central place.
>> > - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.
>> >
>> > Peter Rosin (5):
>> >   dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
>> >   drm: bridge: add API to query the expected input formats of bridges
>> >   drm: of: add display bus-format parser
>> >   drm: bridge: lvds-encoder: allow specifying the input bus format
>> >   drm/atmel-hlcdc: take bridges into account when selecting output
>> >     format
>> >
>> >  .../bindings/display/bridge/lvds-transmitter.txt   | 14 ++++-
>> >  .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
>> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 49 +++++++++++++++--
>> >  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 +++++++++
>> >  drivers/gpu/drm/drm_bridge.c                       | 32 ++++++++++++
>> >  drivers/gpu/drm/drm_of.c                           | 59 +++++++++++++++++
>> >  include/drm/drm_bridge.h                           | 18 +++++++
>> >  include/drm/drm_of.h                               |  9 ++++
>> >  8 files changed, 237 insertions(+), 4 deletions(-)
>> >  create mode 100644
>> >  Documentation/devicetree/bindings/display/bus-format.txt
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Laurent Pinchart]

Hi Daniel,

On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
> > On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
> >> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
> >>> Hi!
> >>> 
> >>> [I got to v2 sooner than expected]
> >>> 
> >>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
> >>> on to an lvds panel. Which seems like something that has been
> >>> done one or two times before...
> >>> 
> >>> The problem is that the bus_format of the SoC and the panel do
> >>> not agree. The SoC driver (atmel-hlcdc) can handle the
> >>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
> >>> wired for the rgb565 case. The lvds encoder supports rgb888 on
> >>> its input side with the LSB wires for each color simply pulled
> >>> down internally in the encoder in my case which means that the
> >>> rgb565 bus_format is the format that works best. And the panel
> >>> is expecting lvds (vesa-24), which is what the encoder outputs.
> >>> 
> >>> The reason I "blame" the bus_format of the drm_connector is that
> >>> with the below DT snippet, things do not work *exactly* due to
> >>> that. At least, it starts to work if I hack the panel-lvds driver
> >>> to report the rgb565 bus_format instead of vesa-24.
> >>> 
> >>>     panel: panel {
> >>>             compatible = "panel-lvds";
> >>>             
> >>>             width-mm = <304>;
> >>>             height-mm = <228;
> >>>             
> >>>             data-mapping = "vesa-24";
> >>>             
> >>>             panel-timing {
> >>>                     // 1024x768 @ 60Hz (typical)
> >>>                     clock-frequency = <52140000 65000000 71100000>;
> >>>                     hactive = <1024>;
> >>>                     vactive = <768>;
> >>>                     hfront-porch = <48 88 88>;
> >>>                     hback-porch = <96 168 168>;
> >>>                     hsync-len = <32 64 64>;
> >>>                     vfront-porch = <8 13 14>;
> >>>                     vback-porch = <8 13 14>;
> >>>                     vsync-len = <8 12 14>;
> >>>             };
> >>>             
> >>>             port {
> >>>                     panel_input: endpoint {
> >>>                             remote-endpoint = <&lvds_encoder_output>;
> >>>                     };
> >>>             };
> >>>     };
> >>>     
> >>>     lvds-encoder {
> >>>             compatible = "ti,ds90c185", "lvds-encoder";
> >>>             
> >>>             ports {
> >>>                     #address-cells = <1>;
> >>>                     #size-cells = <0>;
> >>>                     
> >>>                     port@0 {
> >>>                             reg = <0>;
> >>>                             
> >>>                             lvds_encoder_input: endpoint {
> >>>                                     remote-endpoint = <&hlcdc_output>;
> >>>                             };
> >>>                     };
> >>>                     
> >>>                     port@1 {
> >>>                             reg = <1>;
> >>>                             
> >>>                             lvds_encoder_output: endpoint {
> >>>                                     remote-endpoint = <&panel_input>;
> >>>                             };
> >>>                     };
> >>>             };
> >>>     };
> >>> 
> >>> But, instead of perverting the panel-lvds driver with support
> >>> for a totally fake non-lvds bus_format, I intruduce an API that allows
> >>> display controller drivers to query the required bus_format of any
> >>> intermediate bridges, and match up with that instead of the formats
> >>> given by the drm_connector. I trigger this with this addition to the
> >>> 
> >>> lvds-encoder DT node:
> >>>             interface-pix-fmt = "rgb565";
> >>> 
> >>> Naming is hard though, so I'm not sure if that's good?
> >>> 
> >>> I threw in the first patch, since that is the actual lvds encoder
> >>> I have in this case.
> >>> 
> >>> Suggestions welcome.
> >> 
> >> Took a quick look, feels rather un-atomic. And there's beend discussing
> >> for other bridge related state that we might want to track (like the full
> >> adjusted_mode that might need to be adjusted at each stage in the chain).
> >> So here's my suggestions:
> >> 
> >> - Add an optional per-bridge internal state struct using the support in
> >>   https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-> >>   private-state
> >> 
> >>   Yes it says "driver private", but since bridge is just helper stuff
> >>   that's all included. "driver private" == "not exposed as uapi" here.
> >>   Include all the usual convenience wrappers to get at the state for a
> >>   bridge.
> >> 
> >> - Then stuff your bus_format into that new drm_bridge_state struct.
> >> 
> >> - Add a new bridge callback atomic_check, which gets that bridge state as
> >>   parameter (similar to all the other atomic_check functions).
> >> 
> >> This way we can even handle the bus_format dynamically, through the
> >> atomic framework your bridge's atomic_check callback can look at the
> >> entire atomic state (both up and down the chain if needed), it all neatly
> >> fits into atomic overall and it's much easier to extend.
> > 
> > While I think we'll eventually need bridge states, I don't think that's
> > need yet. The bus formats reported by this patch series are static. We're
> > not talking about the currently configured format for a bridge, but about
> > the list of supported formats. This is similar to the bus_formats field
> > present in the drm_display_info structure. There is thus in my opinion no
> > need to interface this with atomic until we need to track the current
> > format (and I think that will indeed happen at some point, but I don't
> > think Peter needs this feature for now). That's why I've told Peter that
> > I would like a bridge API to report the information and haven't requested
> > a state-based implementation.
> 
> If it's static, why a dynamic callback? Just add it to struct
> drm_bridge, require it's filled out before registering the bridge,
> done.

If I remember correctly I mentioned both options in my initial proposal, 
without a personal preference. A new field in struct drm_bridge would 
certainly work for me.

> >> Please also cc Laurent Pinchart on this.
> >> 
> >>> Changes since v1 https://lkml.org/lkml/2018/3/17/221
> >>> - Add a proper bridge API to query the bus_format instead of abusing
> >>>   the ->get_modes part of the code. This is cleaner but requires
> >>>   changes to all display controller drivers wishing to participate.
> >>> - Add patch to adjust the atmel-hlcdc driver according to the above.
> >>> - Hook the new info into the bridge local to the lvds-encoder instead
> >>>   of messing about with new interfaces for the panel-bridge driver.
> >>> - Add patch with a DT parsing function of bus_formats in a central
> >>>   place.
> >>> - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.
> >>> 
> >>> Peter Rosin (5):
> >>>   dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
> >>>   drm: bridge: add API to query the expected input formats of bridges
> >>>   drm: of: add display bus-format parser
> >>>   drm: bridge: lvds-encoder: allow specifying the input bus format
> >>>   drm/atmel-hlcdc: take bridges into account when selecting output
> >>>     format
> >>>  
> >>>  .../bindings/display/bridge/lvds-transmitter.txt   | 14 ++++-
> >>>  .../devicetree/bindings/display/bus-format.txt     | 35 +++++++++++++
> >>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 49 +++++++++++++--
> >>>  drivers/gpu/drm/bridge/lvds-encoder.c              | 25 +++++++++
> >>>  drivers/gpu/drm/drm_bridge.c                       | 32 ++++++++++++
> >>>  drivers/gpu/drm/drm_of.c                           | 59 +++++++++++++++
> >>>  include/drm/drm_bridge.h                           | 18 +++++++
> >>>  include/drm/drm_of.h                               |  9 ++++
> >>>  8 files changed, 237 insertions(+), 4 deletions(-)
> >>>  create mode 100644
> >>>  Documentation/devicetree/bindings/display/bus-format.txt

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Peter Rosin]

On 2018-04-04 11:07, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
>> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
>>> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
>>>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> [I got to v2 sooner than expected]
>>>>>
>>>>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
>>>>> on to an lvds panel. Which seems like something that has been
>>>>> done one or two times before...
>>>>>
>>>>> The problem is that the bus_format of the SoC and the panel do
>>>>> not agree. The SoC driver (atmel-hlcdc) can handle the
>>>>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
>>>>> wired for the rgb565 case. The lvds encoder supports rgb888 on
>>>>> its input side with the LSB wires for each color simply pulled
>>>>> down internally in the encoder in my case which means that the
>>>>> rgb565 bus_format is the format that works best. And the panel
>>>>> is expecting lvds (vesa-24), which is what the encoder outputs.
>>>>>
>>>>> The reason I "blame" the bus_format of the drm_connector is that
>>>>> with the below DT snippet, things do not work *exactly* due to
>>>>> that. At least, it starts to work if I hack the panel-lvds driver
>>>>> to report the rgb565 bus_format instead of vesa-24.
>>>>>
>>>>>     panel: panel {
>>>>>             compatible = "panel-lvds";
>>>>>             
>>>>>             width-mm = <304>;
>>>>>             height-mm = <228;
>>>>>             
>>>>>             data-mapping = "vesa-24";
>>>>>             
>>>>>             panel-timing {
>>>>>                     // 1024x768 @ 60Hz (typical)
>>>>>                     clock-frequency = <52140000 65000000 71100000>;
>>>>>                     hactive = <1024>;
>>>>>                     vactive = <768>;
>>>>>                     hfront-porch = <48 88 88>;
>>>>>                     hback-porch = <96 168 168>;
>>>>>                     hsync-len = <32 64 64>;
>>>>>                     vfront-porch = <8 13 14>;
>>>>>                     vback-porch = <8 13 14>;
>>>>>                     vsync-len = <8 12 14>;
>>>>>             };
>>>>>             
>>>>>             port {
>>>>>                     panel_input: endpoint {
>>>>>                             remote-endpoint = <&lvds_encoder_output>;
>>>>>                     };
>>>>>             };
>>>>>     };
>>>>>     
>>>>>     lvds-encoder {
>>>>>             compatible = "ti,ds90c185", "lvds-encoder";
>>>>>             
>>>>>             ports {
>>>>>                     #address-cells = <1>;
>>>>>                     #size-cells = <0>;
>>>>>                     
>>>>>                     port@0 {
>>>>>                             reg = <0>;
>>>>>                             
>>>>>                             lvds_encoder_input: endpoint {
>>>>>                                     remote-endpoint = <&hlcdc_output>;
>>>>>                             };
>>>>>                     };
>>>>>                     
>>>>>                     port@1 {
>>>>>                             reg = <1>;
>>>>>                             
>>>>>                             lvds_encoder_output: endpoint {
>>>>>                                     remote-endpoint = <&panel_input>;
>>>>>                             };
>>>>>                     };
>>>>>             };
>>>>>     };
>>>>>
>>>>> But, instead of perverting the panel-lvds driver with support
>>>>> for a totally fake non-lvds bus_format, I intruduce an API that allows
>>>>> display controller drivers to query the required bus_format of any
>>>>> intermediate bridges, and match up with that instead of the formats
>>>>> given by the drm_connector. I trigger this with this addition to the
>>>>>
>>>>> lvds-encoder DT node:
>>>>>             interface-pix-fmt = "rgb565";
>>>>>
>>>>> Naming is hard though, so I'm not sure if that's good?
>>>>>
>>>>> I threw in the first patch, since that is the actual lvds encoder
>>>>> I have in this case.
>>>>>
>>>>> Suggestions welcome.
>>>>
>>>> Took a quick look, feels rather un-atomic. And there's beend discussing
>>>> for other bridge related state that we might want to track (like the full
>>>> adjusted_mode that might need to be adjusted at each stage in the chain).
>>>> So here's my suggestions:
>>>>
>>>> - Add an optional per-bridge internal state struct using the support in
>>>>   https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-> >>   private-state
>>>>
>>>>   Yes it says "driver private", but since bridge is just helper stuff
>>>>   that's all included. "driver private" == "not exposed as uapi" here.
>>>>   Include all the usual convenience wrappers to get at the state for a
>>>>   bridge.
>>>>
>>>> - Then stuff your bus_format into that new drm_bridge_state struct.
>>>>
>>>> - Add a new bridge callback atomic_check, which gets that bridge state as
>>>>   parameter (similar to all the other atomic_check functions).
>>>>
>>>> This way we can even handle the bus_format dynamically, through the
>>>> atomic framework your bridge's atomic_check callback can look at the
>>>> entire atomic state (both up and down the chain if needed), it all neatly
>>>> fits into atomic overall and it's much easier to extend.
>>>
>>> While I think we'll eventually need bridge states, I don't think that's
>>> need yet. The bus formats reported by this patch series are static. We're
>>> not talking about the currently configured format for a bridge, but about
>>> the list of supported formats. This is similar to the bus_formats field
>>> present in the drm_display_info structure. There is thus in my opinion no
>>> need to interface this with atomic until we need to track the current
>>> format (and I think that will indeed happen at some point, but I don't
>>> think Peter needs this feature for now). That's why I've told Peter that
>>> I would like a bridge API to report the information and haven't requested
>>> a state-based implementation.
>>
>> If it's static, why a dynamic callback? Just add it to struct
>> drm_bridge, require it's filled out before registering the bridge,
>> done.
> 
> If I remember correctly I mentioned both options in my initial proposal, 
> without a personal preference. A new field in struct drm_bridge would 
> certainly work for me.

You did. Ok, so v3 coming up...

Cheers,
Peter

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Peter Rosin]

On 2018-04-04 14:35, Peter Rosin wrote:
> On 2018-04-04 11:07, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
>>> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
>>>> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
>>>>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
>>>>>> Hi!
>>>>>>
>>>>>> [I got to v2 sooner than expected]
>>>>>>
>>>>>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
>>>>>> on to an lvds panel. Which seems like something that has been
>>>>>> done one or two times before...
>>>>>>
>>>>>> The problem is that the bus_format of the SoC and the panel do
>>>>>> not agree. The SoC driver (atmel-hlcdc) can handle the
>>>>>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
>>>>>> wired for the rgb565 case. The lvds encoder supports rgb888 on
>>>>>> its input side with the LSB wires for each color simply pulled
>>>>>> down internally in the encoder in my case which means that the
>>>>>> rgb565 bus_format is the format that works best. And the panel
>>>>>> is expecting lvds (vesa-24), which is what the encoder outputs.
>>>>>>
>>>>>> The reason I "blame" the bus_format of the drm_connector is that
>>>>>> with the below DT snippet, things do not work *exactly* due to
>>>>>> that. At least, it starts to work if I hack the panel-lvds driver
>>>>>> to report the rgb565 bus_format instead of vesa-24.
>>>>>>
>>>>>>     panel: panel {
>>>>>>             compatible = "panel-lvds";
>>>>>>             
>>>>>>             width-mm = <304>;
>>>>>>             height-mm = <228;
>>>>>>             
>>>>>>             data-mapping = "vesa-24";
>>>>>>             
>>>>>>             panel-timing {
>>>>>>                     // 1024x768 @ 60Hz (typical)
>>>>>>                     clock-frequency = <52140000 65000000 71100000>;
>>>>>>                     hactive = <1024>;
>>>>>>                     vactive = <768>;
>>>>>>                     hfront-porch = <48 88 88>;
>>>>>>                     hback-porch = <96 168 168>;
>>>>>>                     hsync-len = <32 64 64>;
>>>>>>                     vfront-porch = <8 13 14>;
>>>>>>                     vback-porch = <8 13 14>;
>>>>>>                     vsync-len = <8 12 14>;
>>>>>>             };
>>>>>>             
>>>>>>             port {
>>>>>>                     panel_input: endpoint {
>>>>>>                             remote-endpoint = <&lvds_encoder_output>;
>>>>>>                     };
>>>>>>             };
>>>>>>     };
>>>>>>     
>>>>>>     lvds-encoder {
>>>>>>             compatible = "ti,ds90c185", "lvds-encoder";
>>>>>>             
>>>>>>             ports {
>>>>>>                     #address-cells = <1>;
>>>>>>                     #size-cells = <0>;
>>>>>>                     
>>>>>>                     port@0 {
>>>>>>                             reg = <0>;
>>>>>>                             
>>>>>>                             lvds_encoder_input: endpoint {
>>>>>>                                     remote-endpoint = <&hlcdc_output>;
>>>>>>                             };
>>>>>>                     };
>>>>>>                     
>>>>>>                     port@1 {
>>>>>>                             reg = <1>;
>>>>>>                             
>>>>>>                             lvds_encoder_output: endpoint {
>>>>>>                                     remote-endpoint = <&panel_input>;
>>>>>>                             };
>>>>>>                     };
>>>>>>             };
>>>>>>     };
>>>>>>
>>>>>> But, instead of perverting the panel-lvds driver with support
>>>>>> for a totally fake non-lvds bus_format, I intruduce an API that allows
>>>>>> display controller drivers to query the required bus_format of any
>>>>>> intermediate bridges, and match up with that instead of the formats
>>>>>> given by the drm_connector. I trigger this with this addition to the
>>>>>>
>>>>>> lvds-encoder DT node:
>>>>>>             interface-pix-fmt = "rgb565";
>>>>>>
>>>>>> Naming is hard though, so I'm not sure if that's good?
>>>>>>
>>>>>> I threw in the first patch, since that is the actual lvds encoder
>>>>>> I have in this case.
>>>>>>
>>>>>> Suggestions welcome.
>>>>>
>>>>> Took a quick look, feels rather un-atomic. And there's beend discussing
>>>>> for other bridge related state that we might want to track (like the full
>>>>> adjusted_mode that might need to be adjusted at each stage in the chain).
>>>>> So here's my suggestions:
>>>>>
>>>>> - Add an optional per-bridge internal state struct using the support in
>>>>>   https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-> >>   private-state
>>>>>
>>>>>   Yes it says "driver private", but since bridge is just helper stuff
>>>>>   that's all included. "driver private" == "not exposed as uapi" here.
>>>>>   Include all the usual convenience wrappers to get at the state for a
>>>>>   bridge.
>>>>>
>>>>> - Then stuff your bus_format into that new drm_bridge_state struct.
>>>>>
>>>>> - Add a new bridge callback atomic_check, which gets that bridge state as
>>>>>   parameter (similar to all the other atomic_check functions).
>>>>>
>>>>> This way we can even handle the bus_format dynamically, through the
>>>>> atomic framework your bridge's atomic_check callback can look at the
>>>>> entire atomic state (both up and down the chain if needed), it all neatly
>>>>> fits into atomic overall and it's much easier to extend.
>>>>
>>>> While I think we'll eventually need bridge states, I don't think that's
>>>> need yet. The bus formats reported by this patch series are static. We're
>>>> not talking about the currently configured format for a bridge, but about
>>>> the list of supported formats. This is similar to the bus_formats field
>>>> present in the drm_display_info structure. There is thus in my opinion no
>>>> need to interface this with atomic until we need to track the current
>>>> format (and I think that will indeed happen at some point, but I don't
>>>> think Peter needs this feature for now). That's why I've told Peter that
>>>> I would like a bridge API to report the information and haven't requested
>>>> a state-based implementation.
>>>
>>> If it's static, why a dynamic callback? Just add it to struct
>>> drm_bridge, require it's filled out before registering the bridge,
>>> done.
>>
>> If I remember correctly I mentioned both options in my initial proposal, 
>> without a personal preference. A new field in struct drm_bridge would 
>> certainly work for me.
> 
> You did. Ok, so v3 coming up...

Nope, v3 is not coming up. This feels like an impossible mission for me, as
this changes core DRM design, and it would just be too much of a time sink for
me. Besides, the final paragraph of the nice long "state of bridges"-mail from
Laurent, i.e.

On 2018-04-04 15:07, Laurent Pinchart wrote:
> Finally, still regarding Peter's case, the decision to output RGB565 instead 
> of RGB888 (which the LVDS encoder expects) is due to PCB routing between the 
> display controller and the LVDS encoder. This isn't a property of the LVDS 
> encoder or the display controller, but of their hardware connection. This 
> patch series uses a DT property in the LVDS encoder DT node to convey that 
> information, but wouldn't it be equally correct (or incorrect :-)) to instead 
> use a DT property in the display controller DT node ?

hints at where I'm going. The reason is that I have a patch (that needs some
polish, will post soon) that makes the atmel-hlcdc driver use components in
order to hook it with the tda998x driver (an hdmi encoder), and there too
I need the atmel-hlcdc driver to use rgb565 output. And in that case there
are no bridges involved, so the proposed solution in this series has zero
hope of being adequate. It simply seems that forcing the output mode in the
atmel-hlcdc driver is what fixes the root cause.

End result; the only thing that survives this series is the interesting
discussion and patch 1/5. But I will include that patch in the alternative
solution, so you can drop this series entirely...

Cheers,
Peter

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Laurent Pinchart]

Hi Peter,

On Monday, 9 April 2018 10:04:05 EEST Peter Rosin wrote:
> On 2018-04-04 14:35, Peter Rosin wrote:
> > On 2018-04-04 11:07, Laurent Pinchart wrote:
> >> On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
> >>> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
> >>>> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
> >>>>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
> >>>>>> Hi!
> >>>>>> 
> >>>>>> [I got to v2 sooner than expected]
> >>>>>> 
> >>>>>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
> >>>>>> on to an lvds panel. Which seems like something that has been
> >>>>>> done one or two times before...
> >>>>>> 
> >>>>>> The problem is that the bus_format of the SoC and the panel do
> >>>>>> not agree. The SoC driver (atmel-hlcdc) can handle the
> >>>>>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
> >>>>>> wired for the rgb565 case. The lvds encoder supports rgb888 on
> >>>>>> its input side with the LSB wires for each color simply pulled
> >>>>>> down internally in the encoder in my case which means that the
> >>>>>> rgb565 bus_format is the format that works best. And the panel
> >>>>>> is expecting lvds (vesa-24), which is what the encoder outputs.
> >>>>>> 
> >>>>>> The reason I "blame" the bus_format of the drm_connector is that
> >>>>>> with the below DT snippet, things do not work *exactly* due to
> >>>>>> that. At least, it starts to work if I hack the panel-lvds driver
> >>>>>> to report the rgb565 bus_format instead of vesa-24.
> >>>>>> 
> >>>>>>     panel: panel {
> >>>>>>             compatible = "panel-lvds";
> >>>>>>             
> >>>>>>             width-mm = <304>;
> >>>>>>             height-mm = <228;
> >>>>>>             
> >>>>>>             data-mapping = "vesa-24";
> >>>>>>             
> >>>>>>             panel-timing {
> >>>>>>                     // 1024x768 @ 60Hz (typical)
> >>>>>>                     clock-frequency = <52140000 65000000 71100000>;
> >>>>>>                     hactive = <1024>;
> >>>>>>                     vactive = <768>;
> >>>>>>                     hfront-porch = <48 88 88>;
> >>>>>>                     hback-porch = <96 168 168>;
> >>>>>>                     hsync-len = <32 64 64>;
> >>>>>>                     vfront-porch = <8 13 14>;
> >>>>>>                     vback-porch = <8 13 14>;
> >>>>>>                     vsync-len = <8 12 14>;
> >>>>>>             };
> >>>>>>             
> >>>>>>             port {
> >>>>>>                     panel_input: endpoint {
> >>>>>>                             remote-endpoint = <&lvds_encoder_output>;
> >>>>>>                     };
> >>>>>>             };
> >>>>>>     };
> >>>>>>     
> >>>>>>     lvds-encoder {
> >>>>>>             compatible = "ti,ds90c185", "lvds-encoder";
> >>>>>>             
> >>>>>>             ports {
> >>>>>>                     #address-cells = <1>;
> >>>>>>                     #size-cells = <0>;
> >>>>>>                     
> >>>>>>                     port@0 {
> >>>>>>                             reg = <0>;
> >>>>>>                             
> >>>>>>                             lvds_encoder_input: endpoint {
> >>>>>>                                     remote-endpoint =
> >>>>>>                                     <&hlcdc_output>;
> >>>>>>                             };
> >>>>>>                     };
> >>>>>>                     
> >>>>>>                     port@1 {
> >>>>>>                             reg = <1>;
> >>>>>>                             
> >>>>>>                             lvds_encoder_output: endpoint {
> >>>>>>                                     remote-endpoint = <&panel_input>;
> >>>>>>                             };
> >>>>>>                     };
> >>>>>>             };
> >>>>>>     };
> >>>>>> 
> >>>>>> But, instead of perverting the panel-lvds driver with support
> >>>>>> for a totally fake non-lvds bus_format, I intruduce an API that
> >>>>>> allows display controller drivers to query the required bus_format of
> >>>>>> any intermediate bridges, and match up with that instead of the
> >>>>>> formats given by the drm_connector. I trigger this with this addition
> >>>>>> to the lvds-encoder DT node:
> >>>>>>             interface-pix-fmt = "rgb565";
> >>>>>> 
> >>>>>> Naming is hard though, so I'm not sure if that's good?
> >>>>>> 
> >>>>>> I threw in the first patch, since that is the actual lvds encoder
> >>>>>> I have in this case.
> >>>>>> 
> >>>>>> Suggestions welcome.
> >>>>> 
> >>>>> Took a quick look, feels rather un-atomic. And there's beend
> >>>>> discussing for other bridge related state that we might want to track
> >>>>> (like the full adjusted_mode that might need to be adjusted at each
> >>>>> stage in the chain). So here's my suggestions:
> >>>>> 
> >>>>> - Add an optional per-bridge internal state struct using the support
> >>>>>   in https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-> >>>>>   driver-private-state
> >>>>>   
> >>>>>   Yes it says "driver private", but since bridge is just helper stuff
> >>>>>   that's all included. "driver private" == "not exposed as uapi" here.
> >>>>>   Include all the usual convenience wrappers to get at the state for a
> >>>>>   bridge.
> >>>>> 
> >>>>> - Then stuff your bus_format into that new drm_bridge_state struct.
> >>>>> 
> >>>>> - Add a new bridge callback atomic_check, which gets that bridge state
> >>>>>   as parameter (similar to all the other atomic_check functions).
> >>>>> 
> >>>>> This way we can even handle the bus_format dynamically, through the
> >>>>> atomic framework your bridge's atomic_check callback can look at the
> >>>>> entire atomic state (both up and down the chain if needed), it all
> >>>>> neatly fits into atomic overall and it's much easier to extend.
> >>>> 
> >>>> While I think we'll eventually need bridge states, I don't think that's
> >>>> need yet. The bus formats reported by this patch series are static.
> >>>> We're not talking about the currently configured format for a bridge,
> >>>> but about the list of supported formats. This is similar to the
> >>>> bus_formats field present in the drm_display_info structure. There is
> >>>> thus in my opinion no need to interface this with atomic until we need
> >>>> to track the current format (and I think that will indeed happen at
> >>>> some point, but I don't think Peter needs this feature for now). That's
> >>>> why I've told Peter that I would like a bridge API to report the
> >>>> information and haven't requested a state-based implementation.
> >>> 
> >>> If it's static, why a dynamic callback? Just add it to struct
> >>> drm_bridge, require it's filled out before registering the bridge,
> >>> done.
> >> 
> >> If I remember correctly I mentioned both options in my initial proposal,
> >> without a personal preference. A new field in struct drm_bridge would
> >> certainly work for me.
> > 
> > You did. Ok, so v3 coming up...
> 
> Nope, v3 is not coming up. This feels like an impossible mission for me, as
> this changes core DRM design, and it would just be too much of a time sink
> for me. Besides, the final paragraph of the nice long "state of
> bridges"-mail from Laurent, i.e.
> 
> On 2018-04-04 15:07, Laurent Pinchart wrote:
> > Finally, still regarding Peter's case, the decision to output RGB565
> > instead of RGB888 (which the LVDS encoder expects) is due to PCB routing
> > between the display controller and the LVDS encoder. This isn't a
> > property of the LVDS encoder or the display controller, but of their
> > hardware connection. This patch series uses a DT property in the LVDS
> > encoder DT node to convey that information, but wouldn't it be equally
> > correct (or incorrect :-)) to instead use a DT property in the display
> > controller DT node ?
> 
> hints at where I'm going. The reason is that I have a patch (that needs some
> polish, will post soon) that makes the atmel-hlcdc driver use components in
> order to hook it with the tda998x driver (an hdmi encoder), and there too I
> need the atmel-hlcdc driver to use rgb565 output. And in that case there
> are no bridges involved, so the proposed solution in this series has zero
> hope of being adequate. It simply seems that forcing the output mode in the
> atmel-hlcdc driver is what fixes the root cause.
> 
> End result; the only thing that survives this series is the interesting
> discussion and patch 1/5. But I will include that patch in the alternative
> solution, so you can drop this series entirely...

I feel some disappointment here. I would like to make it very clear that your 
work was appreciated. Not all efforts turn into patches that get merged 
upstream, and some of the greatest work only results in ideas that are then 
taken over by other developers. Even if this patch series ends up being 
dropped, it served as a very useful experiment to get us closer to a good 
solution to the problem. As such the time and efforts you have invested in it 
are certainly not wasted and were very helpful.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/5] allow override of bus format in bridges

[Peter Rosin]

On 2018-04-09 14:51, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Monday, 9 April 2018 10:04:05 EEST Peter Rosin wrote:
>> On 2018-04-04 14:35, Peter Rosin wrote:
>>> On 2018-04-04 11:07, Laurent Pinchart wrote:
>>>> On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
>>>>> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
>>>>>> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
>>>>>>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> [I got to v2 sooner than expected]
>>>>>>>>
>>>>>>>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
>>>>>>>> on to an lvds panel. Which seems like something that has been
>>>>>>>> done one or two times before...
>>>>>>>>
>>>>>>>> The problem is that the bus_format of the SoC and the panel do
>>>>>>>> not agree. The SoC driver (atmel-hlcdc) can handle the
>>>>>>>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
>>>>>>>> wired for the rgb565 case. The lvds encoder supports rgb888 on
>>>>>>>> its input side with the LSB wires for each color simply pulled
>>>>>>>> down internally in the encoder in my case which means that the
>>>>>>>> rgb565 bus_format is the format that works best. And the panel
>>>>>>>> is expecting lvds (vesa-24), which is what the encoder outputs.
>>>>>>>>
>>>>>>>> The reason I "blame" the bus_format of the drm_connector is that
>>>>>>>> with the below DT snippet, things do not work *exactly* due to
>>>>>>>> that. At least, it starts to work if I hack the panel-lvds driver
>>>>>>>> to report the rgb565 bus_format instead of vesa-24.
>>>>>>>>
>>>>>>>>     panel: panel {
>>>>>>>>             compatible = "panel-lvds";
>>>>>>>>             
>>>>>>>>             width-mm = <304>;
>>>>>>>>             height-mm = <228;
>>>>>>>>             
>>>>>>>>             data-mapping = "vesa-24";
>>>>>>>>             
>>>>>>>>             panel-timing {
>>>>>>>>                     // 1024x768 @ 60Hz (typical)
>>>>>>>>                     clock-frequency = <52140000 65000000 71100000>;
>>>>>>>>                     hactive = <1024>;
>>>>>>>>                     vactive = <768>;
>>>>>>>>                     hfront-porch = <48 88 88>;
>>>>>>>>                     hback-porch = <96 168 168>;
>>>>>>>>                     hsync-len = <32 64 64>;
>>>>>>>>                     vfront-porch = <8 13 14>;
>>>>>>>>                     vback-porch = <8 13 14>;
>>>>>>>>                     vsync-len = <8 12 14>;
>>>>>>>>             };
>>>>>>>>             
>>>>>>>>             port {
>>>>>>>>                     panel_input: endpoint {
>>>>>>>>                             remote-endpoint = <&lvds_encoder_output>;
>>>>>>>>                     };
>>>>>>>>             };
>>>>>>>>     };
>>>>>>>>     
>>>>>>>>     lvds-encoder {
>>>>>>>>             compatible = "ti,ds90c185", "lvds-encoder";
>>>>>>>>             
>>>>>>>>             ports {
>>>>>>>>                     #address-cells = <1>;
>>>>>>>>                     #size-cells = <0>;
>>>>>>>>                     
>>>>>>>>                     port@0 {
>>>>>>>>                             reg = <0>;
>>>>>>>>                             
>>>>>>>>                             lvds_encoder_input: endpoint {
>>>>>>>>                                     remote-endpoint =
>>>>>>>>                                     <&hlcdc_output>;
>>>>>>>>                             };
>>>>>>>>                     };
>>>>>>>>                     
>>>>>>>>                     port@1 {
>>>>>>>>                             reg = <1>;
>>>>>>>>                             
>>>>>>>>                             lvds_encoder_output: endpoint {
>>>>>>>>                                     remote-endpoint = <&panel_input>;
>>>>>>>>                             };
>>>>>>>>                     };
>>>>>>>>             };
>>>>>>>>     };
>>>>>>>>
>>>>>>>> But, instead of perverting the panel-lvds driver with support
>>>>>>>> for a totally fake non-lvds bus_format, I intruduce an API that
>>>>>>>> allows display controller drivers to query the required bus_format of
>>>>>>>> any intermediate bridges, and match up with that instead of the
>>>>>>>> formats given by the drm_connector. I trigger this with this addition
>>>>>>>> to the lvds-encoder DT node:
>>>>>>>>             interface-pix-fmt = "rgb565";
>>>>>>>>
>>>>>>>> Naming is hard though, so I'm not sure if that's good?
>>>>>>>>
>>>>>>>> I threw in the first patch, since that is the actual lvds encoder
>>>>>>>> I have in this case.
>>>>>>>>
>>>>>>>> Suggestions welcome.
>>>>>>>
>>>>>>> Took a quick look, feels rather un-atomic. And there's beend
>>>>>>> discussing for other bridge related state that we might want to track
>>>>>>> (like the full adjusted_mode that might need to be adjusted at each
>>>>>>> stage in the chain). So here's my suggestions:
>>>>>>>
>>>>>>> - Add an optional per-bridge internal state struct using the support
>>>>>>>   in https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-> >>>>>   driver-private-state
>>>>>>>   
>>>>>>>   Yes it says "driver private", but since bridge is just helper stuff
>>>>>>>   that's all included. "driver private" == "not exposed as uapi" here.
>>>>>>>   Include all the usual convenience wrappers to get at the state for a
>>>>>>>   bridge.
>>>>>>>
>>>>>>> - Then stuff your bus_format into that new drm_bridge_state struct.
>>>>>>>
>>>>>>> - Add a new bridge callback atomic_check, which gets that bridge state
>>>>>>>   as parameter (similar to all the other atomic_check functions).
>>>>>>>
>>>>>>> This way we can even handle the bus_format dynamically, through the
>>>>>>> atomic framework your bridge's atomic_check callback can look at the
>>>>>>> entire atomic state (both up and down the chain if needed), it all
>>>>>>> neatly fits into atomic overall and it's much easier to extend.
>>>>>>
>>>>>> While I think we'll eventually need bridge states, I don't think that's
>>>>>> need yet. The bus formats reported by this patch series are static.
>>>>>> We're not talking about the currently configured format for a bridge,
>>>>>> but about the list of supported formats. This is similar to the
>>>>>> bus_formats field present in the drm_display_info structure. There is
>>>>>> thus in my opinion no need to interface this with atomic until we need
>>>>>> to track the current format (and I think that will indeed happen at
>>>>>> some point, but I don't think Peter needs this feature for now). That's
>>>>>> why I've told Peter that I would like a bridge API to report the
>>>>>> information and haven't requested a state-based implementation.
>>>>>
>>>>> If it's static, why a dynamic callback? Just add it to struct
>>>>> drm_bridge, require it's filled out before registering the bridge,
>>>>> done.
>>>>
>>>> If I remember correctly I mentioned both options in my initial proposal,
>>>> without a personal preference. A new field in struct drm_bridge would
>>>> certainly work for me.
>>>
>>> You did. Ok, so v3 coming up...
>>
>> Nope, v3 is not coming up. This feels like an impossible mission for me, as
>> this changes core DRM design, and it would just be too much of a time sink
>> for me. Besides, the final paragraph of the nice long "state of
>> bridges"-mail from Laurent, i.e.
>>
>> On 2018-04-04 15:07, Laurent Pinchart wrote:
>>> Finally, still regarding Peter's case, the decision to output RGB565
>>> instead of RGB888 (which the LVDS encoder expects) is due to PCB routing
>>> between the display controller and the LVDS encoder. This isn't a
>>> property of the LVDS encoder or the display controller, but of their
>>> hardware connection. This patch series uses a DT property in the LVDS
>>> encoder DT node to convey that information, but wouldn't it be equally
>>> correct (or incorrect :-)) to instead use a DT property in the display
>>> controller DT node ?
>>
>> hints at where I'm going. The reason is that I have a patch (that needs some
>> polish, will post soon) that makes the atmel-hlcdc driver use components in
>> order to hook it with the tda998x driver (an hdmi encoder), and there too I
>> need the atmel-hlcdc driver to use rgb565 output. And in that case there
>> are no bridges involved, so the proposed solution in this series has zero
>> hope of being adequate. It simply seems that forcing the output mode in the
>> atmel-hlcdc driver is what fixes the root cause.
>>
>> End result; the only thing that survives this series is the interesting
>> discussion and patch 1/5. But I will include that patch in the alternative
>> solution, so you can drop this series entirely...
> 
> I feel some disappointment here. I would like to make it very clear that your 
> work was appreciated. Not all efforts turn into patches that get merged 
> upstream, and some of the greatest work only results in ideas that are then 
> taken over by other developers. Even if this patch series ends up being 
> dropped, it served as a very useful experiment to get us closer to a good 
> solution to the problem. As such the time and efforts you have invested in it 
> are certainly not wasted and were very helpful.

No hard feelings from me, and maybe I'll revisit (not all that keen though)
if the output-mode override for the atmel-hlcdc driver is considered a
workaround in need of a proper solution. Not that I think that's actually
the case, but who knows...

Cheers,
Peter