| Message ID | 1436797833-11493-11-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 13 July 2015 at 15:30, Maarten Lankhorst <maarten.lankhorst@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@collabora.com> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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'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 with the test fixed up to be consistent (gen >= 9 || HAS_DDI): Reviewed-by: Daniel Stone <daniels@collabora.com> Cheers, Daniel
Op 13-07-15 om 18:21 schreef Daniel Stone: > Hi, > > On 13 July 2015 at 15:30, Maarten Lankhorst > <maarten.lankhorst@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@collabora.com> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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. :-) > But with the test fixed up to be consistent (gen >= 9 || HAS_DDI): > Reviewed-by: Daniel Stone <daniels@collabora.com> > > Cheers, > Daniel
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@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@collabora.com> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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@collabora.com> > > > > Cheers, > > Daniel > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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@collabora.com> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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
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); } /* We need to sanitize the plane -> pipe mapping first because this will
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@collabora.com> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 +++ 1 file changed, 3 insertions(+)