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@cerno.tech>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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