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

Am 10.07.26 um 11:09 schrieb Thomas Zimmermann:
> 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 <[email protected]>
>
> Reviewed-by: Thomas Zimmermann <[email protected]>
>
> And I also have this device. Works nicely with the patch
>
> Tested-by: Thomas Zimmermann <[email protected]>

Well, see my reply to the Sashiko bot for possible problems.

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