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

Maarten Lankhorst
2015-11-17T13:59:37Z

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).

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