Re: [PATCH RFC 02/19] drm: Support custom encoder/bridge enable/disable sequences officially

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 02/19] drm: Support custom encoder/bridge enable/disable sequences officially
Date: Wed, 21 Aug 2019 17:30:19 +0200	[thread overview]
Message-ID: <20190821173019.379b1e98@collabora.com> (raw)
In-Reply-To: <20190820190505.GP10820@pendragon.ideasonboard.com>

On Tue, 20 Aug 2019 22:05:05 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote:
> > The VC4 and Exynos DSI encoder drivers need a custom enable/disable
> > sequence and manually set encoder->bridge to NULL to prevent the core
> > from calling the bridge hooks.
> > 
> > Let's provide a more official way to support custom bridge/encoder
> > enable/disable sequences.  
> 
> So, while I'm not opposed to this, I think it's a bit of a hack, and I'd
> like to share my vision of how I'd like to see this being implemented in
> the more distant future (and thus possibly on top of this change).

I agree, it definitely looks like a hack, but I find it less hack-ish
than letting drivers re-initialize the encoder->bridge field to NULL
just after calling bridge_attach() :P.

> 
> It has been established for quite some time now that exposing
> drm_encoder to userspace was likely a mistake, and that it should have
> stayed a kernel-only object. With the generalised usage of drm_bridge, I
> would go one step further: drm_encoder shouldn't matter for DRM/KMS
> drivers at all.

I couldn't agree more.

> 
> drm_bridge has been introduced to model chained encoders, where the
> second (and subsequent) encoders couldn't easily be supported in a
> standard and reusable way with just drm_encoder. A bridge (with the
> exception of the panel bridge) is just an encoder. It shouldn't matter
> whether encoders are internal to the display controller, separate IPs in
> the SoC or external components, they could all be modelled as bridges.
> We do have bridges for DSI encoder IPs, and DSI (and other) bridges
> potentially need similar custom enable/disable sequences. I would thus
> ideally like to see the VC4 and Exynos DSI encoders being implemented as
> bridges, with support for custom enable/disable sequences in bridges,
> and drop support for custom enable/disable sequences in drm_encoder.

Looks like we're on the same page.

> 
> Does this make sense to you ?

Definitely.

> While I would love to see this being
> implemented right away, it may be too much work as a prerequisite for
> this bus format negotiation series, so I don't want to reject this
> patch. I would however like to already make sure we agree on the way
> forward for the next steps.

Actually, I think I have something [1] that implements what you're
proposing here (which is not surprising since we discussed it on
IRC ;)). I'll send a v2 so you can comment on the new approach.

[1]https://github.com/bbrezillon/linux-0day/commits/drm-bridge-busfmt-2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel