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

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

On Fri, Oct 18, 2019 at 02:09:17PM +0200, Maarten Lankhorst wrote:
> Op 18-10-2019 om 12:36 schreef Ville Syrjälä:
> > 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;
> >
> > ?
> 
> This is basically union { struct drm_crtc_state uapi,base,hw; }; Making all 3 aliases until patch 8/14, which kills off base and splits off hw state.
> 
> We keep it as an union because base and uapi temporarily have to be kept in sync, while we split to uapi and hw. So it's easy to keep hw in sync as well.
> 
> In the end it doesn't matter whether we split off hw before or after. The diff between before and after is the same.

Sure but the individual patches are very hard to review. Eg. I can see
a bunch of stuff getting converted from base to hw, but since .base
still exists I have no way of knowing whether the patch really changed
all of them or just some of them.

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