Re: [PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

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

On Thu, Nov 09, 2017 at 06:56:36PM +0100, Maarten Lankhorst wrote:
> Op 09-11-17 om 18:51 schreef Ville Syrjälä:
> > On Thu, Nov 09, 2017 at 06:31:02PM +0100, Maarten Lankhorst wrote:
> >> Op 09-11-17 om 18:09 schreef Ville Syrjälä:
> >>> On Thu, Nov 09, 2017 at 05:24:55PM +0100, Maarten Lankhorst wrote:
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
> >>>>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
> >>>>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
> >>>>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
> >>>>  4 files changed, 20 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >>>> index b8315bca852b..c6373febf617 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_color.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_color.c
> >>>> @@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
> >>>>  	 */
> >>>>  	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
> >>>>  	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> >>>> -		hsw_disable_ips(intel_crtc);
> >>>> +		hsw_disable_ips(intel_crtc, intel_crtc_state);
> >>> I would suggest simplifying more and passing only the state.
> >>> The callee can always dig out the crtc if it needs it.
> >>>
> >>>>  		reenable_ips = true;
> >>>>  	}
> >>>>  
> >>>> @@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
> >>>>  	i9xx_load_luts(crtc_state);
> >>>>  
> >>>>  	if (reenable_ips)
> >>>> -		hsw_enable_ips(intel_crtc);
> >>>> +		hsw_enable_ips(intel_crtc, intel_crtc_state);
> >>>>  }
> >>>>  
> >>>>  static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index f5933b0719c9..13b372e4f06e 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -void hsw_enable_ips(struct intel_crtc *crtc)
> >>>> +void hsw_enable_ips(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->base.dev;
> >>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>>  
> >>>> -	if (!crtc->config->ips_enabled)
> >>>> +	if (!crtc_state->ips_enabled)
> >>>>  		return;
> >>>>  
> >>>>  	/*
> >>>> @@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -void hsw_disable_ips(struct intel_crtc *crtc)
> >>>> +void hsw_disable_ips(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->base.dev;
> >>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>>  
> >>>> -	if (!crtc->config->ips_enabled)
> >>>> +	if (!crtc_state->ips_enabled)
> >>>>  		return;
> >>>>  
> >>>>  	assert_plane_enabled(dev_priv, crtc->plane);
> >>> Hmm. Confusied. Oh right, my plane assert cleanup didn't manage to go in yet :P
> >>>
> >>>> @@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
> >>>>   * completely hide the primary plane.
> >>>>   */
> >>>>  static void
> >>>> -intel_post_enable_primary(struct drm_crtc *crtc)
> >>>> +intel_post_enable_primary(struct drm_crtc *crtc,
> >>>> +			  const struct intel_crtc_state *new_crtc_state)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->dev;
> >>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>> @@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>>>  	 * when going from primary only to sprite only and vice
> >>>>  	 * versa.
> >>>>  	 */
> >>>> -	hsw_enable_ips(intel_crtc);
> >>>> +	hsw_enable_ips(intel_crtc, new_crtc_state);
> >>>>  
> >>>>  	/*
> >>>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> >>>> @@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>>>  
> >>>>  /* FIXME move all this to pre_plane_update() with proper state tracking */
> >>>>  static void
> >>>> -intel_pre_disable_primary(struct drm_crtc *crtc)
> >>>> +intel_pre_disable_primary(struct drm_crtc *crtc,
> >>>> +			  const struct intel_crtc_state *old_crtc_state)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->dev;
> >>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>> @@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >>>>  	 * when going from primary only to sprite only and vice
> >>>>  	 * versa.
> >>>>  	 */
> >>>> -	hsw_disable_ips(intel_crtc);
> >>>> +	hsw_disable_ips(intel_crtc, old_crtc_state);
> >>>>  }
> >>>>  
> >>>>  /* FIXME get rid of this and use pre_plane_update */
> >>>> @@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> >>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>>  	int pipe = intel_crtc->pipe;
> >>>>  
> >>>> -	intel_pre_disable_primary(crtc);
> >>>> +	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> >>>>  
> >>>>  	/*
> >>>>  	 * Vblank time updates from the shadow to live plane control register
> >>>> @@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >>>>  		if (primary_state->base.visible &&
> >>>>  		    (needs_modeset(&pipe_config->base) ||
> >>>>  		     !old_primary_state->base.visible))
> >>>> -			intel_post_enable_primary(&crtc->base);
> >>>> +			intel_post_enable_primary(&crtc->base, pipe_config);
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> @@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >>>>  
> >>>>  		if (old_primary_state->base.visible &&
> >>>>  		    (modeset || !primary_state->base.visible))
> >>>> -			intel_pre_disable_primary(&crtc->base);
> >>>> +			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> >>>>  	}
> >>>>  
> >>>>  	/*
> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>> index cddd96b24878..82c33d9cd466 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>> @@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>>>  	}
> >>>>  
> >>>>   out:
> >>>> -	hsw_enable_ips(intel_crtc);
> >>>> +	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
> >>> crtc->config would seem like the slightly more correct thing to pass
> >>> here. It's also easier to grep for when someone sets out to fix this
> >>> mess properly.
> >> Considered it, but our crtc->config usage is a mess. I'd rather get rid of the intel_dp_sink_crc code tbh. :/
> > I'd rather not spread more ->state uses when I just recently
> > managed to clean out many of them. We still have some left for
> > sure, but adding more seems like going backwards.
> >
> Maybe export hsw_pipe_A_crc_wa ?

Yeah, I suppose that would the proper fix. I didn't actually bother
thinking about it that far when I was last looking at this. I guess the
only downside is that we'd also get the panel fitter thing which we
don't really need here. But not sure anyone would actually notice.

Looks like the locking would have to be reworked though.
i915_sink_crc() still uses drm_modeset_lock_all().

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