Re: [PATCH 1/9] drm/vc4: hdmi: Update all the planes if the TV margins are changed

[Thomas Zimmermann]

Hi

Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> On VC4, the TV margins on the HDMI connector are implemented by scaling
> the planes.
>
> However, if only the TV margins or the connector are changed by a new
> state, the planes ending up on that connector won't be. Thus, they won't
> be updated properly and we'll effectively ignore that change until the
> next commit affecting these planes.
>
> Let's make sure to add all the planes attached to the connector so that
> we can update them properly.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>

Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 12a00d644b61..0eafaf0b76e5 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -522,6 +522,22 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>   	if (!crtc)
>   		return 0;
>   
> +	if (old_state->tv.margins.left != new_state->tv.margins.left ||
> +	    old_state->tv.margins.right != new_state->tv.margins.right ||
> +	    old_state->tv.margins.top != new_state->tv.margins.top ||
> +	    old_state->tv.margins.bottom != new_state->tv.margins.bottom) {
> +		struct drm_crtc_state *crtc_state;
> +		int ret;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		ret = drm_atomic_add_affected_planes(state, crtc);

This is slightly dangerous, but works in the given case. I'd appreciate 
a comment ala 'plane state will be checked by atomic helpers'.

The plane state of the added planes has to be tested via their 
atomic_check helpers. That's no problem here because the connector's 
atomic_check runs before the plane's atomic_check. See _check_modeset vs 
_check_planes in [1].

We had code that invoked drm_atomic_add_affected_planes() from the 
CRTC's atomic_check. At that point, the plane's atomic_check has been 
executed already and the newly added planes were never really state-checked.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.1.4/source/drivers/gpu/drm/drm_atomic_helper.c#L1069

> +		if (ret)
> +			return ret;
> +	}
> +
>   	if (old_state->colorspace != new_state->colorspace ||
>   	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
>   		struct drm_crtc_state *crtc_state;

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature