Received-SPF: pass (google.com: domain of linux-kernel+bounces-186434-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 22 May 2024 18:31:51 +0300
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Michal Simek <michal.simek@amd.com>,
	dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
Message-ID: <20240522153151.GB9789@pendragon.ideasonboard.com>
References: <20240520-dp-layer-enable-v1-1-c9b481209115@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240520-dp-layer-enable-v1-1-c9b481209115@amd.com>

Hi Anatoliy,

Thank you for the patch.

On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> Unconditionally enable the DPSUB layer in the corresponding atomic plane
> update callback. Setting the new display mode may require disabling and
> re-enabling the CRTC. This effectively resets DPSUB to the default state
> with all layers disabled.

Ah, I went through the code and I see that. Oops.

> The original implementation of the plane atomic
> update enables the corresponding DPSUB layer only if the framebuffer
> format has changed. This would leave the layer disabled after switching to
> a different display mode with the same framebuffer format.

Do we need a Fixes: tag or has this issue been there since the beginning
?

> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index 43bf416b33d5..c4f038e34814 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -120,9 +120,8 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
>  		zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
>  						   plane->state->alpha >> 8);
>  
> -	/* Enable or re-enable the plane if the format has changed. */
> -	if (format_changed)
> -		zynqmp_disp_layer_enable(layer);
> +	/* Enable or re-enable the plane. */
> +	zynqmp_disp_layer_enable(layer);

This should be safe for now, as the function will just write the plane
registers with identical values. The waste of CPU cycles may not be a
big issue, even if it would be best to avoid it.

What bothers me more is that we may be working around a larger problem.
Resetting the CRTC when disabling it affects the hardware state of the
whole device, and thus the state of all software DRM objects. The
hardware and software states lose sync. Maybe this patch is all that is
needed for now, but other similar issues could pop up in the future.

Would it be possible, at atomic check time, to detect the objects whose
hardware state will need to be synced, and marked that in their state ?
Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the layer
based on that. You may need to subclass the drm_plane_state if there's
no field in that structure that is suitable to store the information.
The format_changed local variable would move there.

>  }
>  
>  static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {
> 
> ---
> base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> change-id: 20240520-dp-layer-enable-7b561af29ca8

-- 
Regards,

Laurent Pinchart