[PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular atomic helpers

[PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular atomic helpers

[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
functions 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@redhat.com>

---
v2:
- Remove hyperv_crtc_helper_mode_valid
- Remove hyperv_format_mod_supported
- Call helper_add after {plane,crtc}_init
- Move drm_plane_enable_fb_damage_clips to a place close to plane init
- Move hyperv_conn_init() into hyperv_pipe_init
- Remove hyperv_blit_to_vram_fullscreen
- Remove format check
- Replace hyperv_crtc_helper_atomic_check to drm_crtc_helper_atomic_check

v1:
https://lore.kernel.org/all/20250420121945.573915-1-ryasuoka@redhat.com/

 drivers/gpu/drm/hyperv/hyperv_drm.h         |   4 +-
 drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 159 +++++++++++++-------
 2 files changed, 107 insertions(+), 56 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..374f8464f5bc 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
@@ -5,6 +5,8 @@
 
 #include <linux/hyperv.h>
 
+#include <drm/drm_atomic.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
@@ -15,7 +17,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"
 
@@ -38,18 +40,6 @@ static int hyperv_blit_to_vram_rect(struct drm_framebuffer *fb,
 	return 0;
 }
 
-static int hyperv_blit_to_vram_fullscreen(struct drm_framebuffer *fb,
-					  const struct iosys_map *map)
-{
-	struct drm_rect fullscreen = {
-		.x1 = 0,
-		.x2 = fb->width,
-		.y1 = 0,
-		.y2 = fb->height,
-	};
-	return hyperv_blit_to_vram_rect(fb, map, &fullscreen);
-}
-
 static int hyperv_connector_get_modes(struct drm_connector *connector)
 {
 	struct hyperv_drm_device *hv = to_hv(connector->dev);
@@ -98,30 +88,71 @@ 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 void hyperv_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+					     struct drm_atomic_state *state)
 {
-	struct hyperv_drm_device *hv = to_hv(pipe->crtc.dev);
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_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_crtc_state *crtc_state = crtc->state;
 
 	hyperv_hide_hw_ptr(hv->hdev);
 	hyperv_update_situation(hv->hdev, 1,  hv->screen_depth,
 				crtc_state->mode.hdisplay,
 				crtc_state->mode.vdisplay,
 				plane_state->fb->pitches[0]);
-	hyperv_blit_to_vram_fullscreen(plane_state->fb, &shadow_plane_state->data[0]);
 }
 
-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 = {
+	.atomic_check = drm_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 (fb->format->format != DRM_FORMAT_XRGB8888)
-		return -EINVAL;
+	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->pitches[0] * fb->height > hv->fb_size) {
 		drm_err(&hv->dev, "fb size requested by %s for %dX%d (pitch %d) greater than %ld\n",
@@ -132,53 +163,77 @@ 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 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,
+	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);
+	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_plane_helper_add(plane, &hyperv_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(plane);
+
+	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
+					&hyperv_crtc_funcs, NULL);
 	if (ret)
 		return ret;
+	drm_crtc_helper_add(crtc, &hyperv_crtc_helper_funcs);
 
-	drm_plane_enable_fb_damage_clips(&hv->pipe.plane);
+	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)
+		return ret;
 
-	return 0;
+	ret = hyperv_conn_init(hv);
+	if (ret) {
+		drm_err(dev, "Failed to initialized connector.\n");
+		return ret;
+	}
+
+	return drm_connector_attach_encoder(connector, encoder);
 }
 
 static enum drm_mode_status
@@ -221,12 +276,6 @@ int hyperv_mode_config_init(struct hyperv_drm_device *hv)
 
 	dev->mode_config.funcs = &hyperv_mode_config_funcs;
 
-	ret = hyperv_conn_init(hv);
-	if (ret) {
-		drm_err(dev, "Failed to initialized connector.\n");
-		return ret;
-	}
-
 	ret = hyperv_pipe_init(hv);
 	if (ret) {
 		drm_err(dev, "Failed to initialized pipe.\n");

base-commit: b60301774a8fe6c30b14a95104ec099290a2e904
-- 
2.49.0

Re: [PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular atomic helpers

[Javier Martinez Canillas]

Ryosuke Yasuoka <ryasuoka@redhat.com> writes:

Hello Ryosuke,

> 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
> functions 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@redhat.com>
>



> -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
> +};
> +

I think the kernel u32 and u64 types are preferred ?

> +static void hyperv_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					     struct drm_atomic_state *state)
>  {
> -	struct hyperv_drm_device *hv = to_hv(pipe->crtc.dev);
> -	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_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_crtc_state *crtc_state = crtc->state;
>  
>  	hyperv_hide_hw_ptr(hv->hdev);
>  	hyperv_update_situation(hv->hdev, 1,  hv->screen_depth,
>  				crtc_state->mode.hdisplay,
>  				crtc_state->mode.vdisplay,
>  				plane_state->fb->pitches[0]);
> -	hyperv_blit_to_vram_fullscreen(plane_state->fb, &shadow_plane_state->data[0]);
>  }
>  
> -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)
> +{ }
> +

Why do you need an empty CRTC atomic disable callback? Can you just not
set it instead?

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

You should never access the plane->state directly, instead the helper
drm_atomic_get_new_plane_state() should be used. You can also rename
the old_state paramete to just state, since it will be used to lookup
both the old and new atomic states.

More info is in the following email from Ville:

https://lore.kernel.org/dri-devel/Yx9pij4LmFHrq81V@intel.com/

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

I know that most of the simple-KMS drivers do this but since this driver
enables FB damage clips support, it is better to iterate over the damage 
areas. For example:

	struct drm_atomic_helper_damage_iter iter;
        struct drm_rect dst_clip;
	struct drm_rect damage;

	drm_atomic_helper_damage_iter_init(&iter, old_pstate, state);
	drm_atomic_for_each_plane_damage(&iter, &damage) {
		dst_clip = state->dst;

		if (!drm_rect_intersect(&dst_clip, &damage))
			continue;

                hyperv_blit_to_vram_rect(state->fb, &shadow_plane_state->data[0], &damage);
                hyperv_update_dirt(hv->hdev, &damage);
        }


Other than these small comments, the patch looks good to me. So if you take
into account my suggestions, feel free to add:

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular atomic helpers

[Ryosuke Yasuoka]

Hi Javier,

On Fri, Apr 25, 2025 at 4:15 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Ryosuke Yasuoka <ryasuoka@redhat.com> writes:
>
> Hello Ryosuke,
>
> > 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
> > functions 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@redhat.com>
> >
>
>
>
> > -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
> > +};
> > +
>
> I think the kernel u32 and u64 types are preferred ?

