Re: [PATCH RFC 06/19] drm/bridge: Create drm_bridge_chain_xx() wrappers

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	dri-devel@lists.freedesktop.org,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Chris Healy <Chris.Healy@zii.aero>,
	kernel@collabora.com, Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH RFC 06/19] drm/bridge: Create drm_bridge_chain_xx() wrappers
Date: Wed, 21 Aug 2019 17:53:45 +0200	[thread overview]
Message-ID: <20190821175345.6d29a34f@collabora.com> (raw)
In-Reply-To: <20190821144504.GF5433@pendragon.ideasonboard.com>

On Wed, 21 Aug 2019 17:45:04 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 05:11:37PM +0200, Boris Brezillon wrote:
> > DRM bridges should not be operated independently. Let's rename the
> > drm_bridge_xxx() helpers that take the first bridge of the chain and
> > iterate over the whole chain into drm_bridge_chain_xx(). We also pass
> > it the encoder containing the bridge chain instead of the dereferencing
> > encoder->bridge.  
> 
> I'm not sure about this. I do agree that the helpers operate on a chain,
> but I think they're use for calling them on any bridge, especially in
> conjunction with your work. The way I see it is that when a bridge in
> the chain needs a custom enable/disable sequence (flagged by some kind
> of flag in the bridge structure), the helpers will not automatically
> propagate the calls down the chain, and let the bridge call the pre/post
> enable/disable operations on the downstream bridge. This means that
> those bridges with special needs will have to call the helpers on the
> next bridge down the chain, and thus require keeping the ability to do
> so.

Also changed my mind on this one after the discussion we had. I
kept the rename part but dropper the s/bridge/encoder/ change. This way,
bridges that need to control the enable/disable sequence can use those
helpers on a sub-chain (the chain starting at the bridge element just
after them).

> 
> We could of course propose two sets of helpers, one taking a bridge
> pointer, and another one taking an encoder pointer, but I think it's a
> bit overkill. Especially if you consider my comments earlier in this
> series where I propose moving the custom sequence feature to bridges
> instead of encoders, I don't think this patch will be needed.

Agreed. As said above, I kept the rename part of this patch because I
think it better reflects the fact that those helpers are acting on a
bridge chain, and not a single bridge element.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel