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
<laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
---
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, 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);
For the latter, we can't do much as discussed above.
Archit
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.
If you still prefer the simpler API after reading the above, I'll update my
patch.
+
+ 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;
+ }
+ }
- if (bridge->funcs->attach)
- return bridge->funcs->attach(bridge);
+ if (previous)
+ previous->next = bridge;
+ else
+ encoder->bridge = bridge;
return 0;
}
<snip>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
