Re: [PATCH drm-next v2] drm/hyperv: Replace simple-KMS with regular atomic helpers

[Javier Martinez Canillas <javierm@redhat.com>]

Ryosuke Yasuoka <ryasuoka@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-5c4149babad8@suse.de/
>
> Signed-off-by: Ryosuke Yasuoka <ryasuoka@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/Yx9pij4LmFHrq81V@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 <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat