Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Boris Brezillon <boris.brezillon@collabora.com>,
	Jitao Shi <jitao.shi@mediatek.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	dri-devel@lists.freedesktop.org,
	Mark Rutland <mark.rutland@arm.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	kernel@collabora.com,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	Andrey Smirnov <andrew.smirnov@gmail.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chris Healy <cphealy@gmail.com>,
	devicetree@vger.kernel.org, Jonas Karlman <jonas@kwiboo.se>,
	Rob Herring <robh+dt@kernel.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Seung-Woo Kim <sw0312.kim@samsung.com>
Subject: Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Date: Wed, 25 Dec 2019 03:36:56 +0200	[thread overview]
Message-ID: <20191225013656.GB28817@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20191224113111.GB14837@ravnborg.org>

Hi Sam,

On Tue, Dec 24, 2019 at 12:31:11PM +0100, Sam Ravnborg wrote:
> > Just a reminder of my 2 proposals:
> > 
> > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> >    split your enable/disable logic in 2 parts and make sure things are
> >    ready when the panel/next bridge tries to send DSI commands
> > 2/ move everything that's needed to send DSI commands out of the
> >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you
> >    can call that in the DSI transfer path too
> > 
> > As pointed out by Laurent, #1 doesn't work because some panel drivers
> > send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> > are called in reverse order, meaning that the DRM panel bridge driver
> > would try to issue DSI commands before the DSI host controllers is ready
> > to send them. I still thing #2 is a good option.
> 
> Jitao Shi suggested to extend panels so we had a sequence of:
> 
>   prepare_power()  <= new callback,
>                    here one should NOT be allowed to send
>                    DSI commands
>   prepare()
>   enable()
> 
>    #
>    # panel is now ready to show your favourite christmas movie
>    #
> 
>   disable()
>   unprepare()
>   unprepare_power()  <= new callback

Please note that you will need corresponding bridge operations in order
to implement the DRM panel bridge.

> Would this help implement what you suggest above?
> Relevant panels would then have to be updated - but this
> is doable.

The fundamental issue is that the enable/disable sequence requirements
are inherently driven by sinks, while the bridge API goes from source to
sink. A DSI panel could for instance require the following enable
sequence.

0. DSI bus disabled (LP-00 state)
1. Power up the panel
2. Enable the DSI bus (LP-11 state)
3. Configure the panel (through DCS LP mode writes, SPI, GPIOs, ...)
4. Transition the source to HS mode, sending sync packets, but no data
5. Perform additional configuration for the panel
6. Enable transmission of video from the source

For a given bus type there are thus well-specified states for a source
(in this case disabled, enabled in LP mode, in HS mode, sending sync
packets, sending video packets).

As our API operates from source to sink, fine-grained control of bridges
is difficult. We can of course always make bridge and panel operations
more fine-grained, but that's more of a ad-hoc solution that may lead to
abuse. We would at the very least document very explicitly what each
operation would be allowed to do for every bus type.

-- 
Regards,

Laurent Pinchart