Message ID | 20190318161317.30918-5-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915: Clean up intel_color_check() | expand |
On Mon, Mar 18, 2019 at 06:13:11PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Since CHV has the CGM unit we require a custom implementation > of .color_check(). > > This fixes the computation of gamma_enable as previously we > left it enabled even when were using the CGM gamma instead. > Now we turn off the legacy LUT unless it's actually required. The CGM stuff is a bit non-obvious to anyone who hasn't focused on the CHV platform specifically, so it might be worth adding this justification in a code comment. In fact, a general overview comment somewhere (maybe on the CHV_COLORS definition?) about the difference between legacy gamma and CGM might be helpful. It looks like our public PRM's for CHV don't really cover anything display-wise. Anyway, the code change looks correct, so Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_color.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index 3d19b1a7ee95..cb88651f1da4 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -848,6 +848,29 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > return cgm_mode; > } > > +static int chv_color_check(struct intel_crtc_state *crtc_state) > +{ > + int ret; > + > + ret = check_luts(crtc_state); > + if (ret) > + return ret; > + > + crtc_state->gamma_enable = > + crtc_state_is_legacy_gamma(crtc_state) && > + !crtc_state->c8_planes; > + > + crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > + > + crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > + > + ret = intel_color_add_affected_planes(crtc_state); > + if (ret) > + return ret; > + > + return 0; > +} > + > static int _intel_color_check(struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > @@ -877,9 +900,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state) > > crtc_state->csc_mode = 0; > > - if (IS_CHERRYVIEW(dev_priv)) > - crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > - > if (!crtc_state->gamma_enable || > crtc_state_is_legacy_gamma(crtc_state)) > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > @@ -917,7 +937,7 @@ void intel_color_init(struct intel_crtc *crtc) > > if (HAS_GMCH(dev_priv)) { > if (IS_CHERRYVIEW(dev_priv)) { > - dev_priv->display.color_check = _intel_color_check; > + dev_priv->display.color_check = chv_color_check; > dev_priv->display.color_commit = i9xx_color_commit; > dev_priv->display.load_luts = cherryview_load_luts; > } else { > -- > 2.19.2 >
On Tue, Mar 19, 2019 at 03:00:26PM -0700, Matt Roper wrote: > On Mon, Mar 18, 2019 at 06:13:11PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Since CHV has the CGM unit we require a custom implementation > > of .color_check(). > > > > This fixes the computation of gamma_enable as previously we > > left it enabled even when were using the CGM gamma instead. > > Now we turn off the legacy LUT unless it's actually required. > > The CGM stuff is a bit non-obvious to anyone who hasn't focused on the > CHV platform specifically, so it might be worth adding this > justification in a code comment. > > In fact, a general overview comment somewhere (maybe on the CHV_COLORS > definition?) about the difference between legacy gamma and CGM might be > helpful. It looks like our public PRM's for CHV don't really cover > anything display-wise. Yeah. I think I had a patch somewhere where I document the full pipeline for VLV/CHV. I'll see if I can dig it out. > > Anyway, the code change looks correct, so > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > Matt > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_color.c | 28 ++++++++++++++++++++++++---- > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > index 3d19b1a7ee95..cb88651f1da4 100644 > > --- a/drivers/gpu/drm/i915/intel_color.c > > +++ b/drivers/gpu/drm/i915/intel_color.c > > @@ -848,6 +848,29 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > > return cgm_mode; > > } > > > > +static int chv_color_check(struct intel_crtc_state *crtc_state) > > +{ > > + int ret; > > + > > + ret = check_luts(crtc_state); > > + if (ret) > > + return ret; > > + > > + crtc_state->gamma_enable = > > + crtc_state_is_legacy_gamma(crtc_state) && > > + !crtc_state->c8_planes; > > + > > + crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > + > > + crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > + > > + ret = intel_color_add_affected_planes(crtc_state); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > static int _intel_color_check(struct intel_crtc_state *crtc_state) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > @@ -877,9 +900,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state) > > > > crtc_state->csc_mode = 0; > > > > - if (IS_CHERRYVIEW(dev_priv)) > > - crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > - > > if (!crtc_state->gamma_enable || > > crtc_state_is_legacy_gamma(crtc_state)) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > @@ -917,7 +937,7 @@ void intel_color_init(struct intel_crtc *crtc) > > > > if (HAS_GMCH(dev_priv)) { > > if (IS_CHERRYVIEW(dev_priv)) { > > - dev_priv->display.color_check = _intel_color_check; > > + dev_priv->display.color_check = chv_color_check; > > dev_priv->display.color_commit = i9xx_color_commit; > > dev_priv->display.load_luts = cherryview_load_luts; > > } else { > > -- > > 2.19.2 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 3d19b1a7ee95..cb88651f1da4 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -848,6 +848,29 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) return cgm_mode; } +static int chv_color_check(struct intel_crtc_state *crtc_state) +{ + int ret; + + ret = check_luts(crtc_state); + if (ret) + return ret; + + crtc_state->gamma_enable = + crtc_state_is_legacy_gamma(crtc_state) && + !crtc_state->c8_planes; + + crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; + + crtc_state->cgm_mode = chv_cgm_mode(crtc_state); + + ret = intel_color_add_affected_planes(crtc_state); + if (ret) + return ret; + + return 0; +} + static int _intel_color_check(struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); @@ -877,9 +900,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state) crtc_state->csc_mode = 0; - if (IS_CHERRYVIEW(dev_priv)) - crtc_state->cgm_mode = chv_cgm_mode(crtc_state); - if (!crtc_state->gamma_enable || crtc_state_is_legacy_gamma(crtc_state)) crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; @@ -917,7 +937,7 @@ void intel_color_init(struct intel_crtc *crtc) if (HAS_GMCH(dev_priv)) { if (IS_CHERRYVIEW(dev_priv)) { - dev_priv->display.color_check = _intel_color_check; + dev_priv->display.color_check = chv_color_check; dev_priv->display.color_commit = i9xx_color_commit; dev_priv->display.load_luts = cherryview_load_luts; } else {