[Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 17, 2015 at 02:59:35PM +0100, Maarten Lankhorst wrote:
> Op 03-11-15 om 14:11 schreef Ville Syrjälä:
> > On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote:
> >> Op 03-11-15 om 11:40 schreef Ville Syrjälä:
> >>> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
> >>>> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> >>>>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> >>>>>> This fixes a warning when the crtc is turned off. In that case fb
> >>>>>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> >>>>>> active this is not a bug, and shouldn't trigger the WARN_ON.
> >>>>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
> >>>>> check .enabled, no?
> >>>> Not sure what the right decision would be here..
> >>>>
> >>>>      * skl max scale is lower of:
> >>>>      *    close to 3 but not 3, -1 is for that purpose
> >>>>      *            or
> >>>>      *    cdclk/crtc_clock
> >>>>
> >>>> So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
> >>>> cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.
> >>>>
> >>>> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
> >>> I think we should keep around the min required cdclk in the crtc state.
> >>> And we recalculate that one every time anything changes. Then we can
> >>> just compare it against the current cdclk after plane/crtc states have
> >>> otherwise been computed to see if we need to change the current cdclk.
> >>>
> >> What would the use be here? In that case the above formula reduces to 1<<16.
> >> If you want to treat dpms off the same as dpms on then you need the separate cdclk's..
> > I don't understand what you're saying. We need to calculate the required
> > cdclk based on dpms on. And that is what we must check against the
> > hardware limit. If we then want to optimize the dpms off case we can
> > just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to
> > do that when all pipes are in dpms off. But if we have some pipes in
> > dpms off and some in dpms on, we may want to skip the 'active' check
> > for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker
> > when some of the remaining pipes transition from dpms off to dpms on.
> Yeah but this would break things, or we'd need to keep 2 cdclk freqs globally.
> One with the real frequency, the other one used for calculations in atomic.
> And only when all crtc's are disabled they would differ.
> 
> In any case, when a plane is DPMS off this code won't be useful, visible = false
> because the rectangle will get clipped to (0,0)x(0,0).

And that must be changed.

> 
> Saying no scaling is possible would be fine here I think, without side effects.

-- 
Ville Syrjälä
Intel OTC