Re: [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 03, 2013 at 02:08:33PM -0300, Paulo Zanoni wrote:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Panel fitters on ivb/hsw are not created equal since not all of them
> > support the new high-quality upscaling mode. To offset this the hw
> > allows us to freely assign the pfits to pipes.
> >
> > Since our code currently doesn't support this we might fall over when
> > taking over firmware state. So check for this case and WARN about it.
> > We can then improve the code once we've hit this in the wild. Or once
> > we decide to support the improved upscale modes, though that requires
> > global arbitrage of modeset resources across crtcs.
> >
> > Suggested-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 432e699..2b6e141 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5806,6 +5806,14 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc,
> >         if (tmp & PF_ENABLE) {
> >                 pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe));
> >                 pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe));
> > +
> > +               /* We currently do not free assignements of panel fitters on
> > +                * ivb/hsw (since we don't use the higher upscaling modes which
> > +                * differentiates them) so just WARN about this case for now. */
> > +               if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> 
> Or just check for IS_GEN7 or, to be future-proof, check for "gen >= 7"
> since there's a higher chance that newer gens will be similar to gen 7
> instead of the previous ones.

As discussed in private, I think the explicit list here is better. But
checking for IS_GEN7 is a good idea - I didn't realize that vlv is already
excluded here. So applied that little changed.
> 
> Anyway: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged all patches from this series, thanks a lot for your critical
review. It's not always fun, but it _does_ improve the patches by a lot
;-)

Cheers, Daniel

> 
> > +                       WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
> > +                               PF_PIPE_SEL_IVB(crtc->pipe));
> > +               }
> >         }
> >  }
> >
> > --
> > 1.7.11.7
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch