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

[Jonas Karlman]

On 2020-01-23 08:39, Boris Brezillon wrote:
> On Wed, 22 Jan 2020 23:44:28 +0000 (UTC)
> Jonas Karlman <jonas at 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;
 
 		out_bus_fmts = funcs->atomic_get_output_bus_fmts(last_bridge,

Regards,
Jonas

> 
>>
>> [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200122-bus-format
>>
>> Regards,
>> Jonas