Message-ID: <CAHpthZqJPKtXUjFiVRLP+LEmTKFowUKVHGDe9=NS4aGx7WWcMA@mail.gmail.com>
Date: Sat, 26 Apr 2025 21:13:00 +0900
From: Ryosuke Yasuoka <ryasuoka@...hat.com>
To: Javier Martinez Canillas <javierm@...hat.com>
Cc: drawat.floss@...il.com, maarten.lankhorst@...ux.intel.com, 
	mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, 
	jfalempe@...hat.com, linux-hyperv@...r.kernel.org, 
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular
 atomic helpers

Hi Javier,

On Fri, Apr 25, 2025 at 4:15 PM Javier Martinez Canillas
<javierm@...hat.com> wrote:
>
> Ryosuke Yasuoka <ryasuoka@...hat.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@...hat.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@...hat.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
>

Powered by blists - more mailing lists