Hi Javier,
On Fri, Apr 25, 2025 at 4:15 PM Javier Martinez Canillas
<javi...@redhat.com> wrote:
>
> Ryosuke Yasuoka <ryasu...@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-5c4149bab...@suse.de/
> >
> > Signed-off-by: Ryosuke Yasuoka <ryasu...@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/yx9pij4lmfhrq...@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 <javi...@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
>
