Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected

From: Daniel Vetter <daniel@ffwll.ch>
To: "Konduru, Chandra" <chandra.konduru@intel.com>
Cc: "Nikula, Jani" <jani.nikula@intel.com>,
	"Intel-gfx@lists.freedesktop.org"
	<Intel-gfx@lists.freedesktop.org>,
	"Lankhorst, Maarten" <maarten.lankhorst@intel.com>
Subject: Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected
Date: Tue, 16 Jun 2015 15:40:16 +0200	[thread overview]
Message-ID: <20150616134016.GU23637@phenom.ffwll.local> (raw)
In-Reply-To: <76A9B330A4D78C4D99CB292C4CC06C0E3700789E@fmsmsx101.amr.corp.intel.com>

On Mon, Jun 15, 2015 at 09:03:09PM +0000, Konduru, Chandra wrote:
> > >
> > > Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere.
> > >
> > > It is more robust and safe to assume no scaling is possible in this case.
> > >
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > > index 93a5e51..4c99373 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct
> > intel_crtc_state *crtc_state
> > >  	crtc_clock = crtc_state->base.adjusted_mode.crtc_clock;
> > >  	cdclk = dev_priv->display.get_display_clock_speed(dev);
> > 
> > Probably fallout from the in-flight dynamic cdclk stuff - this code checks
> > the wrong bits I guess. Chandra?
> 
> Looks like something elsewhere has fallen out and issue manifested here.
> 
> Damien reported another issue where get_display_clock_speed causing 
> an assert because it is called when dev_priv->pm.suspend is true during
> runtime resume. But later  was resolved after one of atomic patch is 
> reverted.
> 
> While Maarten is addressing recently reported atomic issues, for 
> time being some atomic crtc patches were reverted.
> 
> I am not 100% sure whether issue here is due to same root cause or 
> due to something different.

You need to check the cached (and soon the one in the global atomic
modeset state structure) cdclk value, not the current one in the hw. And
yeah that can result in asserts since the hw might not be one yet when
this code is run. I.e. this isn't about atomic modeset but just about
interaction with the recent cdclk work. And with the existing rpm feature.

In the future we should even upclock the cdclck stuff (once dynamic cdclk
is implemented on skl) to make sure it fits the desired scaler
configuration. But that's follow-up work.
-Daniel
> 
> > 
> > Thanks, Daniel
> > 
> > >
> > > -	if (!crtc_clock || !cdclk)
> > > +	if (!crtc_clock || !cdclk || (cdclk < crtc_clock))
> > >  		return DRM_PLANE_HELPER_NO_SCALING;
> > >
> > >  	/*
> > > --
> > > 2.4.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx