Re: [PATCH 04/14] drm/i915: Add aliases for uapi and hw to crtc_state

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

On Thu, Oct 17, 2019 at 03:20:55PM +0200, Maarten Lankhorst wrote:
> Prepare to split up hw and uapi machinally, by adding a uapi and
> hw alias. We will remove the base in a bit. This is a split from the
> original uapi/hw patch, which did it all in one go.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   |  8 ++++--
>  drivers/gpu/drm/i915/display/intel_display.c  |  2 ++
>  drivers/gpu/drm/i915/display/intel_display.h  |  6 ++---
>  .../drm/i915/display/intel_display_types.h    | 27 ++++++++++++++++++-
>  4 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index e6cb85d41c8d..2cdc92897abd 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -186,9 +186,10 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
>  struct drm_crtc_state *
>  intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  {
> +	const struct intel_crtc_state *old_crtc_state = to_intel_crtc_state(crtc->state);
>  	struct intel_crtc_state *crtc_state;
>  
> -	crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
> +	crtc_state = kmemdup(old_crtc_state, sizeof(*crtc_state), GFP_KERNEL);
>  	if (!crtc_state)
>  		return NULL;
>  
> @@ -219,7 +220,10 @@ void
>  intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			 struct drm_crtc_state *state)
>  {
> -	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +	struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
> +
> +	__drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
> +	kfree(crtc_state);
>  }
>  
>  static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 5632e13d458d..fa0abfdff2ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12282,6 +12282,8 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  
>  	/* Keep base drm_crtc_state intact, only clear our extended struct */
>  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> +	BUILD_BUG_ON(offsetof(struct intel_crtc_state, uapi));
> +	BUILD_BUG_ON(offsetof(struct intel_crtc_state, hw));
>  	memcpy(&crtc_state->base + 1, &saved_state->base + 1,
>  	       sizeof(*crtc_state) - sizeof(crtc_state->base));
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 90807603987c..9b53f65386b5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -444,10 +444,10 @@ enum phy_fia {
>  #define intel_atomic_crtc_state_for_each_plane_state( \
>  		  plane, plane_state, \
>  		  crtc_state) \
> -	for_each_intel_plane_mask(((crtc_state)->base.state->dev), (plane), \
> -				((crtc_state)->base.plane_mask)) \
> +	for_each_intel_plane_mask(((crtc_state)->uapi.state->dev), (plane), \
> +				((crtc_state)->uapi.plane_mask)) \
>  		for_each_if ((plane_state = \
> -			      to_intel_plane_state(__drm_atomic_get_current_plane_state((crtc_state)->base.state, &plane->base))))
> +			      to_intel_plane_state(__drm_atomic_get_current_plane_state((crtc_state)->uapi.state, &plane->base))))
>  
>  void intel_link_compute_m_n(u16 bpp, int nlanes,
>  			    int pixel_clock, int link_clock,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 244e881474fb..4d85ea5832d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -749,7 +749,32 @@ enum intel_output_format {
>  };
>  
>  struct intel_crtc_state {
> +	union {
>  	struct drm_crtc_state base;
> +	/*
> +	 * uapi (drm) state. This is the software state shown to userspace.
> +	 * In particular, the following members are used for bookkeeping:
> +	 * - crtc
> +	 * - state
> +	 * - *_changed
> +	 * - event
> +	 * - commit
> +	 * - mode_blob
> +	 */
> +	struct drm_crtc_state uapi;
> +
> +	/*
> +	 * actual hardware state, the state we program to the hardware.
> +	 * The following members are used to verify the hardware state:
> +	 * - enable
> +	 * - active
> +	 * - mode / adjusted_mode
> +	 * - color property blobs.
> +	 *
> +	 * During initial hw readout, they need to be copied to uapi.
> +	 */
> +	struct drm_crtc_state hw;
> +	};

This part confuses me. Can't we just do

- struct drm_crtc_state base;
+ struct drm_crtc_state uapi;
+ struct {
+	...
+ } base;

?

Or if we somehow need 'base' to remain a drm_crtc_state for a bit then
just start with

+ struct drm_crtc_state uapi;
  struct drm_crtc_state base;

?


>  
>  	/**
>  	 * quirks - bitfield with hw state readout quirks
> @@ -1094,7 +1119,7 @@ struct cxsr_latency {
>  
>  #define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> -#define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, base)
> +#define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, uapi)
>  #define to_intel_connector(x) container_of(x, struct intel_connector, base)
>  #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> -- 
> 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