diff mbox series

[v3,1/4] drm: bridge: dw_hdmi: Add flag to indicate output port is optional

Message ID 20241231192925.97614-1-marex@denx.de (mailing list archive)
State New
Headers show
Series [v3,1/4] drm: bridge: dw_hdmi: Add flag to indicate output port is optional | expand

Commit Message

Marek Vasut Dec. 31, 2024, 7:28 p.m. UTC
Add a flag meant purely to work around broken i.MX8MP DTs which enable
HDMI but do not contain the HDMI connector node. This flag allows such
DTs to work by creating the connector in the HDMI bridge driver. Do not
use this flag, do not proliferate this flag, please fix your DTs.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Liu Ying <victor.liu@nxp.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
---
V3: New patch
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++++----
 include/drm/bridge/dw_hdmi.h              |  2 ++
 2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Dec. 31, 2024, 8:31 p.m. UTC | #1
Hi Marek,

Thank you for the patch.

On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
> Add a flag meant purely to work around broken i.MX8MP DTs which enable
> HDMI but do not contain the HDMI connector node. This flag allows such
> DTs to work by creating the connector in the HDMI bridge driver. Do not
> use this flag, do not proliferate this flag, please fix your DTs.

What's the rationale for this, what prevents fixing DT instead of using
this flag ? Adding such a flag will most likely open the door to
proliferation.

If you can't fix the DT on particular boards, patching it could be an
option. We had a similar problem on Renesas boards, which we fixed with
a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
to create LVDS encoder nodes"). This made the workaround self-contained,
and allowed dropping it several kernel versions later (in commit
841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
compatibility").

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Liu Ying <victor.liu@nxp.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> V3: New patch
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++++----
>  include/drm/bridge/dw_hdmi.h              |  2 ++
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 996733ed2c004..852e73c0f686f 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2893,9 +2893,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
>  {
>  	struct dw_hdmi *hdmi = bridge->driver_private;
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> -		return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
> -					 bridge, flags);
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> +		if (hdmi->plat_data->output_port_optional && !hdmi->next_bridge)
> +			return dw_hdmi_connector_create(hdmi);
> +		else
> +			return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
> +						 bridge, flags);
> +	}
>  
>  	return dw_hdmi_connector_create(hdmi);
>  }
> @@ -3298,7 +3302,7 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>  					  hdmi->plat_data->output_port,
>  					  -1);
>  	if (!remote)
> -		return -ENODEV;
> +		return hdmi->plat_data->output_port_optional ? 0 : -ENODEV;
>  
>  	hdmi->next_bridge = of_drm_find_bridge(remote);
>  	of_node_put(remote);
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 6a46baa0737cd..3bb6e633424a8 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -127,6 +127,8 @@ struct dw_hdmi_plat_data {
>  	struct regmap *regm;
>  
>  	unsigned int output_port;
> +	/* Used purely by MX8MP HDMI to work around broken DTs without HDMI connector node. */
> +	bool output_port_optional;
>  
>  	unsigned long input_bus_encoding;
>  	bool use_drm_infoframe;
Marek Vasut Dec. 31, 2024, 9:10 p.m. UTC | #2
On 12/31/24 9:31 PM, Laurent Pinchart wrote:
> Hi Marek,

Hi,

> Thank you for the patch.
> 
> On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
>> Add a flag meant purely to work around broken i.MX8MP DTs which enable
>> HDMI but do not contain the HDMI connector node. This flag allows such
>> DTs to work by creating the connector in the HDMI bridge driver. Do not
>> use this flag, do not proliferate this flag, please fix your DTs.
> 
> What's the rationale for this, what prevents fixing DT instead of using
> this flag ? Adding such a flag will most likely open the door to
> proliferation.

See the V2 series discussion, there are a few in-tree DTs which do not 
have the HDMI connector node. The rationale is there might be more and 
they might come from vendors, so this flag is necessary to work around 
those DTs.

> If you can't fix the DT on particular boards, patching it could be an
> option. We had a similar problem on Renesas boards, which we fixed with
> a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
> to create LVDS encoder nodes"). This made the workaround self-contained,
> and allowed dropping it several kernel versions later (in commit
> 841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
> compatibility").
Frankly, I would much rather fix the few in-tree DTs and mandate the 
HDMI connector node in DT, which would keep the code simple, rather than 
maintain a backward compatibility workaround for problem which might not 
even exist.
Laurent Pinchart Jan. 1, 2025, 10:36 p.m. UTC | #3
On Tue, Dec 31, 2024 at 10:10:51PM +0100, Marek Vasut wrote:
> On 12/31/24 9:31 PM, Laurent Pinchart wrote:
> > Hi Marek,
> 
> Hi,
> 
> > Thank you for the patch.
> > 
> > On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
> >> Add a flag meant purely to work around broken i.MX8MP DTs which enable
> >> HDMI but do not contain the HDMI connector node. This flag allows such
> >> DTs to work by creating the connector in the HDMI bridge driver. Do not
> >> use this flag, do not proliferate this flag, please fix your DTs.
> > 
> > What's the rationale for this, what prevents fixing DT instead of using
> > this flag ? Adding such a flag will most likely open the door to
> > proliferation.
> 
> See the V2 series discussion, there are a few in-tree DTs which do not 
> have the HDMI connector node. The rationale is there might be more and 
> they might come from vendors, so this flag is necessary to work around 
> those DTs.
>
> > If you can't fix the DT on particular boards, patching it could be an
> > option. We had a similar problem on Renesas boards, which we fixed with
> > a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
> > to create LVDS encoder nodes"). This made the workaround self-contained,
> > and allowed dropping it several kernel versions later (in commit
> > 841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
> > compatibility").
>
> Frankly, I would much rather fix the few in-tree DTs and mandate the 
> HDMI connector node in DT, which would keep the code simple, rather than 
> maintain a backward compatibility workaround for problem which might not 
> even exist.

The in-tree device tree sources should be converted as part of the
series, I don't see a point trying to maintain backward compatibility
for in-tree DT sources.

For out-of-tree sources it depends on how likely the problem is. There's
no regression if nobody is affected. I personally like restricting
backward compatibility to the strict minimum, to ensure that all new DTs
will use proper bindings. Making the backward compatibility code
self-contained helps there, and we could also print a loud warning
(WARN_ON() seems appropriate) and set a date for the removal of the
workaround.
Marek Vasut Jan. 2, 2025, 1:15 a.m. UTC | #4
On 1/1/25 11:36 PM, Laurent Pinchart wrote:
> On Tue, Dec 31, 2024 at 10:10:51PM +0100, Marek Vasut wrote:
>> On 12/31/24 9:31 PM, Laurent Pinchart wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Thank you for the patch.
>>>
>>> On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
>>>> Add a flag meant purely to work around broken i.MX8MP DTs which enable
>>>> HDMI but do not contain the HDMI connector node. This flag allows such
>>>> DTs to work by creating the connector in the HDMI bridge driver. Do not
>>>> use this flag, do not proliferate this flag, please fix your DTs.
>>>
>>> What's the rationale for this, what prevents fixing DT instead of using
>>> this flag ? Adding such a flag will most likely open the door to
>>> proliferation.
>>
>> See the V2 series discussion, there are a few in-tree DTs which do not
>> have the HDMI connector node. The rationale is there might be more and
>> they might come from vendors, so this flag is necessary to work around
>> those DTs.
>>
>>> If you can't fix the DT on particular boards, patching it could be an
>>> option. We had a similar problem on Renesas boards, which we fixed with
>>> a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
>>> to create LVDS encoder nodes"). This made the workaround self-contained,
>>> and allowed dropping it several kernel versions later (in commit
>>> 841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
>>> compatibility").
>>
>> Frankly, I would much rather fix the few in-tree DTs and mandate the
>> HDMI connector node in DT, which would keep the code simple, rather than
>> maintain a backward compatibility workaround for problem which might not
>> even exist.
> 
> The in-tree device tree sources should be converted as part of the
> series, I don't see a point trying to maintain backward compatibility
> for in-tree DT sources.

That's fine, that I can do.

> For out-of-tree sources it depends on how likely the problem is. There's
> no regression if nobody is affected. I personally like restricting
> backward compatibility to the strict minimum, to ensure that all new DTs
> will use proper bindings. Making the backward compatibility code
> self-contained helps there, and we could also print a loud warning
> (WARN_ON() seems appropriate) and set a date for the removal of the
> workaround.
The other alternative would be to not add the workaround at all, and 
wait if someone is going to complain about broken downstream DT. If so, 
then it can be added. I would much rather prefer this simple option, 
because I have this feeling there are not going to be (m)any broken 
downstream DTs, but I might be wrong about that.
Dmitry Baryshkov Jan. 2, 2025, 3:26 a.m. UTC | #5
On Thu, Jan 02, 2025 at 12:36:20AM +0200, Laurent Pinchart wrote:
> On Tue, Dec 31, 2024 at 10:10:51PM +0100, Marek Vasut wrote:
> > On 12/31/24 9:31 PM, Laurent Pinchart wrote:
> > > Hi Marek,
> > 
> > Hi,
> > 
> > > Thank you for the patch.
> > > 
> > > On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
> > >> Add a flag meant purely to work around broken i.MX8MP DTs which enable
> > >> HDMI but do not contain the HDMI connector node. This flag allows such
> > >> DTs to work by creating the connector in the HDMI bridge driver. Do not
> > >> use this flag, do not proliferate this flag, please fix your DTs.
> > > 
> > > What's the rationale for this, what prevents fixing DT instead of using
> > > this flag ? Adding such a flag will most likely open the door to
> > > proliferation.
> > 
> > See the V2 series discussion, there are a few in-tree DTs which do not 
> > have the HDMI connector node. The rationale is there might be more and 
> > they might come from vendors, so this flag is necessary to work around 
> > those DTs.
> >
> > > If you can't fix the DT on particular boards, patching it could be an
> > > option. We had a similar problem on Renesas boards, which we fixed with
> > > a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
> > > to create LVDS encoder nodes"). This made the workaround self-contained,
> > > and allowed dropping it several kernel versions later (in commit
> > > 841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
> > > compatibility").
> >
> > Frankly, I would much rather fix the few in-tree DTs and mandate the 
> > HDMI connector node in DT, which would keep the code simple, rather than 
> > maintain a backward compatibility workaround for problem which might not 
> > even exist.
> 
> The in-tree device tree sources should be converted as part of the
> series, I don't see a point trying to maintain backward compatibility
> for in-tree DT sources.

DT is an ABI. We are supposed to keep backwards compatibility with
existing device trees (at least for a while). I'm adding DT list and
maintainers to be able to provide comments on this topic.

> For out-of-tree sources it depends on how likely the problem is. There's
> no regression if nobody is affected. I personally like restricting
> backward compatibility to the strict minimum, to ensure that all new DTs
> will use proper bindings. Making the backward compatibility code
> self-contained helps there, and we could also print a loud warning
> (WARN_ON() seems appropriate) and set a date for the removal of the
> workaround.
Dmitry Baryshkov Jan. 2, 2025, 4:51 a.m. UTC | #6
On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
> Add a flag meant purely to work around broken i.MX8MP DTs which enable
> HDMI but do not contain the HDMI connector node. This flag allows such
> DTs to work by creating the connector in the HDMI bridge driver. Do not
> use this flag, do not proliferate this flag, please fix your DTs.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Please describe that it is required for backwards compatibility with the
existing DTs. I think it is a good idea unless Shawn, Sascha or Fabio
tell that we should ignore backwards compatibility.

With that in mind:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Liu Ying <victor.liu@nxp.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> V3: New patch
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++++----
>  include/drm/bridge/dw_hdmi.h              |  2 ++
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 996733ed2c004..852e73c0f686f 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2893,9 +2893,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
>  {
>  	struct dw_hdmi *hdmi = bridge->driver_private;
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> -		return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
> -					 bridge, flags);
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> +		if (hdmi->plat_data->output_port_optional && !hdmi->next_bridge)
> +			return dw_hdmi_connector_create(hdmi);
> +		else
> +			return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
> +						 bridge, flags);
> +	}
>  
>  	return dw_hdmi_connector_create(hdmi);
>  }
> @@ -3298,7 +3302,7 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>  					  hdmi->plat_data->output_port,
>  					  -1);
>  	if (!remote)
> -		return -ENODEV;
> +		return hdmi->plat_data->output_port_optional ? 0 : -ENODEV;
>  
>  	hdmi->next_bridge = of_drm_find_bridge(remote);
>  	of_node_put(remote);
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 6a46baa0737cd..3bb6e633424a8 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -127,6 +127,8 @@ struct dw_hdmi_plat_data {
>  	struct regmap *regm;
>  
>  	unsigned int output_port;
> +	/* Used purely by MX8MP HDMI to work around broken DTs without HDMI connector node. */
> +	bool output_port_optional;
>  
>  	unsigned long input_bus_encoding;
>  	bool use_drm_infoframe;
> -- 
> 2.45.2
>
Laurent Pinchart Jan. 2, 2025, 8:30 a.m. UTC | #7
On Thu, Jan 02, 2025 at 05:26:50AM +0200, Dmitry Baryshkov wrote:
> On Thu, Jan 02, 2025 at 12:36:20AM +0200, Laurent Pinchart wrote:
> > On Tue, Dec 31, 2024 at 10:10:51PM +0100, Marek Vasut wrote:
> > > On 12/31/24 9:31 PM, Laurent Pinchart wrote:
> > > > Hi Marek,
> > > 
> > > Hi,
> > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
> > > >> Add a flag meant purely to work around broken i.MX8MP DTs which enable
> > > >> HDMI but do not contain the HDMI connector node. This flag allows such
> > > >> DTs to work by creating the connector in the HDMI bridge driver. Do not
> > > >> use this flag, do not proliferate this flag, please fix your DTs.
> > > > 
> > > > What's the rationale for this, what prevents fixing DT instead of using
> > > > this flag ? Adding such a flag will most likely open the door to
> > > > proliferation.
> > > 
> > > See the V2 series discussion, there are a few in-tree DTs which do not 
> > > have the HDMI connector node. The rationale is there might be more and 
> > > they might come from vendors, so this flag is necessary to work around 
> > > those DTs.
> > >
> > > > If you can't fix the DT on particular boards, patching it could be an
> > > > option. We had a similar problem on Renesas boards, which we fixed with
> > > > a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
> > > > to create LVDS encoder nodes"). This made the workaround self-contained,
> > > > and allowed dropping it several kernel versions later (in commit
> > > > 841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
> > > > compatibility").
> > >
> > > Frankly, I would much rather fix the few in-tree DTs and mandate the 
> > > HDMI connector node in DT, which would keep the code simple, rather than 
> > > maintain a backward compatibility workaround for problem which might not 
> > > even exist.
> > 
> > The in-tree device tree sources should be converted as part of the
> > series, I don't see a point trying to maintain backward compatibility
> > for in-tree DT sources.
> 
> DT is an ABI. We are supposed to keep backwards compatibility with
> existing device trees (at least for a while). I'm adding DT list and
> maintainers to be able to provide comments on this topic.

Backward compatibility is about supporting old DT binaries with a newer
kernel. There's no need to support old DT bindings in in-kernel DT
sources. By definition, if someone compiles a DT from a newer kernel and
installs it along with the newer kernel, there's no "backward"
direction.

The backward compatibility requirements aim at ensuring no breakage when
upgrading the kernel without upgrading the device tree. As I mentioned,
there is no regression if nobody is affected in the first place. Proving
there is no affected DT in the wild is difficult though.

> > For out-of-tree sources it depends on how likely the problem is. There's
> > no regression if nobody is affected. I personally like restricting
> > backward compatibility to the strict minimum, to ensure that all new DTs
> > will use proper bindings. Making the backward compatibility code
> > self-contained helps there, and we could also print a loud warning
> > (WARN_ON() seems appropriate) and set a date for the removal of the
> > workaround.
Dmitry Baryshkov Jan. 3, 2025, 5:33 a.m. UTC | #8
On Thu, Jan 02, 2025 at 10:30:38AM +0200, Laurent Pinchart wrote:
> On Thu, Jan 02, 2025 at 05:26:50AM +0200, Dmitry Baryshkov wrote:
> > On Thu, Jan 02, 2025 at 12:36:20AM +0200, Laurent Pinchart wrote:
> > > On Tue, Dec 31, 2024 at 10:10:51PM +0100, Marek Vasut wrote:
> > > > On 12/31/24 9:31 PM, Laurent Pinchart wrote:
> > > > > Hi Marek,
> > > > 
> > > > Hi,
> > > > 
> > > > > Thank you for the patch.
> > > > > 
> > > > > On Tue, Dec 31, 2024 at 08:28:48PM +0100, Marek Vasut wrote:
> > > > >> Add a flag meant purely to work around broken i.MX8MP DTs which enable
> > > > >> HDMI but do not contain the HDMI connector node. This flag allows such
> > > > >> DTs to work by creating the connector in the HDMI bridge driver. Do not
> > > > >> use this flag, do not proliferate this flag, please fix your DTs.
> > > > > 
> > > > > What's the rationale for this, what prevents fixing DT instead of using
> > > > > this flag ? Adding such a flag will most likely open the door to
> > > > > proliferation.
> > > > 
> > > > See the V2 series discussion, there are a few in-tree DTs which do not 
> > > > have the HDMI connector node. The rationale is there might be more and 
> > > > they might come from vendors, so this flag is necessary to work around 
> > > > those DTs.
> > > >
> > > > > If you can't fix the DT on particular boards, patching it could be an
> > > > > option. We had a similar problem on Renesas boards, which we fixed with
> > > > > a DT overlay, see commit 81c0e3dd82927064 ("drm: rcar-du: Fix legacy DT
> > > > > to create LVDS encoder nodes"). This made the workaround self-contained,
> > > > > and allowed dropping it several kernel versions later (in commit
> > > > > 841281fe52a769fe, "drm: rcar-du: Drop LVDS device tree backward
> > > > > compatibility").
> > > >
> > > > Frankly, I would much rather fix the few in-tree DTs and mandate the 
> > > > HDMI connector node in DT, which would keep the code simple, rather than 
> > > > maintain a backward compatibility workaround for problem which might not 
> > > > even exist.
> > > 
> > > The in-tree device tree sources should be converted as part of the
> > > series, I don't see a point trying to maintain backward compatibility
> > > for in-tree DT sources.
> > 
> > DT is an ABI. We are supposed to keep backwards compatibility with
> > existing device trees (at least for a while). I'm adding DT list and
> > maintainers to be able to provide comments on this topic.
> 
> Backward compatibility is about supporting old DT binaries with a newer
> kernel. There's no need to support old DT bindings in in-kernel DT
> sources. By definition, if someone compiles a DT from a newer kernel and
> installs it along with the newer kernel, there's no "backward"
> direction.

Hmm, nobody is asking to provide compatibility with old DT bindings.
However supporting DTs with no extra "display-connector" bridge after
the DW bridge is exactly "supporting old DT binaries" in my opinion.

> The backward compatibility requirements aim at ensuring no breakage when
> upgrading the kernel without upgrading the device tree. As I mentioned,
> there is no regression if nobody is affected in the first place. Proving
> there is no affected DT in the wild is difficult though.
> 
> > > For out-of-tree sources it depends on how likely the problem is. There's
> > > no regression if nobody is affected. I personally like restricting
> > > backward compatibility to the strict minimum, to ensure that all new DTs
> > > will use proper bindings. Making the backward compatibility code
> > > self-contained helps there, and we could also print a loud warning
> > > (WARN_ON() seems appropriate) and set a date for the removal of the
> > > workaround.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 996733ed2c004..852e73c0f686f 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2893,9 +2893,13 @@  static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
 {
 	struct dw_hdmi *hdmi = bridge->driver_private;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
-		return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
-					 bridge, flags);
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
+		if (hdmi->plat_data->output_port_optional && !hdmi->next_bridge)
+			return dw_hdmi_connector_create(hdmi);
+		else
+			return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
+						 bridge, flags);
+	}
 
 	return dw_hdmi_connector_create(hdmi);
 }
@@ -3298,7 +3302,7 @@  static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
 					  hdmi->plat_data->output_port,
 					  -1);
 	if (!remote)
-		return -ENODEV;
+		return hdmi->plat_data->output_port_optional ? 0 : -ENODEV;
 
 	hdmi->next_bridge = of_drm_find_bridge(remote);
 	of_node_put(remote);
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 6a46baa0737cd..3bb6e633424a8 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -127,6 +127,8 @@  struct dw_hdmi_plat_data {
 	struct regmap *regm;
 
 	unsigned int output_port;
+	/* Used purely by MX8MP HDMI to work around broken DTs without HDMI connector node. */
+	bool output_port_optional;
 
 	unsigned long input_bus_encoding;
 	bool use_drm_infoframe;