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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34dec867-dbb0-5175-351a-ca4b1bd807b6@suse.de>
Date:   Wed, 11 Jan 2023 14:56:30 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Maxime Ripard <maxime@...no.tech>, Emma Anholt <emma@...olt.net>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>
Subject: Re: [PATCH 1/9] drm/vc4: hdmi: Update all the planes if the TV
 margins are changed

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@...no.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>

> ---
>   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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)