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
<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);
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,
Archit
+
+ 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
