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 ? > +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? > > -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/ > 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); } 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> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
