[Intel-gfx] [PATCH v3 10/20] drm/i915: Set csc coefficients in intel_sanitize_crtc.

[Maarten Lankhorst]

Op 14-07-15 om 11:55 schreef Daniel Vetter:
> On Tue, Jul 14, 2015 at 10:50:21AM +0200, Maarten Lankhorst wrote:
>> Op 13-07-15 om 18:21 schreef Daniel Stone:
>>> Hi,
>>>
>>> On 13 July 2015 at 15:30, Maarten Lankhorst
>>> <maarten.lankhorst at linux.intel.com> wrote:
>>>> This might not have been set during boot, and when we preserve
>>>> the initial mode this can result in a black screen.
>>>>
>>>> Cc: Daniel Stone <daniels at collabora.com>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 819a2b551f1f..80e878929bab 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -15251,6 +15251,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>>>                 drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>>>>                 update_scanline_offset(crtc);
>>>>                 drm_crtc_vblank_on(&crtc->base);
>>>> +
>>>> +               if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen > 7)
>>>> +                       intel_set_pipe_csc(&crtc->base);
>>>>         }
>>> This is a bit of a weird one - and not your fault.
>>>
>>> intel_set_pipe_csc is currently only called from haswell_crtc_enable,
>>> which is only called for gen >= 9 or HAS_DDI. The (IS_HASWELL || gen >
>>> 7) test covers these devices, plus CHV as well. Does it work on CHV?
>> I think testing for HAS_DDI is enough, works for skylake too.
>>> I'd be more tempted to just call this unconditionally, and stick an
>>> early-exit test in intel_set_pipe_csc instead of at the callsites. But
>>> intel_set_pipe_csc covers gen >= 6, which can never be triggered
>>> through haswell_crtc_enable. So, if we added the early-exit to
>>> set_pipe_csc, we'd also need to remove the previous-gen codepath, or
>>> add calls to set_pipe_csc which didn't previously exist.
>> But CSC is only enabled in update_primary_plane for haswell+, so no point in sanitizing something unused. :-)
> I think we should instead stick the CSC update into the
> update_primary_plane hook then? Since before that point we don't care
> about CSC state at all.
>
> Also CSC states need to change atomically with plane updates anyway, so
> this seems like the right path forward. Can we postpone fixing this until
> fastboot happens?
>
Postponing is fine.

~Maarten