Re: [PATCH 01/14] drm/i915: Store the pipe pixel rate in the crtc state

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

On Tue, Dec 20, 2016 at 03:10:53PM +0200, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Rather than recomptuing the pipe pixel rate on demand everwhere, let's
> > just stick the precomputed value into the crtc state.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  drivers/gpu/drm/i915/intel_fbc.c     |  3 +--
> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++++-------
> >  4 files changed, 36 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0b0d7e8be630..1d979041c52c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > 
> 
> [...]
> 
> > @@ -16919,7 +16938,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>                 __drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
>                 memset(crtc_state, 0, sizeof(*crtc_state));
>                 crtc_state->base.crtc = &crtc->base;
>  
>                 crtc_state->base.active = crtc_state->base.enable =
>                         dev_priv->display.get_pipe_config(crtc, crtc_state);
>  
>                 crtc->base.enabled = crtc_state->base.enable;
>                 crtc->active = crtc_state->base.active;
>  
>                 if (crtc_state->base.active) {
> >  			dev_priv->active_crtcs |= 1 << crtc->pipe;
> >  
> >  			if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -				pixclk = ilk_pipe_pixel_rate(crtc_state);
> > +				pixclk = crtc_state->pixel_rate;
> 
> Aren't you reading 0 here, because of the memset above? As far as I can tell,
> haswell_get_pipe_config() doesn't set crtc_state->pixel_rate.

Hmm, yeah. Which means this whole piece of min_pixclk[] code is in
the wrong place. You can't know the pixel rate until you know the
clock, and you don't know that until you've done the full readout
(meaning the encoder .get_config() hooks have been called as well).

> 
> >  			else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> >  			else
> > @@ -17031,6 +17050,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  			 */
> >  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
> >  
> > +			intel_crtc_compute_pixel_rate(crtc->config);
> > +
> >  			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
> >  			update_scanline_offset(crtc);
> >  		}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f61ea43c7532..3969e786d566 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -541,6 +541,8 @@ struct intel_crtc_state {
> >  	 * and get clipped at the edges. */
> >  	int pipe_src_w, pipe_src_h;
> >  
> > +	unsigned int pixel_rate;
> > +
> 
> Maybe add some comment about this parameter. This is not in kernel doc, but
> having that already would probably make it easier for whoever does it in the
> end.
> 
> Ander

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