[Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Tvrtko Ursulin]

On 06/15/2015 02:09 PM, Damien Lespiau wrote:
> On Mon, Jun 15, 2015 at 01:40:24PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/15/2015 01:14 PM, Damien Lespiau wrote:
>>> On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
>>>> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>
>>>>> Rather than reading out the current cdclk value use the cached value we
>>>>> have tucked away in dev_priv.
>>>>>
>>>>> v2: Rebased to the latest
>>>>> v3: Rebased to the latest
>>>>> v4: Fix for patch style problems
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> Signed-off-by: Mika Kahola <mika.kahola at intel.com>
>>>>
>>>> This patch needs to be extended to also cover the recently added
>>>> skl_max_scale. Tvrtko has recently written a patch to add some checks to
>>>> that code too, would be good to resurrect that too. Chandra can help with
>>>> any questions wrt the skl scaler code.
>>>
>>> Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
>>> of the atomic state, even bumping CDCLK if possible/needed.
>>>
>>> If you use the cached cdclk in skl_max_scale(), it won't do the right
>>> thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
>>> and we try to do the first modeset before waking up the display.
>>>
>>> I filed a bug about it already to track it:
>>>
>>>    https://bugs.freedesktop.org/show_bug.cgi?id=90874
>>
>> I know nothing about these specific clocks, but FWIW, my patch was only
>> about enabling new platforms - making skl_max_scale more robust in cases
>> where clock querying does not yet work correctly.
>>
>> My reasoning was based on a comment from Ville that one of those two clocks
>> must never be lower than the other.
>>
>> So it sounded reasonable to ignore such cases ie. assume no scaling is
>> possible and allow a normal (unscaled) modeset to succeed rather than fail
>> it and display nothing.
>
> So to be more specific, I believe this is because we detect CDCLK as
> being "disabled" or on the ref clock in simulation?

Probably a reference clock. It definitely wasn't zero since 
skl_max_scale already handles that. But I forgot the exact details.

> Generally speaking, it's questionable if we want to work around such
> limitations in the code like that, I'd rather go for defaulting to max
> CDCLK in simulation.
>
> In this particular case, we really shouldn't get cdclk < crtc_clock at
> this point, I'd expect the cdclk we use (probably part of the atomic
> state) to be bumped to cover crtc_clock prior to plane checks (See
> Marteen's [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic
> state.), I guess we could add a WARN_ON(cdclk < crtc_clock) in
> skl_max_scale() to ensure that's indeed the case?

WARN_ON sounds fine to me. For the other considerations - you're the 
expert. :)

Regards,

Tvrtko