drivers/gpu/drm/hyperv/hyperv_drm.h | 4 +- drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 168 ++++++++++++++++---- 2 files changed, 139 insertions(+), 33 deletions(-)
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@redhat.com>
---
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;
+}
+
+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);
+}
+
+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,
@@ -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]);
}
-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;
@@ -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;
+}
+
+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);
+ 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);
+ 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;
+
+ 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);
return 0;
}
base-commit: b60301774a8fe6c30b14a95104ec099290a2e904
--
2.49.0Hi
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@redhat.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)
© 2016 - 2026 Red Hat, Inc.