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

[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?
-Daniel

> 
> > But with the test fixed up to be consistent (gen >= 9 || HAS_DDI):
> > Reviewed-by: Daniel Stone <daniels at collabora.com>
> >
> > Cheers,
> > Daniel
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch