Message-ID: <a974def2-ee85-4caf-83bc-fa59116fc292@suse.de>
Date: Tue, 22 Apr 2025 09:24:12 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Ryosuke Yasuoka <ryasuoka@...hat.com>, drawat.floss@...il.com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, airlied@...il.com,
 simona@...ll.ch, jfalempe@...hat.com
Cc: linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH drm-next] drm/hyperv: Replace simple-KMS with regular
 atomic helpers

Hi

Am 20.04.25 um 14:19 schrieb Ryosuke Yasuoka:
> Drop simple-KMS in favor of regular atomic helpers to make the code more
> modular. The simple-KMS helper mix up plane and CRTC state, so it is
> obsolete and should go away [1]. Since it just split the simple-pipe
> funtions into per-plane and per-CRTC, no functional changes is expected.
>
> [1] https://lore.kernel.org/lkml/dae5089d-e214-4518-b927-5c4149babad8@suse.de/
>
> Signed-off-by: Ryosuke Yasuoka <ryasuoka@...hat.com>

I cannot test, but thanks for doing this. Comments below.

> ---
>   drivers/gpu/drm/hyperv/hyperv_drm.h         |   4 +-
>   drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 168 ++++++++++++++++----
>   2 files changed, 139 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm.h b/drivers/gpu/drm/hyperv/hyperv_drm.h
> index d2d8582b36df..9e776112c03e 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm.h
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm.h
> @@ -11,7 +11,9 @@
>   struct hyperv_drm_device {
>   	/* drm */
>   	struct drm_device dev;
> -	struct drm_simple_display_pipe pipe;
> +	struct drm_plane plane;
> +	struct drm_crtc crtc;
> +	struct drm_encoder encoder;
>   	struct drm_connector connector;
>   
>   	/* mode */
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> index 6c6b57298797..c273c093b491 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> @@ -5,6 +5,7 @@
>   
>   #include <linux/hyperv.h>
>   
> +#include <drm/drm_atomic.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_edid.h>
> @@ -15,7 +16,7 @@
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_gem_shmem_helper.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_plane.h>
>   
>   #include "hyperv_drm.h"
>   
> @@ -98,12 +99,47 @@ static int hyperv_check_size(struct hyperv_drm_device *hv, int w, int h,
>   	return 0;
>   }
>   
> -static void hyperv_pipe_enable(struct drm_simple_display_pipe *pipe,
> -			       struct drm_crtc_state *crtc_state,
> -			       struct drm_plane_state *plane_state)
> +static const uint32_t hyperv_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static const uint64_t hyperv_modifiers[] = {
> +	DRM_FORMAT_MOD_LINEAR,
> +	DRM_FORMAT_MOD_INVALID
> +};
> +

> +static enum drm_mode_status
> +hyperv_crtc_helper_mode_valid(struct drm_crtc *crtc,
> +			      const struct drm_display_mode *mode)
> +{
> +	return MODE_OK;
> +}

This helper should not be necessary.

> +
> +static int hyperv_crtc_helper_atomic_check(struct drm_crtc *crtc,
> +					   struct drm_atomic_state *state)
>   {
> -	struct hyperv_drm_device *hv = to_hv(pipe->crtc.dev);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	int ret;
> +
> +	if (!crtc_state->enable)
> +		goto out;
> +
> +	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +out:

> +	return drm_atomic_add_affected_planes(state, crtc);

IIRC this function call was only required to work around an issue in the 
simple-kms helpers. It's not necessary if you use the regular interface 
and should be removed.

> +}
> +
> +static void hyperv_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					     struct drm_atomic_state *state)
> +{
> +	struct hyperv_drm_device *hv = to_hv(crtc->dev);
> +	struct drm_plane *plane = &hv->plane;
> +	struct drm_plane_state *plane_state = plane->state;
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc_state *crtc_state = crtc->state;
>   
>   	hyperv_hide_hw_ptr(hv->hdev);

>   	hyperv_update_situation(hv->hdev, 1,  hv->screen_depth,

This looks more like something to do in the plane's atomic_enable, but 
it's OK for now.

> @@ -113,12 +149,48 @@ static void hyperv_pipe_enable(struct drm_simple_display_pipe *pipe,
>   	hyperv_blit_to_vram_fullscreen(plane_state->fb, &shadow_plane_state->data[0]);

I think this can be removed. Blit'ing is for planes and the plane 
already does it it's atomic_update. Doing this here as well is another 
artifact of the simple-kms helpers.

>   }
>   
> -static int hyperv_pipe_check(struct drm_simple_display_pipe *pipe,
> -			     struct drm_plane_state *plane_state,
> -			     struct drm_crtc_state *crtc_state)
> +static void hyperv_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					      struct drm_atomic_state *state)
> +{ }
> +
> +static const struct drm_crtc_helper_funcs hyperv_crtc_helper_funcs = {
> +	.mode_valid = hyperv_crtc_helper_mode_valid,
> +	.atomic_check = hyperv_crtc_helper_atomic_check,
> +	.atomic_enable = hyperv_crtc_helper_atomic_enable,
> +	.atomic_disable = hyperv_crtc_helper_atomic_disable,
> +};
> +
> +static const struct drm_crtc_funcs hyperv_crtc_funcs = {
> +	.reset = drm_atomic_helper_crtc_reset,
> +	.destroy = drm_crtc_cleanup,
> +	.set_config = drm_atomic_helper_set_config,
> +	.page_flip = drm_atomic_helper_page_flip,
> +	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +};
> +
> +static int hyperv_plane_atomic_check(struct drm_plane *plane,
> +				     struct drm_atomic_state *state)
>   {
> -	struct hyperv_drm_device *hv = to_hv(pipe->crtc.dev);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct hyperv_drm_device *hv = to_hv(plane->dev);
>   	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct drm_crtc_state *crtc_state = NULL;
> +	int ret;
> +
> +	if (crtc)
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, false);
> +	if (ret)
> +		return ret;
> +
> +	if (!plane_state->visible)
> +		return 0;
>   

>   	if (fb->format->format != DRM_FORMAT_XRGB8888)
>   		return -EINVAL;

Our DRM core should have done this test already.

> @@ -132,51 +204,83 @@ static int hyperv_pipe_check(struct drm_simple_display_pipe *pipe,
>   	return 0;
>   }
>   
> -static void hyperv_pipe_update(struct drm_simple_display_pipe *pipe,
> -			       struct drm_plane_state *old_state)
> +static void hyperv_plane_atomic_update(struct drm_plane *plane,
> +						      struct drm_atomic_state *old_state)
>   {
> -	struct hyperv_drm_device *hv = to_hv(pipe->crtc.dev);
> -	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_plane_state *old_pstate = drm_atomic_get_old_plane_state(old_state, plane);
> +	struct hyperv_drm_device *hv = to_hv(plane->dev);
> +	struct drm_plane_state *state = plane->state;
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>   	struct drm_rect rect;
>   
> -	if (drm_atomic_helper_damage_merged(old_state, state, &rect)) {
> +	if (drm_atomic_helper_damage_merged(old_pstate, state, &rect)) {
>   		hyperv_blit_to_vram_rect(state->fb, &shadow_plane_state->data[0], &rect);
>   		hyperv_update_dirt(hv->hdev, &rect);
>   	}
>   }
>   
> -static const struct drm_simple_display_pipe_funcs hyperv_pipe_funcs = {
> -	.enable	= hyperv_pipe_enable,
> -	.check = hyperv_pipe_check,
> -	.update	= hyperv_pipe_update,
> -	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,

> +static bool hyperv_format_mod_supported(struct drm_plane *plane,
> +					uint32_t format, uint64_t modifier)
> +{
> +	return modifier == DRM_FORMAT_MOD_LINEAR;
> +}

This helper is probably not necessary either.

> +
> +static const struct drm_plane_helper_funcs hyperv_plane_helper_funcs = {
> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +	.atomic_check = hyperv_plane_atomic_check,
> +	.atomic_update = hyperv_plane_atomic_update,
>   };
>   
> -static const uint32_t hyperv_formats[] = {
> -	DRM_FORMAT_XRGB8888,
> +static const struct drm_plane_funcs hyperv_plane_funcs = {
> +	.update_plane		= drm_atomic_helper_update_plane,
> +	.disable_plane		= drm_atomic_helper_disable_plane,
> +	.destroy		= drm_plane_cleanup,
> +	.format_mod_supported   = hyperv_format_mod_supported,
> +	DRM_GEM_SHADOW_PLANE_FUNCS,
>   };
>   
> -static const uint64_t hyperv_modifiers[] = {
> -	DRM_FORMAT_MOD_LINEAR,
> -	DRM_FORMAT_MOD_INVALID
> +static const struct drm_encoder_funcs hyperv_drm_simple_encoder_funcs_cleanup = {
> +	.destroy = drm_encoder_cleanup,
>   };
>   
>   static inline int hyperv_pipe_init(struct hyperv_drm_device *hv)
>   {
> +	struct drm_device *dev = &hv->dev;
> +	struct drm_encoder *encoder = &hv->encoder;
> +	struct drm_plane *plane = &hv->plane;
> +	struct drm_crtc *crtc = &hv->crtc;
> +	struct drm_connector *connector = &hv->connector;
>   	int ret;
>   
> -	ret = drm_simple_display_pipe_init(&hv->dev,
> -					   &hv->pipe,
> -					   &hyperv_pipe_funcs,
> -					   hyperv_formats,
> -					   ARRAY_SIZE(hyperv_formats),
> -					   hyperv_modifiers,
> -					   &hv->connector);
> +	drm_plane_helper_add(plane, &hyperv_plane_helper_funcs);

Rather call drm_universal_plane_init() first and then add the helper funcs.

> +	ret = drm_universal_plane_init(dev, plane, 0,
> +				       &hyperv_plane_funcs,
> +				       hyperv_formats, ARRAY_SIZE(hyperv_formats),
> +				       hyperv_modifiers,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret)
> +		return ret;
> +
> +	drm_crtc_helper_add(crtc, &hyperv_crtc_helper_funcs);
> +	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
> +					&hyperv_crtc_funcs, NULL);

Same ordering nit as with the plane.

> +	if (ret)
> +		return ret;
> +
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +	ret = drm_encoder_init(dev, encoder,
> +			       &hyperv_drm_simple_encoder_funcs_cleanup,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +
> +	if (ret || !connector)
> +		return ret;

Connector init happens elsewhere, but you can inline hyperv_conn_init() 
right here.

> +
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +
>   	if (ret)
>   		return ret;
>   
> -	drm_plane_enable_fb_damage_clips(&hv->pipe.plane);
> +	drm_plane_enable_fb_damage_clips(&hv->plane);

This is a plane function you can move it close to the plane init.

Best regards
Thomas

>   
>   	return 0;
>   }
>
> base-commit: b60301774a8fe6c30b14a95104ec099290a2e904

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Powered by blists - more mailing lists