I'm not sure if I should fix this in this patch because I did not add these
variables. IMO, we need to split the commit if we fix them.

> > +static void hyperv_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> > +                                          struct drm_atomic_state *state)
> >  {
> > -     struct hyperv_drm_device *hv = to_hv(pipe->crtc.dev);
> > -     struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_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_crtc_state *crtc_state = crtc->state;
> >
> >       hyperv_hide_hw_ptr(hv->hdev);
> >       hyperv_update_situation(hv->hdev, 1,  hv->screen_depth,
> >                               crtc_state->mode.hdisplay,
> >                               crtc_state->mode.vdisplay,
> >                               plane_state->fb->pitches[0]);
> > -     hyperv_blit_to_vram_fullscreen(plane_state->fb, &shadow_plane_state->data[0]);
> >  }
> >
> > -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)
> > +{ }
> > +
>
> Why do you need an empty CRTC atomic disable callback? Can you just not
> set it instead?

OK. I'll fix it in my next patch.

> >
> > -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;
>
> You should never access the plane->state directly, instead the helper
> drm_atomic_get_new_plane_state() should be used. You can also rename
> the old_state paramete to just state, since it will be used to lookup
> both the old and new atomic states.
>
> More info is in the following email from Ville:
>
> https://lore.kernel.org/dri-devel/Yx9pij4LmFHrq81V@intel.com/

OK. I'll fix it in my next patch. Thank you for sharing the url.

> >       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)) {
>
> I know that most of the simple-KMS drivers do this but since this driver
> enables FB damage clips support, it is better to iterate over the damage
> areas. For example:
>
>         struct drm_atomic_helper_damage_iter iter;
>         struct drm_rect dst_clip;
>         struct drm_rect damage;
>
>         drm_atomic_helper_damage_iter_init(&iter, old_pstate, state);
>         drm_atomic_for_each_plane_damage(&iter, &damage) {
>                 dst_clip = state->dst;
>
>                 if (!drm_rect_intersect(&dst_clip, &damage))
>                         continue;
>
>                 hyperv_blit_to_vram_rect(state->fb, &shadow_plane_state->data[0], &damage);
>                 hyperv_update_dirt(hv->hdev, &damage);
>         }
>

OK. As you said, other drivers like mgag200 implement like this. I'll
fix them in my next patch.

> Other than these small comments, the patch looks good to me. So if you take
> into account my suggestions, feel free to add:
>
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>

Thank you for your review and comment. I'll fix them and add your ack.

Best regards,
Ryosuke

> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>

Re: [PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular atomic helpers

[Javier Martinez Canillas]

Ryosuke Yasuoka <ryasuoka@redhat.com> writes:

Hello Ryosuke,

> Hi Javier,
>
> On Fri, Apr 25, 2025 at 4:15 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>>
>> Ryosuke Yasuoka <ryasuoka@redhat.com> writes:
>>
>> Hello Ryosuke,
>>
>> > 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
>> > functions 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@redhat.com>
>> >
>>
>>
>>
>> > -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
>> > +};
>> > +
>>
>> I think the kernel u32 and u64 types are preferred ?
>
> I'm not sure if I should fix this in this patch because I did not add these
> variables. IMO, we need to split the commit if we fix them.
>

Right, I got confused for how the diff showed the changes. But I agree with
you that should be a separate patch since the variables already exist.

[...]

>>
>> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>
> Thank you for your review and comment. I'll fix them and add your ack.
>

Thanks!

> Best regards,
> Ryosuke
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat