Re: [PATCH v7 06/12] drm/bridge: Add the necessary bits to support bus format negotiation

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Jonas Karlman <jonas@kwiboo.se>
Cc: dri-devel@lists.freedesktop.org,
	Lucas Stach <l.stach@pengutronix.de>,
	Chris Healy <cphealy@gmail.com>,
	Andrey Smirnov <andrew.smirnov@gmail.com>,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	kernel@collabora.com, Daniel Vetter <daniel@ffwll.ch>,
	Thierry Reding <thierry.reding@gmail.com>,
	Sam Ravnborg <sam@ravnborg.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v7 06/12] drm/bridge: Add the necessary bits to support bus format negotiation
Date: Thu, 23 Jan 2020 09:07:00 +0100	[thread overview]
Message-ID: <20200123090700.3b878cd5@collabora.com> (raw)
In-Reply-To: <cc8ce5e0-f502-e3ee-e677-eeb7ad532513@kwiboo.se>

On Thu, 23 Jan 2020 07:52:59 +0000 (UTC)
Jonas Karlman <jonas@kwiboo.se> wrote:

> On 2020-01-23 08:39, Boris Brezillon wrote:
> > On Wed, 22 Jan 2020 23:44:28 +0000 (UTC)
> > Jonas Karlman <jonas@kwiboo.se> wrote:
> >   
> >>> +static int
> >>> +drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
> >>> +					struct drm_crtc_state *crtc_state,
> >>> +					struct drm_connector_state *conn_state)
> >>> +{
> >>> +	struct drm_connector *conn = conn_state->connector;
> >>> +	struct drm_encoder *encoder = bridge->encoder;
> >>> +	struct drm_bridge_state *last_bridge_state;
> >>> +	unsigned int i, num_out_bus_fmts;
> >>> +	struct drm_bridge *last_bridge;
> >>> +	u32 *out_bus_fmts;
> >>> +	int ret = 0;
> >>> +
> >>> +	last_bridge = list_last_entry(&encoder->bridge_chain,
> >>> +				      struct drm_bridge, chain_node);
> >>> +	last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> >>> +							    last_bridge);
> >>> +
> >>> +	if (last_bridge->funcs->atomic_get_output_bus_fmts) {
> >>> +		const struct drm_bridge_funcs *funcs = last_bridge->funcs;
> >>> +
> >>> +		/*
> >>> +		 * If the driver implements ->atomic_get_output_bus_fmts() it
> >>> +		 * should also implement the atomic state hooks.
> >>> +		 */
> >>> +		if (WARN_ON(last_bridge_state))    
> >>
> >> This looks wrong, with this changed to WARN_ON(!last_bridge_state)
> >> my RK3328 HDMI2.0/YUV444/YUV420/10-bit branch at [1] starts working.
> >>
> >> With WARN_ON(last_bridge_state) I get:
> >>
> >> [    6.606658] WARNING: CPU: 0 PID: 167 at drivers/gpu/drm/drm_bridge.c:746 drm_atomic_bridge_chain_check+0x2b8/0x308
> >> [    6.606673] Hardware name: Pine64 Rock64 (DT)
> >>
> >> [    6.606754] Call trace:
> >> [    6.606759]  drm_atomic_bridge_chain_check+0x2b8/0x308
> >> [    6.606764]  drm_atomic_helper_check_modeset+0x89c/0xab8
> >> [    6.606768]  drm_atomic_helper_check+0x1c/0xa0
> >> [    6.606772]  drm_atomic_check_only+0x464/0x708
> >> [    6.606777]  drm_atomic_commit+0x18/0x58  
> > 
> > Add
> > 
> > const drm_bridge_funcs ... = {
> > 	...
> > 	.atomic_reset = drm_atomic_helper_bridge_reset,
> > 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > 	...
> > };
> > 
> > and that should work.  
> 
> That is what I added and what made that this warning is being triggered.
> The comment state that atomic state is needed, but the check warns when there is a state.
> 
> I have this:
> 
> static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
> 	...
> 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> 	.atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts,
> 	.atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts,
> 	.atomic_check = dw_hdmi_bridge_atomic_check,
> 	.atomic_reset = drm_atomic_helper_bridge_reset,
> 	...
> };
> 
> and
> 
> static const struct drm_bridge_funcs dw_hdmi_rockchip_bridge_funcs = {
> 	...
> 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> 	.atomic_get_input_bus_fmts = dw_hdmi_rockchip_get_input_bus_fmts,
> 	.atomic_check = dw_hdmi_rockchip_bridge_atomic_check,
> 	.atomic_reset = drm_atomic_helper_bridge_reset,
> };
> 
> after applying the following I got a hdmi signal again
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 0c28816146ba..7e7b0fac8f4f 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -743,7 +743,7 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
>  		 * If the driver implements ->atomic_get_output_bus_fmts() it
>  		 * should also implement the atomic state hooks.
>  		 */
> -		if (WARN_ON(last_bridge_state))
> +		if (WARN_ON(!last_bridge_state))
>  			return -EINVAL;

My bad, I didn't read your email carefully. You're right, I'll fix it
in v8.

Thanks,

Boris