Re: [CI 12/12] drm/i915: Complete plane hw and uapi split, v2.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 29, 2019 at 08:22:29AM +0100, Maarten Lankhorst wrote:
> Splitting plane state is easier than splitting crtc_state,
> before plane check we copy the drm properties to hw so we can
> do the same in bigjoiner later on.
> 
> We copy the state after we did all the modeset handling, but fortunately
> i915 seems to be split correctly and nothing during modeset looks
> at plane_state.
> 
> Changes since v1:
> - Do not clear hw state on duplication.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++-
>  .../gpu/drm/i915/display/intel_atomic_plane.h |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>  .../drm/i915/display/intel_display_types.h    | 23 +++++++++---
>  4 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index ba7d5421f791..75a5004b234e 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -93,6 +93,10 @@ intel_plane_duplicate_state(struct drm_plane *plane)
>  	intel_state->vma = NULL;
>  	intel_state->flags = 0;
>  
> +	/* add reference to fb */
> +	if (intel_state->hw.fb)
> +		drm_framebuffer_get(intel_state->hw.fb);
> +
>  	return &intel_state->uapi;
>  }
>  
> @@ -112,6 +116,8 @@ intel_plane_destroy_state(struct drm_plane *plane,
>  	WARN_ON(plane_state->vma);
>  
>  	__drm_atomic_helper_plane_destroy_state(&plane_state->uapi);
> +	if (plane_state->hw.fb)
> +		drm_framebuffer_put(plane_state->hw.fb);
>  	kfree(plane_state);
>  }
>  
> @@ -176,15 +182,44 @@ bool intel_plane_calc_min_cdclk(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> +static void intel_plane_clear_hw_state(struct intel_plane_state *plane_state)
> +{
> +	if (plane_state->hw.fb)
> +		drm_framebuffer_put(plane_state->hw.fb);
> +
> +	memset(&plane_state->hw, 0, sizeof(plane_state->hw));
> +}
> +
> +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> +				       const struct intel_plane_state *from_plane_state)
> +{
> +	intel_plane_clear_hw_state(plane_state);
> +
> +	plane_state->hw.crtc = from_plane_state->uapi.crtc;
> +	plane_state->hw.fb = from_plane_state->uapi.fb;
> +	if (plane_state->hw.fb)
> +		drm_framebuffer_get(plane_state->hw.fb);
> +
> +	plane_state->hw.alpha = from_plane_state->uapi.alpha;
> +	plane_state->hw.pixel_blend_mode =
> +		from_plane_state->uapi.pixel_blend_mode;
> +	plane_state->hw.rotation = from_plane_state->uapi.rotation;
> +	plane_state->hw.color_encoding = from_plane_state->uapi.color_encoding;
> +	plane_state->hw.color_range = from_plane_state->uapi.color_range;
> +}
> +
>  int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
>  					struct intel_crtc_state *new_crtc_state,
>  					const struct intel_plane_state *old_plane_state,
>  					struct intel_plane_state *new_plane_state)
>  {
>  	struct intel_plane *plane = to_intel_plane(new_plane_state->uapi.plane);
> -	const struct drm_framebuffer *fb = new_plane_state->hw.fb;
> +	const struct drm_framebuffer *fb;
>  	int ret;
>  
> +	intel_plane_copy_uapi_to_hw_state(new_plane_state, new_plane_state);
> +	fb = new_plane_state->hw.fb;
> +
>  	new_crtc_state->active_planes &= ~BIT(plane->id);
>  	new_crtc_state->nv12_planes &= ~BIT(plane->id);
>  	new_crtc_state->c8_planes &= ~BIT(plane->id);
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index e61e9a82aadf..cdb0f97d09f9 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -20,6 +20,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
>  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state);
> +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> +				       const struct intel_plane_state *from_plane_state);
>  void intel_update_plane(struct intel_plane *plane,
>  			const struct intel_crtc_state *crtc_state,
>  			const struct intel_plane_state *plane_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 91a059e5fdcb..ed9504d0336e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3283,6 +3283,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  
>  	plane_state->fb = fb;
>  	plane_state->crtc = &intel_crtc->base;
> +	intel_plane_copy_uapi_to_hw_state(intel_state, intel_state);
>  
>  	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
>  		  &to_intel_frontbuffer(fb)->bits);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e43d5a09550d..344b40687a30 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -523,11 +523,24 @@ struct intel_atomic_state {
>  };
>  
>  struct intel_plane_state {
> -	union {
> -		struct drm_plane_state base;
> -		struct drm_plane_state uapi;
> -		struct drm_plane_state hw;
> -	};
> +	struct drm_plane_state uapi;
> +
> +	/*
> +	 * actual hardware state, the state we program to the hardware.
> +	 * The following members are used to verify the hardware state:
> +	 * During initial hw readout, they need to be copied from uapi.
> +	 */
> +	struct {
> +		struct drm_crtc *crtc;
> +		struct drm_framebuffer *fb;
> +
> +		u16 alpha;
> +		uint16_t pixel_blend_mode;

u16, though this will easily fit into u8.

> +		unsigned int rotation;

This too can fit into u8.

Would be nice to change those types in the core struct too I guess.

Otherwise lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		enum drm_color_encoding color_encoding;
> +		enum drm_color_range color_range;
> +	} hw;
> +
>  	struct i915_ggtt_view view;
>  	struct i915_vma *vma;
>  	unsigned long flags;
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx