Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Vincent ABRIOU]

On 11/29/2016 10:04 AM, Laurent Pinchart wrote:
> Instead of linking encoders and bridges in every driver (and getting it
> wrong half of the time, as many drivers forget to set the drm_bridge
> encoder pointer), do so in core code. The drm_bridge_attach() function
> needs the encoder and optional previous bridge to perform that task,
> update all the callers.
>
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
>  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
>  drivers/gpu/drm/drm_bridge.c   | 46 
> --
>  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
>  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
>  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
>  drivers/gpu/drm/imx/parallel-display.c |  4 +-
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
>  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
>  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
>  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
>  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
>  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-

Hi Laurent,

For sti dvo, hda and hdmi:
Acked-by: Vincent Abriou 

Vincent

>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
>  include/drm/drm_bridge.h   |  3 +-
>  23 files changed, 83 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 6119b5085501..e7799b6ee829 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -230,9 +230,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device 
> *dev,
> of_node_put(np);
>
> if (bridge) {
> -   output->encoder.bridge = bridge;
> -   bridge->encoder = >encoder;
> -   ret = drm_bridge_attach(dev, bridge);
> +   ret = drm_bridge_attach(>encoder, bridge, NULL);
> if (!ret)
> return 0;
> }
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6e0447f329a2..1835f1fdad19 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1227,12 +1227,10 @@ static int analogix_dp_create_bridge(struct 
> drm_device *drm_dev,
>
> dp->bridge = bridge;
>
> -   dp->encoder->bridge = bridge;
> bridge->driver_private = dp;
> -   bridge->encoder = dp->encoder;
> bridge->funcs = _dp_bridge_funcs;
>
> -   ret = drm_bridge_attach(drm_dev, bridge);
> +   ret = drm_bridge_attach(dp->encoder, bridge, NULL);
> if (ret) {
> DRM_ERROR("failed to attach drm bridge\n");
> return -EINVAL;
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index b71088dab268..432e0e3fff72 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1841,13 +1841,12 @@ static int dw_hdmi_register(struct drm_device *drm, 
> struct dw_hdmi *hdmi)
> hdmi->bridge = bridge;
> bridge->driver_private = hdmi;
> bridge->funcs = _hdmi_bridge_funcs;
> -   ret = drm_bridge_attach(drm, bridge);
> +   ret = drm_bridge_attach(encoder, bridge, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize bridge with drm\n");
> return -EINVAL;
> }
>
> -   encoder->bridge = bridge;
> hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(>connector,
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 0ee052b7c21a..850bd6509ef1 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  /**
>   * DOC: overview
> @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  EXPORT_SYMBOL(drm_bridge_remove);
>
>  /**
> - * drm_bridge_attach - associate given bridge to our DRM device
> + * drm_bridge_attach - attach the bridge to an encoder's chain
>   *
> - * @dev: DRM device
> - * 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Archit Taneja]

On 11/30/2016 4:35 PM, Laurent Pinchart wrote:

Hi Archit,

On Wednesday 30 Nov 2016 16:30:53 Archit Taneja wrote:

On 11/30/2016 03:53 PM, Laurent Pinchart wrote:

On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote:

On 11/29/2016 11:27 PM, Laurent Pinchart wrote:

On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:

On 11/29/2016 02:34 PM, Laurent Pinchart wrote:

Instead of linking encoders and bridges in every driver (and getting
it wrong half of the time, as many drivers forget to set the
drm_bridge encoder pointer), do so in core code. The
drm_bridge_attach() function needs the encoder and optional previous
bridge to perform that task, update all the callers.

Signed-off-by: Laurent Pinchart

---


[snip]


I think we could derive previous from the encoder itself. Something
like:

previous = encoder->bridge;
while (previous && previous->next)
previous = previous->next;


That's a very good point. It would however prevent us from catching
drivers that attach bridges in the wrong order, which the !previous->dev
currently allows us to do (and it should be turned into a WARN_ON as
Daniel proposed).


With the simpler API, I don't think we will ever hit the case of
!previous->dev. The previous bridge (if it exists) in the chain would
already have a dev attached to it. In other words, we would remove the
risk of the chance of the 'previous' bridge being unattached.

I'm a bit unclear about what you mean about the order part. If a kms
driver
wants to create a chain: encoder->bridge1->bridge2, it should ideally do:

drm_bridge_attach(encoder, bridge1, NULL);
drm_bridge_attach(encoder, bridge2, bridge1);


Correct.


We can't do much if the kms driver does the opposite:

drm_bridge_attach(encoder, bridge2, NULL);
drm_bridge_attach(encoder, bridge2, bridge1);


That would certainly be a very stupid thing for a driver to do :-) The
problem that we could catch with my current proposal is

drm_bridge_attach(encoder, bridge2, bridge1);
...
drm_bridge_attach(encoder, bridge1, NULL);

which I expect to happen from time to time as the two bridge can be
attached through separate code paths sometimes a bit difficult to trace.
It's not a big deal though, you could convince me that the advantages of
a simpler API exceed its drawbacks.


Having no 'previous' argument would prevent the possibility of this
altogether, won't it?

With no 'previous' arg in the API, the driver can only do:

drm_bridge_attach(encoder, bridge1);
drm_bridge_attach(encoder, bridge2);

or

drm_bridge_attach(encoder, bridge2);
drm_bridge_attach(encoder, bridge1);


Correct.


For the latter, we can't do much as discussed above.


Except that with the currently proposed API the code would be

drm_bridge_attach(encoder, bridge2, bridge1);
drm_bridge_attach(encoder, bridge1, NULL);

(correct case)

or

drm_bridge_attach(encoder, bridge2, bridge1);
drm_bridge_attach(encoder, bridge1, NULL);

(incorrect case)

The second one could be caught by the drm_bridge_attach() function as bridge1-

dev will be NULL when attaching bridge2 in the incorrect case.


Okay, I got it now.

As you said, it does make sense for cases like analogix_dp, where one
attach is in the bridge driver, and the other is in the kms driver.
It makes things more legible too. Passing 'previous' as NULL makes it
clear in the code that we're attaching first bridge in the chain.
Let's stick to your proposal.

One additional thing we could do is to compare the 'previous' arg
passed by the API with the last bridge in the chain, and return
an error if they aren't the same, just as an additional safety
measure.

Archit





--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Archit Taneja]

On 11/30/2016 03:53 PM, Laurent Pinchart wrote:

Hi Archit,

On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote:

On 11/29/2016 11:27 PM, Laurent Pinchart wrote:

On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:

On 11/29/2016 02:34 PM, Laurent Pinchart wrote:

Instead of linking encoders and bridges in every driver (and getting it
wrong half of the time, as many drivers forget to set the drm_bridge
encoder pointer), do so in core code. The drm_bridge_attach() function
needs the encoder and optional previous bridge to perform that task,
update all the callers.

Signed-off-by: Laurent Pinchart

---

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
 drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
 drivers/gpu/drm/drm_bridge.c   | 46 ++-
 drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
 drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
 drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
 drivers/gpu/drm/imx/parallel-display.c |  4 +-
 drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
 drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
 drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
 drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
 drivers/gpu/drm/sti/sti_hda.c  |  3 +-
 drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
 include/drm/drm_bridge.h   |  3 +-
 23 files changed, 83 insertions(+), 103 deletions(-)


[snip]


diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 0ee052b7c21a..850bd6509ef1 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c


[snip]


@@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 EXPORT_SYMBOL(drm_bridge_remove);

 /**
- * drm_bridge_attach - associate given bridge to our DRM device
+ * drm_bridge_attach - attach the bridge to an encoder's chain
  *
- * @dev: DRM device
- * @bridge: bridge control structure
+ * @encoder: DRM encoder
+ * @bridge: bridge to attach
+ * @previous: previous bridge in the chain (optional)
  *
- * Called by a kms driver to link one of our encoder/bridge to the
given
- * bridge.
+ * Called by a kms driver to link the bridge to an encoder's chain. The
previous
+ * argument specifies the previous bridge in the chain. If NULL, the
bridge is
+ * linked directly at the encoder's output. Otherwise it is linked at
the
+ * previous bridge's output.
  *
- * Note that setting up links between the bridge and our encoder/bridge
- * objects needs to be handled by the kms driver itself.
+ * If non-NULL the previous bridge must be already attached by a call
to this
+ * function.
  *
  * RETURNS:
  * Zero on success, error code on failure
  */
-int drm_bridge_attach(struct drm_device *dev, struct drm_bridge
*bridge)
+int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
*bridge,
+ struct drm_bridge *previous)
 {
-   if (!dev || !bridge)
+   int ret;
+
+   if (!encoder || !bridge)
+   return -EINVAL;


I think we could derive previous from the encoder itself. Something like:
previous = encoder->bridge;
while (previous && previous->next)

previous = previous->next;


That's a very good point. It would however prevent us from catching
drivers that attach bridges in the wrong order, which the !previous->dev
currently allows us to do (and it should be turned into a WARN_ON as
Daniel proposed).


With the simpler API, I don't think we will ever hit the case of
!previous->dev. The previous bridge (if it exists) in the chain would
already have a dev attached to it. In other words, we would remove the
risk of the chance of the 'previous' bridge being unattached.

I'm a bit unclear about what you mean about the order part. If a kms driver
wants to create a chain: encoder->bridge1->bridge2, it should ideally do:

drm_bridge_attach(encoder, bridge1, NULL);
drm_bridge_attach(encoder, bridge2, bridge1);


Correct.


We can't do much if the kms driver does the opposite:

drm_bridge_attach(encoder, bridge2, NULL);
drm_bridge_attach(encoder, bridge2, bridge1);


That would certainly be a very stupid thing for a driver to do :-) The problem
that we could catch with my current proposal is

drm_bridge_attach(encoder, bridge2, 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Archit,

On Wednesday 30 Nov 2016 16:30:53 Archit Taneja wrote:
> On 11/30/2016 03:53 PM, Laurent Pinchart wrote:
> > On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote:
> >> On 11/29/2016 11:27 PM, Laurent Pinchart wrote:
> >>> On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:
>  On 11/29/2016 02:34 PM, Laurent Pinchart wrote:
> > Instead of linking encoders and bridges in every driver (and getting
> > it wrong half of the time, as many drivers forget to set the
> > drm_bridge encoder pointer), do so in core code. The
> > drm_bridge_attach() function needs the encoder and optional previous
> > bridge to perform that task, update all the callers.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---

[snip]

>  I think we could derive previous from the encoder itself. Something
>  like:
> 
>   previous = encoder->bridge;
>   while (previous && previous->next)
>   previous = previous->next;
> >>> 
> >>> That's a very good point. It would however prevent us from catching
> >>> drivers that attach bridges in the wrong order, which the !previous->dev
> >>> currently allows us to do (and it should be turned into a WARN_ON as
> >>> Daniel proposed).
> >> 
> >> With the simpler API, I don't think we will ever hit the case of
> >> !previous->dev. The previous bridge (if it exists) in the chain would
> >> already have a dev attached to it. In other words, we would remove the
> >> risk of the chance of the 'previous' bridge being unattached.
> >> 
> >> I'm a bit unclear about what you mean about the order part. If a kms
> >> driver
> >> wants to create a chain: encoder->bridge1->bridge2, it should ideally do:
> >> 
> >> drm_bridge_attach(encoder, bridge1, NULL);
> >> drm_bridge_attach(encoder, bridge2, bridge1);
> > 
> > Correct.
> > 
> >> We can't do much if the kms driver does the opposite:
> >> 
> >> drm_bridge_attach(encoder, bridge2, NULL);
> >> drm_bridge_attach(encoder, bridge2, bridge1);
> > 
> > That would certainly be a very stupid thing for a driver to do :-) The
> > problem that we could catch with my current proposal is
> > 
> > drm_bridge_attach(encoder, bridge2, bridge1);
> > ...
> > drm_bridge_attach(encoder, bridge1, NULL);
> > 
> > which I expect to happen from time to time as the two bridge can be
> > attached through separate code paths sometimes a bit difficult to trace.
> > It's not a big deal though, you could convince me that the advantages of
> > a simpler API exceed its drawbacks.
> 
> Having no 'previous' argument would prevent the possibility of this
> altogether, won't it?
> 
> With no 'previous' arg in the API, the driver can only do:
> 
> drm_bridge_attach(encoder, bridge1);
> drm_bridge_attach(encoder, bridge2);
> 
> or
> 
> drm_bridge_attach(encoder, bridge2);
> drm_bridge_attach(encoder, bridge1);

Correct.

> For the latter, we can't do much as discussed above.

Except that with the currently proposed API the code would be

drm_bridge_attach(encoder, bridge2, bridge1);
drm_bridge_attach(encoder, bridge1, NULL);

(correct case)

or

drm_bridge_attach(encoder, bridge2, bridge1);
drm_bridge_attach(encoder, bridge1, NULL);

(incorrect case)

The second one could be caught by the drm_bridge_attach() function as bridge1-
>dev will be NULL when attaching bridge2 in the incorrect case.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Archit,

On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote:
> On 11/29/2016 11:27 PM, Laurent Pinchart wrote:
> > On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:
> >> On 11/29/2016 02:34 PM, Laurent Pinchart wrote:
> >>> Instead of linking encoders and bridges in every driver (and getting it
> >>> wrong half of the time, as many drivers forget to set the drm_bridge
> >>> encoder pointer), do so in core code. The drm_bridge_attach() function
> >>> needs the encoder and optional previous bridge to perform that task,
> >>> update all the callers.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> 
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
> >>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
> >>>  drivers/gpu/drm/drm_bridge.c   | 46 ++-
> >>>  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
> >>>  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
> >>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
> >>>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
> >>>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
> >>>  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
> >>>  drivers/gpu/drm/imx/parallel-display.c |  4 +-
> >>>  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
> >>>  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
> >>>  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
> >>>  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
> >>>  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
> >>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
> >>>  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
> >>>  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
> >>>  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
> >>>  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
> >>>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
> >>>  include/drm/drm_bridge.h   |  3 +-
> >>>  23 files changed, 83 insertions(+), 103 deletions(-)
> > 
> > [snip]
> > 
> >>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >>> index 0ee052b7c21a..850bd6509ef1 100644
> >>> --- a/drivers/gpu/drm/drm_bridge.c
> >>> +++ b/drivers/gpu/drm/drm_bridge.c
> > 
> > [snip]
> > 
> >>> @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> >>>  EXPORT_SYMBOL(drm_bridge_remove);
> >>>  
> >>>  /**
> >>> - * drm_bridge_attach - associate given bridge to our DRM device
> >>> + * drm_bridge_attach - attach the bridge to an encoder's chain
> >>>   *
> >>> - * @dev: DRM device
> >>> - * @bridge: bridge control structure
> >>> + * @encoder: DRM encoder
> >>> + * @bridge: bridge to attach
> >>> + * @previous: previous bridge in the chain (optional)
> >>>   *
> >>> - * Called by a kms driver to link one of our encoder/bridge to the
> >>> given
> >>> - * bridge.
> >>> + * Called by a kms driver to link the bridge to an encoder's chain. The
> >>> previous
> >>> + * argument specifies the previous bridge in the chain. If NULL, the
> >>> bridge is
> >>> + * linked directly at the encoder's output. Otherwise it is linked at
> >>> the
> >>> + * previous bridge's output.
> >>>   *
> >>> - * Note that setting up links between the bridge and our encoder/bridge
> >>> - * objects needs to be handled by the kms driver itself.
> >>> + * If non-NULL the previous bridge must be already attached by a call
> >>> to this
> >>> + * function.
> >>>   *
> >>>   * RETURNS:
> >>>   * Zero on success, error code on failure
> >>>   */
> >>> -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge
> >>> *bridge)
> >>> +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> >>> *bridge,
> >>> +   struct drm_bridge *previous)
> >>>  {
> >>> - if (!dev || !bridge)
> >>> + int ret;
> >>> +
> >>> + if (!encoder || !bridge)
> >>> + return -EINVAL;
> >> 
> >> I think we could derive previous from the encoder itself. Something like:
> >>previous = encoder->bridge;
> >>while (previous && previous->next)
> >>
> >>previous = previous->next;
> > 
> > That's a very good point. It would however prevent us from catching
> > drivers that attach bridges in the wrong order, which the !previous->dev
> > currently allows us to do (and it should be turned into a WARN_ON as
> > Daniel proposed).
>
> With the simpler API, I don't think we will ever hit the case of
> !previous->dev. The previous bridge (if it exists) in the chain would
> already have a dev attached to it. In other words, we would remove the
> risk of the chance of the 'previous' bridge being unattached.
> 
> I'm a bit unclear about what you mean about the order part. If a kms driver
> wants to 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Archit Taneja]

On 11/29/2016 11:27 PM, Laurent Pinchart wrote:

Hi Archit,

On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:

On 11/29/2016 02:34 PM, Laurent Pinchart wrote:

Instead of linking encoders and bridges in every driver (and getting it
wrong half of the time, as many drivers forget to set the drm_bridge
encoder pointer), do so in core code. The drm_bridge_attach() function
needs the encoder and optional previous bridge to perform that task,
update all the callers.

Signed-off-by: Laurent Pinchart

---

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
 drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
 drivers/gpu/drm/drm_bridge.c   | 46 -
 drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
 drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
 drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
 drivers/gpu/drm/imx/parallel-display.c |  4 +-
 drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
 drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
 drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
 drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
 drivers/gpu/drm/sti/sti_hda.c  |  3 +-
 drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
 include/drm/drm_bridge.h   |  3 +-
 23 files changed, 83 insertions(+), 103 deletions(-)


[snip]


diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 0ee052b7c21a..850bd6509ef1 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c


[snip]


@@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 EXPORT_SYMBOL(drm_bridge_remove);

 /**
- * drm_bridge_attach - associate given bridge to our DRM device
+ * drm_bridge_attach - attach the bridge to an encoder's chain
  *
- * @dev: DRM device
- * @bridge: bridge control structure
+ * @encoder: DRM encoder
+ * @bridge: bridge to attach
+ * @previous: previous bridge in the chain (optional)
  *
- * Called by a kms driver to link one of our encoder/bridge to the given
- * bridge.
+ * Called by a kms driver to link the bridge to an encoder's chain. The
previous
+ * argument specifies the previous bridge in the chain. If NULL, the
bridge is
+ * linked directly at the encoder's output. Otherwise it is linked at the
+ * previous bridge's output.
  *
- * Note that setting up links between the bridge and our encoder/bridge
- * objects needs to be handled by the kms driver itself.
+ * If non-NULL the previous bridge must be already attached by a call to
this
+ * function.
  *
  * RETURNS:
  * Zero on success, error code on failure
  */
-int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
+int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
*bridge,
+ struct drm_bridge *previous)
 {
-   if (!dev || !bridge)
+   int ret;
+
+   if (!encoder || !bridge)
+   return -EINVAL;


I think we could derive previous from the encoder itself. Something like:

previous = encoder->bridge;
while (previous && previous->next)
previous = previous->next;


That's a very good point. It would however prevent us from catching drivers
that attach bridges in the wrong order, which the !previous->dev currently
allows us to do (and it should be turned into a WARN_ON as Daniel proposed).



With the simpler API, I don't think we will ever hit the case of
!previous->dev. The previous bridge (if it exists) in the chain would
already have a dev attached to it. In other words, we would remove the
risk of the chance of the 'previous' bridge being unattached.

I'm a bit unclear about what you mean about the order part. If a kms driver
wants to create a chain: encoder->bridge1->bridge2, it should ideally do:

drm_bridge_attach(encoder, bridge1, NULL);
drm_bridge_attach(encoder, bridge2, bridge1);

We can't do much if the kms driver does the opposite:

drm_bridge_attach(encoder, bridge2, NULL);
drm_bridge_attach(encoder, bridge2, bridge1);



I'm fine losing that ability, as your proposal makes the API simpler. I'll let
you decide, which option do you prefer ?


I prefer the simpler API. I guess the main aim of the patch was to prevent the
driver setting up the encoder<->bridge links, which will be done anyway.

Thanks,

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Daniel,

On Tuesday 29 Nov 2016 20:02:01 Laurent Pinchart wrote:
> On Tuesday 29 Nov 2016 11:05:27 Daniel Vetter wrote:
> > On Tue, Nov 29, 2016 at 11:43:19AM +0200, Laurent Pinchart wrote:
> >> On Tuesday 29 Nov 2016 10:35:24 Daniel Vetter wrote:
> >>> On Tue, Nov 29, 2016 at 11:04:33AM +0200, Laurent Pinchart wrote:
>  Instead of linking encoders and bridges in every driver (and getting
>  it wrong half of the time, as many drivers forget to set the
>  drm_bridge encoder pointer), do so in core code. The
>  drm_bridge_attach() function needs the encoder and optional previous
>  bridge to perform that task, update all the callers.
>  
>  Signed-off-by: Laurent Pinchart
>  
>  ---

[snip]

>  diff --git a/drivers/gpu/drm/drm_bridge.c
>  b/drivers/gpu/drm/drm_bridge.c
>  index 0ee052b7c21a..850bd6509ef1 100644
>  --- a/drivers/gpu/drm/drm_bridge.c
>  +++ b/drivers/gpu/drm/drm_bridge.c
> 
> [snip]
> 
>  -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge
>  *bridge)
>  +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
>  *bridge,
>  +  struct drm_bridge *previous)
>   {
>  -if (!dev || !bridge)
>  +int ret;
>  +
>  +if (!encoder || !bridge)
>  +return -EINVAL;
>  +
>  +if (previous && (!previous->dev || previous->encoder !=
>  encoder))
>   return -EINVAL;
> >>> 
> >>> Not sure we want to allow setting both encoder and bridge for chaining.
> >>> I'd kinda expect that for chained use-case the bridge doesn't care that
> >>> much who started the chain. And if not we can always create a helper to
> >>> look up the drm_encoder.
> >> 
> >> As bridge drivers currently create connectors (I'd like to change that
> >> at some point, but one thing at a time) they need to call
> >> drm_mode_connector_attach_encoder() and thus need to have access to the
> >> drm_encoder object at the beginning of the chain. The drm_bridge
> >> structure has an encoder field, it seems to me that the easiest is to
> >> always populate it, regardless of the position of the bridge in the
> >> chain.
> > 
> > I mean the function inteface, not what we fill out in the drm_bridge
> > struct. I.e. instead of expecting callers to give you the encoder for
> > 
> > chained bridges, look it up yourself:
> > bridge->encoder = previous : previous->encoder ? encoder;
> > 
> > Or something  like that. Just feels confusing to connect a bridge to both
> > a bridge _and_ the first encoder.
> 
> Right. Archit proposed doing it the other way around:
> 
> previous = encoder->bridge;
> while (previous && previous->next)
> previous = previous->next;
> 
> That would simplify the API, at the cost of preventing us from catching
> drivers that attach bridges in the wrong order (through the !previous->dev
> check that you suggested should be turned into a WARN_ON). The previous-
> 
> >encoder != encoder check is also a safety net.
> 
> Any opinion on which option you like better ? I'd be very tempted to go for
> Archit's proposal as it removes the previous parameter from the API, if it
> wasn't for the loss of sanity checking.
> 
> > Wrt creating the connector: Some helpers to make that easier could be
> > useful, and probably we need a separate ->register callback. That's needed
> > for proper demidlayered init/teardown sequence anyway, and then the
> > drm_bridge.c code make sure to only call ->register for the very last
> > bridge. Other bridges could still create ther connectors (less conditions
> > in the code), as long as they don't register them no one will ever see
> > them ;-)

One thing at a time, we'll get there :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Daniel,

On Tuesday 29 Nov 2016 11:05:27 Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 11:43:19AM +0200, Laurent Pinchart wrote:
> > On Tuesday 29 Nov 2016 10:35:24 Daniel Vetter wrote:
> >> On Tue, Nov 29, 2016 at 11:04:33AM +0200, Laurent Pinchart wrote:
> >>> Instead of linking encoders and bridges in every driver (and getting
> >>> it wrong half of the time, as many drivers forget to set the
> >>> drm_bridge encoder pointer), do so in core code. The
> >>> drm_bridge_attach() function needs the encoder and optional previous
> >>> bridge to perform that task, update all the callers.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> 
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
> >>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
> >>>  drivers/gpu/drm/drm_bridge.c   | 46 +
> >>>  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
> >>>  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
> >>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
> >>>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
> >>>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
> >>>  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
> >>>  drivers/gpu/drm/imx/parallel-display.c |  4 +-
> >>>  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
> >>>  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
> >>>  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
> >>>  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
> >>>  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
> >>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
> >>>  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
> >>>  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
> >>>  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
> >>>  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
> >>>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
> >>>  include/drm/drm_bridge.h   |  3 +-
> >>>  23 files changed, 83 insertions(+), 103 deletions(-)
> > 
> > [snip]
> > 
> >>> diff --git a/drivers/gpu/drm/drm_bridge.c
> >>> b/drivers/gpu/drm/drm_bridge.c
> >>> index 0ee052b7c21a..850bd6509ef1 100644
> >>> --- a/drivers/gpu/drm/drm_bridge.c
> >>> +++ b/drivers/gpu/drm/drm_bridge.c

[snip]

> >>> -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge
> >>> *bridge)
> >>> +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> >>> *bridge,
> >>> +   struct drm_bridge *previous)
> >>>  {
> >>> - if (!dev || !bridge)
> >>> + int ret;
> >>> +
> >>> + if (!encoder || !bridge)
> >>> + return -EINVAL;
> >>> +
> >>> + if (previous && (!previous->dev || previous->encoder != encoder))
> >>>   return -EINVAL;
> >> 
> >> Not sure we want to allow setting both encoder and bridge for chaining.
> >> I'd kinda expect that for chained use-case the bridge doesn't care that
> >> much who started the chain. And if not we can always create a helper to
> >> look up the drm_encoder.
> > 
> > As bridge drivers currently create connectors (I'd like to change that at
> > some point, but one thing at a time) they need to call
> > drm_mode_connector_attach_encoder() and thus need to have access to the
> > drm_encoder object at the beginning of the chain. The drm_bridge structure
> > has an encoder field, it seems to me that the easiest is to always
> > populate it, regardless of the position of the bridge in the chain.
> 
> I mean the function inteface, not what we fill out in the drm_bridge
> struct. I.e. instead of expecting callers to give you the encoder for
> chained bridges, look it up yourself:
> 
>   bridge->encoder = previous : previous->encoder ? encoder;
> 
> Or something  like that. Just feels confusing to connect a bridge to both
> a bridge _and_ the first encoder.

Right. Archit proposed doing it the other way around:

previous = encoder->bridge;
while (previous && previous->next)
previous = previous->next;

That would simplify the API, at the cost of preventing us from catching 
drivers that attach bridges in the wrong order (through the !previous->dev 
check that you suggested should be turned into a WARN_ON). The previous-
>encoder != encoder check is also a safety net.

Any opinion on which option you like better ? I'd be very tempted to go for 
Archit's proposal as it removes the previous parameter from the API, if it 
wasn't for the loss of sanity checking.

> Wrt creating the connector: Some helpers to make that easier could be
> useful, and probably we need a separate ->register callback. That's needed
> for proper demidlayered init/teardown sequence anyway, and then the
> drm_bridge.c 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Archit,

On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:
> On 11/29/2016 02:34 PM, Laurent Pinchart wrote:
> > Instead of linking encoders and bridges in every driver (and getting it
> > wrong half of the time, as many drivers forget to set the drm_bridge
> > encoder pointer), do so in core code. The drm_bridge_attach() function
> > needs the encoder and optional previous bridge to perform that task,
> > update all the callers.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
> >  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
> >  drivers/gpu/drm/drm_bridge.c   | 46 -
> >  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
> >  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
> >  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
> >  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
> >  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
> >  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
> >  drivers/gpu/drm/imx/parallel-display.c |  4 +-
> >  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
> >  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
> >  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
> >  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
> >  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
> >  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
> >  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
> >  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
> >  include/drm/drm_bridge.h   |  3 +-
> >  23 files changed, 83 insertions(+), 103 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 0ee052b7c21a..850bd6509ef1 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c

[snip]

> > @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> >  EXPORT_SYMBOL(drm_bridge_remove);
> >  
> >  /**
> > - * drm_bridge_attach - associate given bridge to our DRM device
> > + * drm_bridge_attach - attach the bridge to an encoder's chain
> >   *
> > - * @dev: DRM device
> > - * @bridge: bridge control structure
> > + * @encoder: DRM encoder
> > + * @bridge: bridge to attach
> > + * @previous: previous bridge in the chain (optional)
> >   *
> > - * Called by a kms driver to link one of our encoder/bridge to the given
> > - * bridge.
> > + * Called by a kms driver to link the bridge to an encoder's chain. The
> > previous
> > + * argument specifies the previous bridge in the chain. If NULL, the
> > bridge is
> > + * linked directly at the encoder's output. Otherwise it is linked at the
> > + * previous bridge's output.
> >   *
> > - * Note that setting up links between the bridge and our encoder/bridge
> > - * objects needs to be handled by the kms driver itself.
> > + * If non-NULL the previous bridge must be already attached by a call to
> > this
> > + * function.
> >   *
> >   * RETURNS:
> >   * Zero on success, error code on failure
> >   */
> > -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
> > +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> > *bridge,
> > + struct drm_bridge *previous)
> >  {
> > -   if (!dev || !bridge)
> > +   int ret;
> > +
> > +   if (!encoder || !bridge)
> > +   return -EINVAL;
> 
> I think we could derive previous from the encoder itself. Something like:
> 
>   previous = encoder->bridge;
>   while (previous && previous->next)
>   previous = previous->next;

That's a very good point. It would however prevent us from catching drivers 
that attach bridges in the wrong order, which the !previous->dev currently 
allows us to do (and it should be turned into a WARN_ON as Daniel proposed).

I'm fine losing that ability, as your proposal makes the API simpler. I'll let 
you decide, which option do you prefer ?

> > +
> > +   if (previous && (!previous->dev || previous->encoder != encoder))
> > return -EINVAL;
> > 
> > if (bridge->dev)
> > return -EBUSY;
> > 
> > -   bridge->dev = dev;
> > +   bridge->dev = encoder->dev;
> > +   bridge->encoder = encoder;
> > +
> > +   if (bridge->funcs->attach) {
> > +   ret = bridge->funcs->attach(bridge);
> > +   if (ret < 0) {
> > +   bridge->dev = NULL;
> > +   bridge->encoder = NULL;
> > +   return ret;
> > +   }
> > +   }
> > 
> > -   

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Stefan Agner]

On 2016-11-29 01:04, Laurent Pinchart wrote:
> Instead of linking encoders and bridges in every driver (and getting it
> wrong half of the time, as many drivers forget to set the drm_bridge
> encoder pointer), do so in core code. The drm_bridge_attach() function
> needs the encoder and optional previous bridge to perform that task,
> update all the callers.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
>  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
>  drivers/gpu/drm/drm_bridge.c   | 46 
> --
>  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
>  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--

For DCU

Acked-by: Stefan Agner 

>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
>  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
>  drivers/gpu/drm/imx/parallel-display.c |  4 +-
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
>  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
>  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
>  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
>  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
>  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
>  include/drm/drm_bridge.h   |  3 +-
>  23 files changed, 83 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 6119b5085501..e7799b6ee829 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -230,9 +230,7 @@ static int atmel_hlcdc_attach_endpoint(struct
> drm_device *dev,
>   of_node_put(np);
>  
>   if (bridge) {
> - output->encoder.bridge = bridge;
> - bridge->encoder = >encoder;
> - ret = drm_bridge_attach(dev, bridge);
> + ret = drm_bridge_attach(>encoder, bridge, NULL);
>   if (!ret)
>   return 0;
>   }
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6e0447f329a2..1835f1fdad19 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1227,12 +1227,10 @@ static int analogix_dp_create_bridge(struct
> drm_device *drm_dev,
>  
>   dp->bridge = bridge;
>  
> - dp->encoder->bridge = bridge;
>   bridge->driver_private = dp;
> - bridge->encoder = dp->encoder;
>   bridge->funcs = _dp_bridge_funcs;
>  
> - ret = drm_bridge_attach(drm_dev, bridge);
> + ret = drm_bridge_attach(dp->encoder, bridge, NULL);
>   if (ret) {
>   DRM_ERROR("failed to attach drm bridge\n");
>   return -EINVAL;
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index b71088dab268..432e0e3fff72 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1841,13 +1841,12 @@ static int dw_hdmi_register(struct drm_device
> *drm, struct dw_hdmi *hdmi)
>   hdmi->bridge = bridge;
>   bridge->driver_private = hdmi;
>   bridge->funcs = _hdmi_bridge_funcs;
> - ret = drm_bridge_attach(drm, bridge);
> + ret = drm_bridge_attach(encoder, bridge, NULL);
>   if (ret) {
>   DRM_ERROR("Failed to initialize bridge with drm\n");
>   return -EINVAL;
>   }
>  
> - encoder->bridge = bridge;
>   hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>  
>   drm_connector_helper_add(>connector,
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 0ee052b7c21a..850bd6509ef1 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include 
> +#include 
>  
>  /**
>   * DOC: overview
> @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  EXPORT_SYMBOL(drm_bridge_remove);
>  
>  /**
> - * drm_bridge_attach - associate given bridge to our DRM device
> + * drm_bridge_attach - attach the bridge to an encoder's chain
>   *
> - * @dev: DRM device
> - * @bridge: bridge control structure
> + * @encoder: DRM encoder
> + * @bridge: bridge to attach
> + * 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Archit Taneja]

On 11/29/2016 02:34 PM, Laurent Pinchart wrote:

Instead of linking encoders and bridges in every driver (and getting it
wrong half of the time, as many drivers forget to set the drm_bridge
encoder pointer), do so in core code. The drm_bridge_attach() function
needs the encoder and optional previous bridge to perform that task,
update all the callers.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
 drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
 drivers/gpu/drm/drm_bridge.c   | 46 --
 drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
 drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
 drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
 drivers/gpu/drm/imx/parallel-display.c |  4 +-
 drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
 drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
 drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
 drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
 drivers/gpu/drm/sti/sti_hda.c  |  3 +-
 drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
 include/drm/drm_bridge.h   |  3 +-
 23 files changed, 83 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index 6119b5085501..e7799b6ee829 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -230,9 +230,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device 
*dev,
of_node_put(np);

if (bridge) {
-   output->encoder.bridge = bridge;
-   bridge->encoder = >encoder;
-   ret = drm_bridge_attach(dev, bridge);
+   ret = drm_bridge_attach(>encoder, bridge, NULL);
if (!ret)
return 0;
}
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6e0447f329a2..1835f1fdad19 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1227,12 +1227,10 @@ static int analogix_dp_create_bridge(struct drm_device 
*drm_dev,

dp->bridge = bridge;

-   dp->encoder->bridge = bridge;
bridge->driver_private = dp;
-   bridge->encoder = dp->encoder;
bridge->funcs = _dp_bridge_funcs;

-   ret = drm_bridge_attach(drm_dev, bridge);
+   ret = drm_bridge_attach(dp->encoder, bridge, NULL);
if (ret) {
DRM_ERROR("failed to attach drm bridge\n");
return -EINVAL;
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b71088dab268..432e0e3fff72 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1841,13 +1841,12 @@ static int dw_hdmi_register(struct drm_device *drm, 
struct dw_hdmi *hdmi)
hdmi->bridge = bridge;
bridge->driver_private = hdmi;
bridge->funcs = _hdmi_bridge_funcs;
-   ret = drm_bridge_attach(drm, bridge);
+   ret = drm_bridge_attach(encoder, bridge, NULL);
if (ret) {
DRM_ERROR("Failed to initialize bridge with drm\n");
return -EINVAL;
}

-   encoder->bridge = bridge;
hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;

drm_connector_helper_add(>connector,
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 0ee052b7c21a..850bd6509ef1 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include 

 #include 
+#include 

 /**
  * DOC: overview
@@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 EXPORT_SYMBOL(drm_bridge_remove);

 /**
- * drm_bridge_attach - associate given bridge to our DRM device
+ * drm_bridge_attach - attach the bridge to an encoder's chain
  *
- * @dev: DRM device
- * @bridge: bridge control structure
+ * @encoder: DRM encoder
+ * @bridge: bridge to attach
+ * @previous: previous bridge in the chain (optional)
  *
- * Called by a kms driver to link one of our encoder/bridge to the given
- * bridge.
+ * Called by a kms driver to link the bridge to an encoder's chain. The 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Daniel Vetter]

On Tue, Nov 29, 2016 at 11:43:19AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Tuesday 29 Nov 2016 10:35:24 Daniel Vetter wrote:
> > On Tue, Nov 29, 2016 at 11:04:33AM +0200, Laurent Pinchart wrote:
> > > Instead of linking encoders and bridges in every driver (and getting it
> > > wrong half of the time, as many drivers forget to set the drm_bridge
> > > encoder pointer), do so in core code. The drm_bridge_attach() function
> > > needs the encoder and optional previous bridge to perform that task,
> > > update all the callers.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > 
> > > ---
> > > 
> > >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
> > >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
> > >  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
> > >  drivers/gpu/drm/drm_bridge.c   | 46 +
> > >  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
> > >  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
> > >  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
> > >  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
> > >  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
> > >  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
> > >  drivers/gpu/drm/imx/parallel-display.c |  4 +-
> > >  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
> > >  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
> > >  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
> > >  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
> > >  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
> > >  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
> > >  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
> > >  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
> > >  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
> > >  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
> > >  include/drm/drm_bridge.h   |  3 +-
> > >  23 files changed, 83 insertions(+), 103 deletions(-)
> 
> [snip]
> 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 0ee052b7c21a..850bd6509ef1 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -26,6 +26,7 @@
> > > 
> > >  #include 
> > >  #include 
> > > +#include 
> > > 
> > >  /**
> > >   * DOC: overview
> > > @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> > >  EXPORT_SYMBOL(drm_bridge_remove);
> > >  
> > >  /**
> > > - * drm_bridge_attach - associate given bridge to our DRM device
> > > + * drm_bridge_attach - attach the bridge to an encoder's chain
> > >   *
> > > - * @dev: DRM device
> > > - * @bridge: bridge control structure
> > > + * @encoder: DRM encoder
> > > + * @bridge: bridge to attach
> > > + * @previous: previous bridge in the chain (optional)
> > >   *
> > > - * Called by a kms driver to link one of our encoder/bridge to the given
> > > - * bridge.
> > > + * Called by a kms driver to link the bridge to an encoder's chain. The
> > > previous
> > > + * argument specifies the previous bridge in the chain. If NULL, the
> > > bridge is
> > > + * linked directly at the encoder's output. Otherwise it is linked at the
> > > + * previous bridge's output.
> > >   *
> > > - * Note that setting up links between the bridge and our encoder/bridge
> > > - * objects needs to be handled by the kms driver itself.
> > > + * If non-NULL the previous bridge must be already attached by a call to
> > > this
> > > + * function.
> > >   *
> > >   * RETURNS:
> > >   * Zero on success, error code on failure
> > >   */
> > > -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
> > > +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> > > *bridge,
> > > +   struct drm_bridge *previous)
> > >  {
> > > - if (!dev || !bridge)
> > > + int ret;
> > > +
> > > + if (!encoder || !bridge)
> > > + return -EINVAL;
> > > +
> > > + if (previous && (!previous->dev || previous->encoder != encoder))
> > >   return -EINVAL;
> > 
> > Not sure we want to allow setting both encoder and bridge for chaining.
> > I'd kinda expect that for chained use-case the bridge doesn't care that
> > much who started the chain. And if not we can always create a helper to
> > look up the drm_encoder.
> 
> As bridge drivers currently create connectors (I'd like to change that at 
> some 
> point, but one thing at a time) they need to call 
> drm_mode_connector_attach_encoder() and thus need to have access to the 
> drm_encoder object at the beginning of the chain. The drm_bridge structure 
> has 
> an encoder field, it seems to me that the easiest is to always populate it, 
> regardless of 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Daniel,

On Tuesday 29 Nov 2016 10:35:24 Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 11:04:33AM +0200, Laurent Pinchart wrote:
> > Instead of linking encoders and bridges in every driver (and getting it
> > wrong half of the time, as many drivers forget to set the drm_bridge
> > encoder pointer), do so in core code. The drm_bridge_attach() function
> > needs the encoder and optional previous bridge to perform that task,
> > update all the callers.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
> >  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
> >  drivers/gpu/drm/drm_bridge.c   | 46 +
> >  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
> >  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
> >  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
> >  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
> >  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
> >  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
> >  drivers/gpu/drm/imx/parallel-display.c |  4 +-
> >  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
> >  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
> >  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
> >  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
> >  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
> >  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
> >  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
> >  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
> >  include/drm/drm_bridge.h   |  3 +-
> >  23 files changed, 83 insertions(+), 103 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 0ee052b7c21a..850bd6509ef1 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -26,6 +26,7 @@
> > 
> >  #include 
> >  #include 
> > +#include 
> > 
> >  /**
> >   * DOC: overview
> > @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> >  EXPORT_SYMBOL(drm_bridge_remove);
> >  
> >  /**
> > - * drm_bridge_attach - associate given bridge to our DRM device
> > + * drm_bridge_attach - attach the bridge to an encoder's chain
> >   *
> > - * @dev: DRM device
> > - * @bridge: bridge control structure
> > + * @encoder: DRM encoder
> > + * @bridge: bridge to attach
> > + * @previous: previous bridge in the chain (optional)
> >   *
> > - * Called by a kms driver to link one of our encoder/bridge to the given
> > - * bridge.
> > + * Called by a kms driver to link the bridge to an encoder's chain. The
> > previous
> > + * argument specifies the previous bridge in the chain. If NULL, the
> > bridge is
> > + * linked directly at the encoder's output. Otherwise it is linked at the
> > + * previous bridge's output.
> >   *
> > - * Note that setting up links between the bridge and our encoder/bridge
> > - * objects needs to be handled by the kms driver itself.
> > + * If non-NULL the previous bridge must be already attached by a call to
> > this
> > + * function.
> >   *
> >   * RETURNS:
> >   * Zero on success, error code on failure
> >   */
> > -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
> > +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> > *bridge,
> > + struct drm_bridge *previous)
> >  {
> > -   if (!dev || !bridge)
> > +   int ret;
> > +
> > +   if (!encoder || !bridge)
> > +   return -EINVAL;
> > +
> > +   if (previous && (!previous->dev || previous->encoder != encoder))
> > return -EINVAL;
> 
> Not sure we want to allow setting both encoder and bridge for chaining.
> I'd kinda expect that for chained use-case the bridge doesn't care that
> much who started the chain. And if not we can always create a helper to
> look up the drm_encoder.

As bridge drivers currently create connectors (I'd like to change that at some 
point, but one thing at a time) they need to call 
drm_mode_connector_attach_encoder() and thus need to have access to the 
drm_encoder object at the beginning of the chain. The drm_bridge structure has 
an encoder field, it seems to me that the easiest is to always populate it, 
regardless of the position of the bridge in the chain.

> Also, should we convert these to WARN_ON, they're kinda glaring driver (or
> well DT) bugs?

Yes, that seems like a good idea. I can add a patch on top of this one to 
convert the checks to WARN_ON, as I'd rather separate that change 

Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Daniel Vetter]

On Tue, Nov 29, 2016 at 11:04:33AM +0200, Laurent Pinchart wrote:
> Instead of linking encoders and bridges in every driver (and getting it
> wrong half of the time, as many drivers forget to set the drm_bridge
> encoder pointer), do so in core code. The drm_bridge_attach() function
> needs the encoder and optional previous bridge to perform that task,
> update all the callers.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
>  drivers/gpu/drm/bridge/dw-hdmi.c   |  3 +-
>  drivers/gpu/drm/drm_bridge.c   | 46 
> --
>  drivers/gpu/drm/drm_simple_kms_helper.c|  4 +-
>  drivers/gpu/drm/exynos/exynos_dp.c |  5 +--
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c|  6 +--
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  5 +--
>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  5 +--
>  drivers/gpu/drm/imx/imx-ldb.c  |  6 +--
>  drivers/gpu/drm/imx/parallel-display.c |  4 +-
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  8 ++--
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c| 11 +++---
>  drivers/gpu/drm/msm/dsi/dsi_manager.c  | 17 +---
>  drivers/gpu/drm/msm/edp/edp_bridge.c   |  2 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c  |  5 +--
>  drivers/gpu/drm/sti/sti_dvo.c  |  3 +-
>  drivers/gpu/drm/sti/sti_hda.c  |  3 +-
>  drivers/gpu/drm/sti/sti_hdmi.c |  3 +-
>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 13 +++---
>  include/drm/drm_bridge.h   |  3 +-
>  23 files changed, 83 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 6119b5085501..e7799b6ee829 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -230,9 +230,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device 
> *dev,
>   of_node_put(np);
>  
>   if (bridge) {
> - output->encoder.bridge = bridge;
> - bridge->encoder = >encoder;
> - ret = drm_bridge_attach(dev, bridge);
> + ret = drm_bridge_attach(>encoder, bridge, NULL);
>   if (!ret)
>   return 0;
>   }
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6e0447f329a2..1835f1fdad19 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1227,12 +1227,10 @@ static int analogix_dp_create_bridge(struct 
> drm_device *drm_dev,
>  
>   dp->bridge = bridge;
>  
> - dp->encoder->bridge = bridge;
>   bridge->driver_private = dp;
> - bridge->encoder = dp->encoder;
>   bridge->funcs = _dp_bridge_funcs;
>  
> - ret = drm_bridge_attach(drm_dev, bridge);
> + ret = drm_bridge_attach(dp->encoder, bridge, NULL);
>   if (ret) {
>   DRM_ERROR("failed to attach drm bridge\n");
>   return -EINVAL;
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index b71088dab268..432e0e3fff72 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1841,13 +1841,12 @@ static int dw_hdmi_register(struct drm_device *drm, 
> struct dw_hdmi *hdmi)
>   hdmi->bridge = bridge;
>   bridge->driver_private = hdmi;
>   bridge->funcs = _hdmi_bridge_funcs;
> - ret = drm_bridge_attach(drm, bridge);
> + ret = drm_bridge_attach(encoder, bridge, NULL);
>   if (ret) {
>   DRM_ERROR("Failed to initialize bridge with drm\n");
>   return -EINVAL;
>   }
>  
> - encoder->bridge = bridge;
>   hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>  
>   drm_connector_helper_add(>connector,
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 0ee052b7c21a..850bd6509ef1 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include 
> +#include 
>  
>  /**
>   * DOC: overview
> @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  EXPORT_SYMBOL(drm_bridge_remove);
>  
>  /**
> - * drm_bridge_attach - associate given bridge to our DRM device
> + * drm_bridge_attach - attach the bridge to an encoder's chain
>   *
> - * @dev: DRM device
> - * @bridge: bridge control structure
> + * @encoder: DRM encoder
> + * @bridge: bridge to attach
> + * @previous: previous bridge