Re: [PATCH 6/9] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 04.07.26 um 20:31 schrieb Ze Huang:
> Convert gm12u320 to direct primary plane, CRTC and encoder setup.
>
> Keep shadow-plane helper state, framebuffer access helpers and
> no-scaling plane-state check from simple-KMS path.
>
> Signed-off-by: Ze Huang <ze.huang@xxxxxxxxxxxxxxxx>
Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

And I also have this device. Works nicely with the patch

Tested-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

> ---
>   drivers/gpu/drm/tiny/gm12u320.c | 128 ++++++++++++++++++++++++++++++++--------
>   1 file changed, 104 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index d73dfebb4353..992160ea794d 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -8,6 +8,7 @@
>   #include <linux/usb.h>
>   
>   #include <drm/clients/drm_client_setup.h>
> +#include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_atomic_state_helper.h>
>   #include <drm/drm_connector.h>
> @@ -27,7 +28,6 @@
>   #include <drm/drm_modeset_helper_vtables.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
>   
>   static bool eco_mode;
>   module_param(eco_mode, bool, 0644);
> @@ -87,7 +87,9 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more 
> silent)");
>   
>   struct gm12u320_device {
>         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             conn;
>         unsigned char                   *cmd_buf;
>         unsigned char                   *data_buf[GM12U320_BLOCK_COUNT];
> @@ -554,31 +556,33 @@ static int gm12u320_conn_init(struct gm12u320_device 
> *gm12u320)
>   }
>   
>   /* ------------------------------------------------------------------ */
> -/* gm12u320 (simple) display pipe                                    */
> +/* gm12u320 display pipe                                                     */
>   
> -static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe,
> -                                struct drm_crtc_state *crtc_state,
> -                                struct drm_plane_state *plane_state)
> +static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +                                              struct drm_atomic_commit *commit)
>   {
>         struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
> -       struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> +       struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
> +       struct drm_plane_state *plane_state = crtc->primary->state;
>         struct drm_shadow_plane_state *shadow_plane_state = 
> to_drm_shadow_plane_state(plane_state);
>   
>   	gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
>         gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0], 
> &rect);
>   }
>   
> -static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void gm12u320_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +                                               struct drm_atomic_commit 
> *commit)
>   {
> -       struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> +       struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
>   
>   	gm12u320_stop_fb_update(gm12u320);
>   }
>   
> -static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
> -                                struct drm_plane_state *old_state)
> +static void gm12u320_plane_helper_atomic_update(struct drm_plane *plane,
> +                                               struct drm_atomic_commit 
> *commit)
>   {
> -       struct drm_plane_state *state = pipe->plane.state;
> +       struct drm_plane_state *old_state = 
> drm_atomic_get_old_plane_state(commit, plane);
> +       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;
>   
> @@ -586,11 +590,71 @@ static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
>                 gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], 
> &rect);
>   }
>   
> -static const struct drm_simple_display_pipe_funcs gm12u320_pipe_funcs = {
> -       .enable     = gm12u320_pipe_enable,
> -       .disable    = gm12u320_pipe_disable,
> -       .update     = gm12u320_pipe_update,
> -       DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
> +static const struct drm_plane_funcs gm12u320_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 int gm12u320_plane_helper_atomic_check(struct drm_plane *plane,
> +                                             struct drm_atomic_commit *state)
> +{
> +       struct drm_plane_state *plane_state = 
> drm_atomic_get_new_plane_state(state, plane);
> +       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);
> +       return ret;
Interestingly, this patch handled _check_plane_state() correctly.

Best regards
Thomas

> +}
> +
> +static const struct drm_plane_helper_funcs gm12u320_plane_helper_funcs = {
> +       DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +       .atomic_check   = gm12u320_plane_helper_atomic_check,
> +       .atomic_update  = gm12u320_plane_helper_atomic_update,
> +};
> +
> +static int gm12u320_crtc_helper_atomic_check(struct drm_crtc *crtc,
> +                                            struct drm_atomic_commit *state)
> +{
> +       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 const struct drm_crtc_helper_funcs gm12u320_crtc_helper_funcs = {
> +       .atomic_check   = gm12u320_crtc_helper_atomic_check,
> +       .atomic_enable  = gm12u320_crtc_helper_atomic_enable,
> +       .atomic_disable = gm12u320_crtc_helper_atomic_disable,
> +};
> +
> +static const struct drm_crtc_funcs gm12u320_crtc_funcs = {
> +       .set_config             = drm_atomic_helper_set_config,
> +       .page_flip              = drm_atomic_helper_page_flip,
> +       .reset                  = drm_atomic_helper_crtc_reset,
> +       .destroy                = drm_crtc_cleanup,
> +       .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> +       .atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
> +};
> +
> +static const struct drm_encoder_funcs gm12u320_encoder_funcs = {
> +       .destroy = drm_encoder_cleanup,
>   };
>   
>   static const uint32_t gm12u320_pipe_formats[] = {
> @@ -677,13 +741,29 @@ static int gm12u320_usb_probe(struct usb_interface 
> *interface,
>         if (ret)
>                 return ret;
>   
> -	ret = drm_simple_display_pipe_init(&gm12u320->dev,
> -                                          &gm12u320->pipe,
> -                                          &gm12u320_pipe_funcs,
> -                                          gm12u320_pipe_formats,
> -                                          ARRAY_SIZE(gm12u320_pipe_formats),
> -                                          gm12u320_pipe_modifiers,
> -                                          &gm12u320->conn);
> +       ret = drm_universal_plane_init(dev, &gm12u320->plane, 0,
> +                                      &gm12u320_plane_funcs,
> +                                      gm12u320_pipe_formats,
> +                                      ARRAY_SIZE(gm12u320_pipe_formats),
> +                                      gm12u320_pipe_modifiers,
> +                                      DRM_PLANE_TYPE_PRIMARY, NULL);
> +       if (ret)
> +               return ret;
> +       drm_plane_helper_add(&gm12u320->plane, &gm12u320_plane_helper_funcs);
> +
> +       ret = drm_crtc_init_with_planes(dev, &gm12u320->crtc, &gm12u320->plane, 
> NULL,
> +                                       &gm12u320_crtc_funcs, NULL);
> +       if (ret)
> +               return ret;
> +       drm_crtc_helper_add(&gm12u320->crtc, &gm12u320_crtc_helper_funcs);
> +
> +       ret = drm_encoder_init(dev, &gm12u320->encoder, &gm12u320_encoder_funcs,
> +                              DRM_MODE_ENCODER_NONE, NULL);
> +       if (ret)
> +               return ret;
> +       gm12u320->encoder.possible_crtcs = drm_crtc_mask(&gm12u320->crtc);
> +
> +       ret = drm_connector_attach_encoder(&gm12u320->conn, &gm12u320->encoder);
>         if (ret)
>                 return ret;
>   
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)