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.
> > 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>
--
Regards,
Laurent Pinchart
