Re: [PATCH 08/24] drm/i915: Prepare to split crtc state in uapi and hw state

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

On Thu, Oct 10, 2019 at 04:21:00PM +0200, Maarten Lankhorst wrote:
> Op 08-10-2019 om 19:06 schreef Ville Syrjälä:
> > On Fri, Oct 04, 2019 at 01:34:58PM +0200, Maarten Lankhorst wrote:
> >> We want to split drm_crtc_state into the user visible state
> >> and actual hardware state. To prepare for this, we need some
> >> ground rules what should be in each state:
> >>
> >> In uapi we use:
> >> - crtc, *_changed flags, event, commit, state, mode_blob,
> >>   (plane/connector/encoder)_mask.
> >>
> >> In hw state we use what's displayed in hardware:
> >> - enable, active, (adjusted) mode, color property blobs.
> >>
> >> clear_intel_crtc_state and hw readout need to be updated for these rules,
> >> which will allow us to enable 2 joined pipes.
> > I still have hard time with reading this patch. I still think it
> > would be easier to read if we didn't do both the "uapi" and "hw" changes
> > at the same time.
> >
> > step 1.
> > 	struct drm_crtc_state uapi;
> > 	struct {
> > 		// hw state
> > 	} base;
> >
> > step 2. 
> > 	s/base/hw/
> >
> > I think that would make it more obvious which parts of the code are
> > looking at which state.
> 
> It wouldn't I think, but here's
> a dumb change with spatch on this patch.
> 
> //+       struct {
> //+               bool active, enable;
> //+               struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> //+               struct drm_display_mode mode, adjusted_mode;
> //+       } hw;
> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.active
> +T->hw.active

This doesn't really help me. There is no .uapi in upstream
code. I would like to see just the .base->.uapi changes
alone first so I can review which parts start to look at
the uapi state to make sure we aren't changing too much.
Then I'd like to to see the .base->.hw changes so that I
convince myself we didn't miss anything in the .base->.uapi
conversion.

And all the remaining drm_crtc_state usage is going to
make us miss something for sure, so getting rid of all that
first would probably help.

> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.enable
> +T->hw.enable
> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.degamma_lut
> +T->hw.degamma_lut
> 
> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.gamma_lut
> +T->hw.gamma_lut
> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.ctm
> +T->hw.ctm
> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.mode
> +T->hw.mode
> 
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.adjusted_mode
> +T->hw.adjusted_mode
> 
> I replaced all the instances where we use the uapi members instead of the hw members explicitly in this patch, and came up with the following diff below.
> 
> Only the intel_color readout is potentially incorrect, the 2 explicit uapi uses in intel_display.c are needed.
> Didn't fix it because of hw readout, it possibly needs slightly more thought.
> 
> Does this satisfy the readability requirements? :)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index ab10c33266bf..cbf4c6e6e661 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11217,7 +11217,7 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  		goto fail;
>  	}
>  
> -	crtc_state->uapi.active = crtc_state->uapi.enable = true;
> +	crtc_state->hw.active = crtc_state->hw.enable = true;
>  
>  	if (!mode)
>  		mode = &load_detect_mode;
> @@ -13578,7 +13578,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  		if (!needs_modeset(new_crtc_state))
>  			continue;
>  
> -		if (!new_crtc_state->uapi.enable) {
> +		if (!new_crtc_state->hw.enable) {
>  			any_ms = true;
>  			continue;
>  		}
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 5586891572f8..52712bb9ed15 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1623,7 +1623,7 @@ static void i9xx_read_luts(struct intel_crtc_state *crtc_state)
>  	if (!crtc_state->gamma_enable)
>  		return;
>  
> -	crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> +	crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
>  }
>  
>  static struct drm_property_blob *
> @@ -1673,9 +1673,9 @@ static void i965_read_luts(struct intel_crtc_state *crtc_state)
>  		return;
>  
>  	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> -		crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> +		crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
>  	else
> -		crtc_state->uapi.gamma_lut = i965_read_lut_10p6(crtc_state);
> +		crtc_state->hw.gamma_lut = i965_read_lut_10p6(crtc_state);
>  }
>  
>  static struct drm_property_blob *
> @@ -1715,7 +1715,7 @@ chv_read_cgm_lut(const struct intel_crtc_state *crtc_state)
>  static void chv_read_luts(struct intel_crtc_state *crtc_state)
>  {
>  	if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
> -		crtc_state->uapi.gamma_lut = chv_read_cgm_lut(crtc_state);
> +		crtc_state->hw.gamma_lut = chv_read_cgm_lut(crtc_state);
>  	else
>  		i965_read_luts(crtc_state);
>  }
> @@ -1762,9 +1762,9 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
>  		return;
>  
>  	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> -		crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> +		crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
>  	else
> -		crtc_state->uapi.gamma_lut = ilk_read_lut_10(crtc_state);
> +		crtc_state->hw.gamma_lut = ilk_read_lut_10(crtc_state);
>  }
>  
>  static struct drm_property_blob *
> @@ -1811,9 +1811,9 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state)
>  		return;
>  
>  	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> -		crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> +		crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
>  	else
> -		crtc_state->uapi.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
> +		crtc_state->hw.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
>  }
>  
>  void intel_color_init(struct intel_crtc *crtc)

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