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

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

On Thu, Jan 12, 2017 at 12:37:46PM -0800, Rodrigo Vivi wrote:
> On Tue, Dec 20, 2016 at 03:29:54PM +0200, Ville Syrjälä wrote:
> > 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).
> 
> Apparently this is the only piece blocking this already reviewed series, right?

Not sure. I'll have to go over it again to see what was said. At least
adding some docs for intel_cdclk.c was requested.

> 
> So, what are the plans? drop this patch and move with the other patches?

I think I pushed the fix to move the min_pixclk[] handling to happen a
little later, so all I should have to do is rebase this patch and it
should all be good.

But as usual I have a few too many irons in the fire, so I haven't
managed to get back to this series yet.

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