Re: [PATCH 3/5] drm/i915: Check pixel clock limits on pre-gen4

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 02, 2013 at 10:24:18PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 02, 2013 at 08:51:41PM +0200, Daniel Vetter wrote:
> > On Mon, Sep 02, 2013 at 09:24:26PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We don't want to try to push the hardware beyond it's capabilities,
> > > so check the pixel clock against the display core clock limit. Do
> > > it for pre-gen4 for now since that's where we alread have the double
> > > wide pixel clock limit check.
> > > 
> > > Let's assume that when double wide mode is enabled the max
> > > pixel clock limit is also doubled.
> > > 
> > > FIXME: panel fitter downscaling probably affects the limit on
> > > non-pch platforms too, so we'd need another version of
> > > ilk_pipe_pixel_rate() to figure that out.
> > > 
> > > FIXME: should check the limits on all platforms. Also sprites
> > > affect the max allowed pixel rate on some platforms, so we need
> > > to eventually tie all the planes and pipes into one check in
> > > the future. But we need plane state pre-compute before that can
> > > happen.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index ea33468..040e0ef 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4140,6 +4140,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> > >  			return -EINVAL;
> > >  	}
> > >  
> > > +	/* FIXME should check pixel clock limits on all platforms */
> > 
> > We do by failing to find the pll. The fun is to move all that code into
> > the pipe computation stage.
> 
> Are you 100% sure the PLL algorithm can never give you a clock that
> exceeds the display core clock? Looking at the functions to calculate
> the core clock, there is an awful lot of variation there already, and we
> don't even have those functions filled out for modern platforms.
> 
> > The second part of the fun is that on newer
> > platforms dotclock limits are all encoder specific, so we need to shovel
> > them into encoder callbacks anyway.
> > 
> > I don't know whether there are any additional pixel clock limits on top of
> > what the plls can handle, but at least I haven't spotted them yet ...
> 
> The display core clock is the big one. We have to check it for other
> platforms too eventually since the limit depends on a bunch of factors.
> For example on ILK/SNB we need to check whether there's one or two sprites
> enabled, are they using RGB or YUV, are they scaled, and if so by how
> much? Obviouly we can't do all that until we can pre-compute the state
> of all planes. 
> 
> So actually we may need some more steps in the process. Something like
> this:
> 1) adjust timings
> 2) calculate PLL settings to get the real clock
> 3) check double wide etc. that depends on the clock
> 4) compute plane config
> 5) check core clock and other plane related global limits
> 
> But I don't mind dropping this one now, and revisiting the issue with a
> bigger hammer in the future.

I think this one here is good, I've only taking issues with the FIXME
comment since I've thought most of this is encoder specific. But it sounds
like there's an entire set of constraints on the pixel value construction
side of things that we're currently completely missing. So I guess this is
ok.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch