| Message ID | 20190830124533.26573-9-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSB enablement. | expand |
On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@intel.com> wrote: > Gamma lut programming can be programmed using DSB > where bulk register programming can be done using indexed > register write which takes number of data and the mmio offset > to be written. > > v1: Initial version. > v2: Directly call dsb-api at callsites. (Jani) > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- > drivers/gpu/drm/i915/i915_drv.h | 2 + > 2 files changed, 43 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index 71a0201437a9..e4090d8e4547 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > const struct drm_color_lut *lut = blob->data; > int i, lut_size = drm_color_lut_size(blob); > enum pipe pipe = crtc->pipe; > + struct intel_dsb *dsb = dev_priv->dsb; No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You have intel_crtc for that. Was this not clear from my previous review? You have tons of functions here that will never have a DSB engine, it makes no sense to convert all of them to use the DSB. > > - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | > - PAL_PREC_AUTO_INCREMENT); > + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), > + prec_index | PAL_PREC_AUTO_INCREMENT); > > for (i = 0; i < hw_lut_size; i++) { > /* We discard half the user entries in split gamma mode */ > const struct drm_color_lut *entry = > &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; > > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > + ilk_lut_10(entry)); > } > > /* > * Reset the index, otherwise it prevents the legacy palette to be > * written properly. > */ > - I915_WRITE(PREC_PAL_INDEX(pipe), 0); > + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); > } > > static void ivb_load_lut_ext_max(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_dsb *dsb = dev_priv->dsb; > enum pipe pipe = crtc->pipe; > > /* Program the max register to clamp values > 1.0. */ > - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); > - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); > - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); > + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); > + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); > + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); > + > > /* > * Program the gc max 2 register to clamp values > 1.0. > @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) > * from 3.0 to 7.0 > */ > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); > - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); > - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); > + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), > + 1 << 16); > + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), > + 1 << 16); > + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), > + 1 << 16); > } > } > > @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state, > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_dsb *dsb = dev_priv->dsb; > enum pipe pipe = crtc->pipe; > > /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ > - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); > - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); > - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); > + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); > + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); > + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); > } > > static void > @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > const struct drm_property_blob *blob = crtc_state->base.gamma_lut; > const struct drm_color_lut *lut = blob->data; > + struct intel_dsb *dsb = dev_priv->dsb; > enum pipe pipe = crtc->pipe; > u32 i; > > @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) > * Superfine segment has 9 entries, corresponding to values > * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). > */ > - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); > + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), > + PAL_PREC_AUTO_INCREMENT); > > for (i = 0; i < 9; i++) { > const struct drm_color_lut *entry = &lut[i]; > > - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), > - ilk_lut_12p4_ldw(entry)); > - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), > - ilk_lut_12p4_udw(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), > + ilk_lut_12p4_ldw(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), > + ilk_lut_12p4_udw(entry)); > } > } > > @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) > const struct drm_property_blob *blob = crtc_state->base.gamma_lut; > const struct drm_color_lut *lut = blob->data; > const struct drm_color_lut *entry; > + struct intel_dsb *dsb = dev_priv->dsb; > enum pipe pipe = crtc->pipe; > u32 i; > > @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) > * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], > * with seg2[0] being unused by the hardware. > */ > - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); > + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); > for (i = 1; i < 257; i++) { > entry = &lut[i * 8]; > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > + ilk_lut_12p4_ldw(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > + ilk_lut_12p4_udw(entry)); > } > > /* > @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) > */ > for (i = 0; i < 256; i++) { > entry = &lut[i * 8 * 128]; > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > + ilk_lut_12p4_ldw(entry)); > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > + ilk_lut_12p4_udw(entry)); > } > > /* The last entry in the LUT is to be programmed in GCMAX */ > @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); No, don't do this. Don't store crtc specific info in a device global structure. > dev_priv->display.load_luts(crtc_state); > + intel_dsb_commit(dev_priv->dsb); > + intel_dsb_put(dev_priv->dsb); Please localize the use of DSB where needed. Move the gets and puts within the relevant platform specific ->load_luts hooks. > } > > void intel_color_commit(const struct intel_crtc_state *crtc_state) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7aa11e3dd413..98f546c4ad49 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1760,6 +1760,8 @@ struct drm_i915_private { > /* Mutex to protect the above hdcp component related values. */ > struct mutex hdcp_comp_mutex; > > + struct intel_dsb *dsb; > + > /* > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch > * will be rejected. Instead look for a better place.
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani > Nikula > Sent: Friday, August 30, 2019 7:02 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v4 08/10] drm/i915/dsb: Enable gamma lut > programming using DSB. > > On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@intel.com> wrote: > > Gamma lut programming can be programmed using DSB where bulk register > > programming can be done using indexed register write which takes > > number of data and the mmio offset to be written. > > > > v1: Initial version. > > v2: Directly call dsb-api at callsites. (Jani) > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- > > drivers/gpu/drm/i915/i915_drv.h | 2 + > > 2 files changed, 43 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c > > b/drivers/gpu/drm/i915/display/intel_color.c > > index 71a0201437a9..e4090d8e4547 100644 > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > > const struct drm_color_lut *lut = blob->data; > > int i, lut_size = drm_color_lut_size(blob); > > enum pipe pipe = crtc->pipe; > > + struct intel_dsb *dsb = dev_priv->dsb; > > No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You have > intel_crtc for that. > > Was this not clear from my previous review? > > You have tons of functions here that will never have a DSB engine, it makes no sense > to convert all of them to use the DSB. > Jani, I was thinking even among the 3 engines available per pipe, would it make more sense to keep them reserve on the basis of user ? like DSB0 for all pipe operations, DSB1 for all plane, and DSB2 for all encoder related stuff. We can create a DSB user (like we have for scaler) and index these engines based on that. Do you think so ? > > > > - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | > > - PAL_PREC_AUTO_INCREMENT); > > + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), > > + prec_index | PAL_PREC_AUTO_INCREMENT); > > > > for (i = 0; i < hw_lut_size; i++) { > > /* We discard half the user entries in split gamma mode */ > > const struct drm_color_lut *entry = > > &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; > > > > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); > > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > > + ilk_lut_10(entry)); > > } > > > > /* > > * Reset the index, otherwise it prevents the legacy palette to be > > * written properly. > > */ > > - I915_WRITE(PREC_PAL_INDEX(pipe), 0); > > + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); > > } > > > > static void ivb_load_lut_ext_max(struct intel_crtc *crtc) { > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_dsb *dsb = dev_priv->dsb; > > enum pipe pipe = crtc->pipe; > > > > /* Program the max register to clamp values > 1.0. */ > > - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); > > - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); > > - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); > > + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); > > + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); > > + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); > > + > > > > /* > > * Program the gc max 2 register to clamp values > 1.0. > > @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) > > * from 3.0 to 7.0 > > */ > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > > - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); > > - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); > > - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); > > + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), > > + 1 << 16); > > + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), > > + 1 << 16); > > + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), > > + 1 << 16); > > } > > } > > > > @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state > > *crtc_state, { > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_dsb *dsb = dev_priv->dsb; > > enum pipe pipe = crtc->pipe; > > > > /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ > > - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); > > - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); > > - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); > > + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); > > + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); > > + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); > > } > > > > static void > > @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct > intel_crtc_state *crtc_state) > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > const struct drm_property_blob *blob = crtc_state->base.gamma_lut; > > const struct drm_color_lut *lut = blob->data; > > + struct intel_dsb *dsb = dev_priv->dsb; > > enum pipe pipe = crtc->pipe; > > u32 i; > > > > @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct > intel_crtc_state *crtc_state) > > * Superfine segment has 9 entries, corresponding to values > > * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). > > */ > > - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), > PAL_PREC_AUTO_INCREMENT); > > + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), > > + PAL_PREC_AUTO_INCREMENT); > > > > for (i = 0; i < 9; i++) { > > const struct drm_color_lut *entry = &lut[i]; > > > > - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), > > - ilk_lut_12p4_ldw(entry)); > > - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), > > - ilk_lut_12p4_udw(entry)); > > + intel_dsb_indexed_reg_write(dsb, > PREC_PAL_MULTI_SEG_DATA(pipe), > > + ilk_lut_12p4_ldw(entry)); > > + intel_dsb_indexed_reg_write(dsb, > PREC_PAL_MULTI_SEG_DATA(pipe), > > + ilk_lut_12p4_udw(entry)); > > } > > } > > > > @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct > intel_crtc_state *crtc_state) > > const struct drm_property_blob *blob = crtc_state->base.gamma_lut; > > const struct drm_color_lut *lut = blob->data; > > const struct drm_color_lut *entry; > > + struct intel_dsb *dsb = dev_priv->dsb; > > enum pipe pipe = crtc->pipe; > > u32 i; > > > > @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct > intel_crtc_state *crtc_state) > > * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], > > * with seg2[0] being unused by the hardware. > > */ > > - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); > > + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), > > +PAL_PREC_AUTO_INCREMENT); > > for (i = 1; i < 257; i++) { > > entry = &lut[i * 8]; > > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); > > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); > > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > > + ilk_lut_12p4_ldw(entry)); > > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > > + ilk_lut_12p4_udw(entry)); > > } > > > > /* > > @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct > intel_crtc_state *crtc_state) > > */ > > for (i = 0; i < 256; i++) { > > entry = &lut[i * 8 * 128]; > > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); > > - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); > > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > > + ilk_lut_12p4_ldw(entry)); > > + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), > > + ilk_lut_12p4_udw(entry)); > > } > > > > /* The last entry in the LUT is to be programmed in GCMAX */ @@ > > -980,7 +995,10 @@ void intel_color_load_luts(const struct > > intel_crtc_state *crtc_state) { > > struct drm_i915_private *dev_priv = > > to_i915(crtc_state->base.crtc->dev); > > > > + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); > > No, don't do this. Don't store crtc specific info in a device global structure. > > > dev_priv->display.load_luts(crtc_state); > > + intel_dsb_commit(dev_priv->dsb); > > + intel_dsb_put(dev_priv->dsb); > > Please localize the use of DSB where needed. Move the gets and puts within the > relevant platform specific ->load_luts hooks. > > > } > > > > void intel_color_commit(const struct intel_crtc_state *crtc_state) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index 7aa11e3dd413..98f546c4ad49 > > 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1760,6 +1760,8 @@ struct drm_i915_private { > > /* Mutex to protect the above hdcp component related values. */ > > struct mutex hdcp_comp_mutex; > > > > + struct intel_dsb *dsb; > > + > > /* > > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch > > * will be rejected. Instead look for a better place. > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 03 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote: >> -----Original Message----- >> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani >> Nikula >> Sent: Friday, August 30, 2019 7:02 PM >> To: Manna, Animesh <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH v4 08/10] drm/i915/dsb: Enable gamma lut >> programming using DSB. >> You have tons of functions here that will never have a DSB engine, it >> makes no sense to convert all of them to use the DSB. >> > Jani, > I was thinking even among the 3 engines available per pipe, would it > make more sense to keep them reserve on the basis of user ? like DSB0 > for all pipe operations, DSB1 for all plane, and DSB2 for all encoder > related stuff. We can create a DSB user (like we have for scaler) and > index these engines based on that. Do you think so ? And perhaps use some DSB engines to write immediately, some to write at vblank. However, all of this should be postponed to follow-up work. For now, if we use intel_dsb_write and friends with the dsb parameter as local variable passed in, converting to use a different engine is a metter of changing the preceding intel_dsb_get call to fetch the dsb pointer. Considering the progress of a patch series, the focus should be on getting patches merged. Getting the minimum sebsible enabling of DSB merged should be the focus here. The further iteration should happen in-tree, not out-of-tree. BR, Jani.
On 9/3/2019 1:29 PM, Jani Nikula wrote: > On Tue, 03 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote: >>> -----Original Message----- >>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani >>> Nikula >>> Sent: Friday, August 30, 2019 7:02 PM >>> To: Manna, Animesh <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org >>> Subject: Re: [Intel-gfx] [PATCH v4 08/10] drm/i915/dsb: Enable gamma lut >>> programming using DSB. >>> You have tons of functions here that will never have a DSB engine, it >>> makes no sense to convert all of them to use the DSB. >>> >> Jani, >> I was thinking even among the 3 engines available per pipe, would it >> make more sense to keep them reserve on the basis of user ? like DSB0 >> for all pipe operations, DSB1 for all plane, and DSB2 for all encoder >> related stuff. We can create a DSB user (like we have for scaler) and >> index these engines based on that. Do you think so ? > And perhaps use some DSB engines to write immediately, some to write at > vblank. However, all of this should be postponed to follow-up work. > > For now, if we use intel_dsb_write and friends with the dsb parameter as > local variable passed in, converting to use a different engine is a > metter of changing the preceding intel_dsb_get call to fetch the dsb > pointer. > > Considering the progress of a patch series, the focus should be on > getting patches merged. Getting the minimum sebsible enabling of DSB > merged should be the focus here. The further iteration should happen > in-tree, not out-of-tree. Sure, it makes sense to simplify this in steps. - Shashank > BR, > Jani. > >
On Fri, 30 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote: > On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@intel.com> wrote: >> Gamma lut programming can be programmed using DSB >> where bulk register programming can be done using indexed >> register write which takes number of data and the mmio offset >> to be written. >> >> v1: Initial version. >> v2: Directly call dsb-api at callsites. (Jani) >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- >> drivers/gpu/drm/i915/i915_drv.h | 2 + >> 2 files changed, 43 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >> index 71a0201437a9..e4090d8e4547 100644 >> --- a/drivers/gpu/drm/i915/display/intel_color.c >> +++ b/drivers/gpu/drm/i915/display/intel_color.c >> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, >> const struct drm_color_lut *lut = blob->data; >> int i, lut_size = drm_color_lut_size(blob); >> enum pipe pipe = crtc->pipe; >> + struct intel_dsb *dsb = dev_priv->dsb; > > No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You > have intel_crtc for that. > > Was this not clear from my previous review? > > You have tons of functions here that will never have a DSB engine, it > makes no sense to convert all of them to use the DSB. To clarify: All functions that could and should use the DSB, need to be converted to use intel_dsb_write and friends. That means replacing I915_WRITE with the relevant intel_dsb_write calls. *NOT* having if (dsb) intel_dsb_write else I915_WRITE. As agreed a long time ago, intel_dsb_write should handle the non-DSB cases by falling back to regular intel_uncore_write. My point is, if there are functions that will never be called on a platform that has DSB, there's no point in converting them over to use DSB. Obviously there are e.g. legacy gamma functions that also get called on newer platforms. Maybe I was hasty in my assesment, need to double check if all of these paths are reachable from DSB platforms. BR, Jani. > >> >> - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | >> - PAL_PREC_AUTO_INCREMENT); >> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), >> + prec_index | PAL_PREC_AUTO_INCREMENT); >> >> for (i = 0; i < hw_lut_size; i++) { >> /* We discard half the user entries in split gamma mode */ >> const struct drm_color_lut *entry = >> &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; >> >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_10(entry)); >> } >> >> /* >> * Reset the index, otherwise it prevents the legacy palette to be >> * written properly. >> */ >> - I915_WRITE(PREC_PAL_INDEX(pipe), 0); >> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); >> } >> >> static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> >> /* Program the max register to clamp values > 1.0. */ >> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >> + >> >> /* >> * Program the gc max 2 register to clamp values > 1.0. >> @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >> * from 3.0 to 7.0 >> */ >> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { >> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), >> + 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), >> + 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), >> + 1 << 16); >> } >> } >> >> @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state, >> { >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> >> /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ >> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); >> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); >> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); >> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); >> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); >> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); >> } >> >> static void >> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >> const struct drm_color_lut *lut = blob->data; >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> u32 i; >> >> @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >> * Superfine segment has 9 entries, corresponding to values >> * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). >> */ >> - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), >> + PAL_PREC_AUTO_INCREMENT); >> >> for (i = 0; i < 9; i++) { >> const struct drm_color_lut *entry = &lut[i]; >> >> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >> - ilk_lut_12p4_ldw(entry)); >> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >> - ilk_lut_12p4_udw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >> + ilk_lut_12p4_ldw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >> + ilk_lut_12p4_udw(entry)); >> } >> } >> >> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >> const struct drm_color_lut *lut = blob->data; >> const struct drm_color_lut *entry; >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> u32 i; >> >> @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >> * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], >> * with seg2[0] being unused by the hardware. >> */ >> - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> for (i = 1; i < 257; i++) { >> entry = &lut[i * 8]; >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_ldw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_udw(entry)); >> } >> >> /* >> @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >> */ >> for (i = 0; i < 256; i++) { >> entry = &lut[i * 8 * 128]; >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_ldw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_udw(entry)); >> } >> >> /* The last entry in the LUT is to be programmed in GCMAX */ >> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >> >> + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); > > No, don't do this. Don't store crtc specific info in a device global > structure. > >> dev_priv->display.load_luts(crtc_state); >> + intel_dsb_commit(dev_priv->dsb); >> + intel_dsb_put(dev_priv->dsb); > > Please localize the use of DSB where needed. Move the gets and puts > within the relevant platform specific ->load_luts hooks. > >> } >> >> void intel_color_commit(const struct intel_crtc_state *crtc_state) >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 7aa11e3dd413..98f546c4ad49 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1760,6 +1760,8 @@ struct drm_i915_private { >> /* Mutex to protect the above hdcp component related values. */ >> struct mutex hdcp_comp_mutex; >> >> + struct intel_dsb *dsb; >> + >> /* >> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch >> * will be rejected. Instead look for a better place.
Hi, On 9/3/2019 1:38 PM, Jani Nikula wrote: > On Fri, 30 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote: >> On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@intel.com> wrote: >>> Gamma lut programming can be programmed using DSB >>> where bulk register programming can be done using indexed >>> register write which takes number of data and the mmio offset >>> to be written. >>> >>> v1: Initial version. >>> v2: Directly call dsb-api at callsites. (Jani) >>> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- >>> drivers/gpu/drm/i915/i915_drv.h | 2 + >>> 2 files changed, 43 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >>> index 71a0201437a9..e4090d8e4547 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_color.c >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c >>> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, >>> const struct drm_color_lut *lut = blob->data; >>> int i, lut_size = drm_color_lut_size(blob); >>> enum pipe pipe = crtc->pipe; >>> + struct intel_dsb *dsb = dev_priv->dsb; >> No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You >> have intel_crtc for that. >> >> Was this not clear from my previous review? >> >> You have tons of functions here that will never have a DSB engine, it >> makes no sense to convert all of them to use the DSB. > To clarify: > > All functions that could and should use the DSB, need to be converted to > use intel_dsb_write and friends. That means replacing I915_WRITE with > the relevant intel_dsb_write calls. *NOT* having if (dsb) > intel_dsb_write else I915_WRITE. As agreed a long time ago, > intel_dsb_write should handle the non-DSB cases by falling back to > regular intel_uncore_write. > > My point is, if there are functions that will never be called on a > platform that has DSB, there's no point in converting them over to use > DSB. Obviously there are e.g. legacy gamma functions that also get > called on newer platforms. > > Maybe I was hasty in my assesment, need to double check if all of these > paths are reachable from DSB platforms. Currently multi-segmented gamma (12 bit gamma) is enabled by default for icl+ platform. Will it be fine to enable only 12 bit gamma through DSB and 8-bit/10-bit can be enabled later based on need? Regards, Animesh > > BR, > Jani. > > >>> >>> - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | >>> - PAL_PREC_AUTO_INCREMENT); >>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), >>> + prec_index | PAL_PREC_AUTO_INCREMENT); >>> >>> for (i = 0; i < hw_lut_size; i++) { >>> /* We discard half the user entries in split gamma mode */ >>> const struct drm_color_lut *entry = >>> &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; >>> >>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>> + ilk_lut_10(entry)); >>> } >>> >>> /* >>> * Reset the index, otherwise it prevents the legacy palette to be >>> * written properly. >>> */ >>> - I915_WRITE(PREC_PAL_INDEX(pipe), 0); >>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); >>> } >>> >>> static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >>> { >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> + struct intel_dsb *dsb = dev_priv->dsb; >>> enum pipe pipe = crtc->pipe; >>> >>> /* Program the max register to clamp values > 1.0. */ >>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >>> + >>> >>> /* >>> * Program the gc max 2 register to clamp values > 1.0. >>> @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >>> * from 3.0 to 7.0 >>> */ >>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { >>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); >>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); >>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); >>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), >>> + 1 << 16); >>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), >>> + 1 << 16); >>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), >>> + 1 << 16); >>> } >>> } >>> >>> @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state, >>> { >>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> + struct intel_dsb *dsb = dev_priv->dsb; >>> enum pipe pipe = crtc->pipe; >>> >>> /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ >>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); >>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); >>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); >>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); >>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); >>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); >>> } >>> >>> static void >>> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >>> const struct drm_color_lut *lut = blob->data; >>> + struct intel_dsb *dsb = dev_priv->dsb; >>> enum pipe pipe = crtc->pipe; >>> u32 i; >>> >>> @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >>> * Superfine segment has 9 entries, corresponding to values >>> * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). >>> */ >>> - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >>> + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), >>> + PAL_PREC_AUTO_INCREMENT); >>> >>> for (i = 0; i < 9; i++) { >>> const struct drm_color_lut *entry = &lut[i]; >>> >>> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >>> - ilk_lut_12p4_ldw(entry)); >>> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >>> - ilk_lut_12p4_udw(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >>> + ilk_lut_12p4_ldw(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >>> + ilk_lut_12p4_udw(entry)); >>> } >>> } >>> >>> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >>> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >>> const struct drm_color_lut *lut = blob->data; >>> const struct drm_color_lut *entry; >>> + struct intel_dsb *dsb = dev_priv->dsb; >>> enum pipe pipe = crtc->pipe; >>> u32 i; >>> >>> @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >>> * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], >>> * with seg2[0] being unused by the hardware. >>> */ >>> - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >>> for (i = 1; i < 257; i++) { >>> entry = &lut[i * 8]; >>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>> + ilk_lut_12p4_ldw(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>> + ilk_lut_12p4_udw(entry)); >>> } >>> >>> /* >>> @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >>> */ >>> for (i = 0; i < 256; i++) { >>> entry = &lut[i * 8 * 128]; >>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>> + ilk_lut_12p4_ldw(entry)); >>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>> + ilk_lut_12p4_udw(entry)); >>> } >>> >>> /* The last entry in the LUT is to be programmed in GCMAX */ >>> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) >>> { >>> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >>> >>> + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); >> No, don't do this. Don't store crtc specific info in a device global >> structure. >> >>> dev_priv->display.load_luts(crtc_state); >>> + intel_dsb_commit(dev_priv->dsb); >>> + intel_dsb_put(dev_priv->dsb); >> Please localize the use of DSB where needed. Move the gets and puts >> within the relevant platform specific ->load_luts hooks. >> >>> } >>> >>> void intel_color_commit(const struct intel_crtc_state *crtc_state) >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index 7aa11e3dd413..98f546c4ad49 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -1760,6 +1760,8 @@ struct drm_i915_private { >>> /* Mutex to protect the above hdcp component related values. */ >>> struct mutex hdcp_comp_mutex; >>> >>> + struct intel_dsb *dsb; >>> + >>> /* >>> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch >>> * will be rejected. Instead look for a better place.
On Tue, 03 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote: > Hi, > > > On 9/3/2019 1:38 PM, Jani Nikula wrote: >> On Fri, 30 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote: >>> On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@intel.com> wrote: >>>> Gamma lut programming can be programmed using DSB >>>> where bulk register programming can be done using indexed >>>> register write which takes number of data and the mmio offset >>>> to be written. >>>> >>>> v1: Initial version. >>>> v2: Directly call dsb-api at callsites. (Jani) >>>> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- >>>> drivers/gpu/drm/i915/i915_drv.h | 2 + >>>> 2 files changed, 43 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >>>> index 71a0201437a9..e4090d8e4547 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_color.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c >>>> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, >>>> const struct drm_color_lut *lut = blob->data; >>>> int i, lut_size = drm_color_lut_size(blob); >>>> enum pipe pipe = crtc->pipe; >>>> + struct intel_dsb *dsb = dev_priv->dsb; >>> No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You >>> have intel_crtc for that. >>> >>> Was this not clear from my previous review? >>> >>> You have tons of functions here that will never have a DSB engine, it >>> makes no sense to convert all of them to use the DSB. >> To clarify: >> >> All functions that could and should use the DSB, need to be converted to >> use intel_dsb_write and friends. That means replacing I915_WRITE with >> the relevant intel_dsb_write calls. *NOT* having if (dsb) >> intel_dsb_write else I915_WRITE. As agreed a long time ago, >> intel_dsb_write should handle the non-DSB cases by falling back to >> regular intel_uncore_write. >> >> My point is, if there are functions that will never be called on a >> platform that has DSB, there's no point in converting them over to use >> DSB. Obviously there are e.g. legacy gamma functions that also get >> called on newer platforms. >> >> Maybe I was hasty in my assesment, need to double check if all of these >> paths are reachable from DSB platforms. > > Currently multi-segmented gamma (12 bit gamma) is enabled by default for > icl+ platform. > Will it be fine to enable only 12 bit gamma through DSB and 8-bit/10-bit > can be enabled later based on need? Minimal enabling is fine. Let's get *something* out there first, and iterate. BR, Jani. > > Regards, > Animesh > >> >> BR, >> Jani. >> >> >>>> >>>> - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | >>>> - PAL_PREC_AUTO_INCREMENT); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), >>>> + prec_index | PAL_PREC_AUTO_INCREMENT); >>>> >>>> for (i = 0; i < hw_lut_size; i++) { >>>> /* We discard half the user entries in split gamma mode */ >>>> const struct drm_color_lut *entry = >>>> &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; >>>> >>>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>>> + ilk_lut_10(entry)); >>>> } >>>> >>>> /* >>>> * Reset the index, otherwise it prevents the legacy palette to be >>>> * written properly. >>>> */ >>>> - I915_WRITE(PREC_PAL_INDEX(pipe), 0); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); >>>> } >>>> >>>> static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >>>> { >>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>>> + struct intel_dsb *dsb = dev_priv->dsb; >>>> enum pipe pipe = crtc->pipe; >>>> >>>> /* Program the max register to clamp values > 1.0. */ >>>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >>>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >>>> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >>>> + >>>> >>>> /* >>>> * Program the gc max 2 register to clamp values > 1.0. >>>> @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >>>> * from 3.0 to 7.0 >>>> */ >>>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { >>>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); >>>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); >>>> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), >>>> + 1 << 16); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), >>>> + 1 << 16); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), >>>> + 1 << 16); >>>> } >>>> } >>>> >>>> @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state, >>>> { >>>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>>> + struct intel_dsb *dsb = dev_priv->dsb; >>>> enum pipe pipe = crtc->pipe; >>>> >>>> /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ >>>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); >>>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); >>>> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); >>>> } >>>> >>>> static void >>>> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>>> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >>>> const struct drm_color_lut *lut = blob->data; >>>> + struct intel_dsb *dsb = dev_priv->dsb; >>>> enum pipe pipe = crtc->pipe; >>>> u32 i; >>>> >>>> @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >>>> * Superfine segment has 9 entries, corresponding to values >>>> * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). >>>> */ >>>> - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), >>>> + PAL_PREC_AUTO_INCREMENT); >>>> >>>> for (i = 0; i < 9; i++) { >>>> const struct drm_color_lut *entry = &lut[i]; >>>> >>>> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >>>> - ilk_lut_12p4_ldw(entry)); >>>> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >>>> - ilk_lut_12p4_udw(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >>>> + ilk_lut_12p4_ldw(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >>>> + ilk_lut_12p4_udw(entry)); >>>> } >>>> } >>>> >>>> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >>>> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >>>> const struct drm_color_lut *lut = blob->data; >>>> const struct drm_color_lut *entry; >>>> + struct intel_dsb *dsb = dev_priv->dsb; >>>> enum pipe pipe = crtc->pipe; >>>> u32 i; >>>> >>>> @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >>>> * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], >>>> * with seg2[0] being unused by the hardware. >>>> */ >>>> - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >>>> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >>>> for (i = 1; i < 257; i++) { >>>> entry = &lut[i * 8]; >>>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >>>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>>> + ilk_lut_12p4_ldw(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>>> + ilk_lut_12p4_udw(entry)); >>>> } >>>> >>>> /* >>>> @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >>>> */ >>>> for (i = 0; i < 256; i++) { >>>> entry = &lut[i * 8 * 128]; >>>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >>>> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>>> + ilk_lut_12p4_ldw(entry)); >>>> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >>>> + ilk_lut_12p4_udw(entry)); >>>> } >>>> >>>> /* The last entry in the LUT is to be programmed in GCMAX */ >>>> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) >>>> { >>>> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >>>> >>>> + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); >>> No, don't do this. Don't store crtc specific info in a device global >>> structure. >>> >>>> dev_priv->display.load_luts(crtc_state); >>>> + intel_dsb_commit(dev_priv->dsb); >>>> + intel_dsb_put(dev_priv->dsb); >>> Please localize the use of DSB where needed. Move the gets and puts >>> within the relevant platform specific ->load_luts hooks. >>> >>>> } >>>> >>>> void intel_color_commit(const struct intel_crtc_state *crtc_state) >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index 7aa11e3dd413..98f546c4ad49 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -1760,6 +1760,8 @@ struct drm_i915_private { >>>> /* Mutex to protect the above hdcp component related values. */ >>>> struct mutex hdcp_comp_mutex; >>>> >>>> + struct intel_dsb *dsb; >>>> + >>>> /* >>>> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch >>>> * will be rejected. Instead look for a better place. >
Hi, On 8/30/2019 7:02 PM, Jani Nikula wrote: > On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@intel.com> wrote: >> Gamma lut programming can be programmed using DSB >> where bulk register programming can be done using indexed >> register write which takes number of data and the mmio offset >> to be written. >> >> v1: Initial version. >> v2: Directly call dsb-api at callsites. (Jani) >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- >> drivers/gpu/drm/i915/i915_drv.h | 2 + >> 2 files changed, 43 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >> index 71a0201437a9..e4090d8e4547 100644 >> --- a/drivers/gpu/drm/i915/display/intel_color.c >> +++ b/drivers/gpu/drm/i915/display/intel_color.c >> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, >> const struct drm_color_lut *lut = blob->data; >> int i, lut_size = drm_color_lut_size(blob); >> enum pipe pipe = crtc->pipe; >> + struct intel_dsb *dsb = dev_priv->dsb; > No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You > have intel_crtc for that. > > Was this not clear from my previous review? > > You have tons of functions here that will never have a DSB engine, it > makes no sense to convert all of them to use the DSB. > >> >> - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | >> - PAL_PREC_AUTO_INCREMENT); >> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), >> + prec_index | PAL_PREC_AUTO_INCREMENT); >> >> for (i = 0; i < hw_lut_size; i++) { >> /* We discard half the user entries in split gamma mode */ >> const struct drm_color_lut *entry = >> &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; >> >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_10(entry)); >> } >> >> /* >> * Reset the index, otherwise it prevents the legacy palette to be >> * written properly. >> */ >> - I915_WRITE(PREC_PAL_INDEX(pipe), 0); >> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); >> } >> >> static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> >> /* Program the max register to clamp values > 1.0. */ >> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); >> + >> >> /* >> * Program the gc max 2 register to clamp values > 1.0. >> @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) >> * from 3.0 to 7.0 >> */ >> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { >> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); >> - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), >> + 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), >> + 1 << 16); >> + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), >> + 1 << 16); >> } >> } >> >> @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state, >> { >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> >> /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ >> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); >> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); >> - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); >> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); >> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); >> + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); >> } >> >> static void >> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >> const struct drm_color_lut *lut = blob->data; >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> u32 i; >> >> @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) >> * Superfine segment has 9 entries, corresponding to values >> * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). >> */ >> - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), >> + PAL_PREC_AUTO_INCREMENT); >> >> for (i = 0; i < 9; i++) { >> const struct drm_color_lut *entry = &lut[i]; >> >> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >> - ilk_lut_12p4_ldw(entry)); >> - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), >> - ilk_lut_12p4_udw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >> + ilk_lut_12p4_ldw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), >> + ilk_lut_12p4_udw(entry)); >> } >> } >> >> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >> const struct drm_property_blob *blob = crtc_state->base.gamma_lut; >> const struct drm_color_lut *lut = blob->data; >> const struct drm_color_lut *entry; >> + struct intel_dsb *dsb = dev_priv->dsb; >> enum pipe pipe = crtc->pipe; >> u32 i; >> >> @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >> * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], >> * with seg2[0] being unused by the hardware. >> */ >> - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> for (i = 1; i < 257; i++) { >> entry = &lut[i * 8]; >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_ldw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_udw(entry)); >> } >> >> /* >> @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) >> */ >> for (i = 0; i < 256; i++) { >> entry = &lut[i * 8 * 128]; >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); >> - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_ldw(entry)); >> + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), >> + ilk_lut_12p4_udw(entry)); >> } >> >> /* The last entry in the LUT is to be programmed in GCMAX */ >> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >> >> + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); > No, don't do this. Don't store crtc specific info in a device global > structure. > >> dev_priv->display.load_luts(crtc_state); >> + intel_dsb_commit(dev_priv->dsb); >> + intel_dsb_put(dev_priv->dsb); > Please localize the use of DSB where needed. Move the gets and puts > within the relevant platform specific ->load_luts hooks. As per offline discussion, adding get/put call in load_luts hook will be tricky as crtc-state is constant, As suggested by Shashank currently trying to have a single element in crtc-state and will add dsb-get call in atomic check if there is any change in color-property. Will add dsb-commit & dsb-put in commit-tail. Further improvement will be done based on need in future. Regards, Animesh > >> } >> >> void intel_color_commit(const struct intel_crtc_state *crtc_state) >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 7aa11e3dd413..98f546c4ad49 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1760,6 +1760,8 @@ struct drm_i915_private { >> /* Mutex to protect the above hdcp component related values. */ >> struct mutex hdcp_comp_mutex; >> >> + struct intel_dsb *dsb; >> + >> /* >> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch >> * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 71a0201437a9..e4090d8e4547 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, const struct drm_color_lut *lut = blob->data; int i, lut_size = drm_color_lut_size(blob); enum pipe pipe = crtc->pipe; + struct intel_dsb *dsb = dev_priv->dsb; - I915_WRITE(PREC_PAL_INDEX(pipe), prec_index | - PAL_PREC_AUTO_INCREMENT); + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), + prec_index | PAL_PREC_AUTO_INCREMENT); for (i = 0; i < hw_lut_size; i++) { /* We discard half the user entries in split gamma mode */ const struct drm_color_lut *entry = &lut[i * (lut_size - 1) / (hw_lut_size - 1)]; - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), + ilk_lut_10(entry)); } /* * Reset the index, otherwise it prevents the legacy palette to be * written properly. */ - I915_WRITE(PREC_PAL_INDEX(pipe), 0); + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0); } static void ivb_load_lut_ext_max(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_dsb *dsb = dev_priv->dsb; enum pipe pipe = crtc->pipe; /* Program the max register to clamp values > 1.0. */ - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); - I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16); + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16); + intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16); + /* * Program the gc max 2 register to clamp values > 1.0. @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc) * from 3.0 to 7.0 */ if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16); - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16); - I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16); + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0), + 1 << 16); + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1), + 1 << 16); + intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2), + 1 << 16); } } @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state, { struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_dsb *dsb = dev_priv->dsb; enum pipe pipe = crtc->pipe; /* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */ - I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red); - I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green); - I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue); + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red); + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green); + intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue); } static void @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); const struct drm_property_blob *blob = crtc_state->base.gamma_lut; const struct drm_color_lut *lut = blob->data; + struct intel_dsb *dsb = dev_priv->dsb; enum pipe pipe = crtc->pipe; u32 i; @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state) * Superfine segment has 9 entries, corresponding to values * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256). */ - I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); + intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe), + PAL_PREC_AUTO_INCREMENT); for (i = 0; i < 9; i++) { const struct drm_color_lut *entry = &lut[i]; - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), - ilk_lut_12p4_ldw(entry)); - I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), - ilk_lut_12p4_udw(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), + ilk_lut_12p4_ldw(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe), + ilk_lut_12p4_udw(entry)); } } @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) const struct drm_property_blob *blob = crtc_state->base.gamma_lut; const struct drm_color_lut *lut = blob->data; const struct drm_color_lut *entry; + struct intel_dsb *dsb = dev_priv->dsb; enum pipe pipe = crtc->pipe; u32 i; @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1], * with seg2[0] being unused by the hardware. */ - I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); + intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); for (i = 1; i < 257; i++) { entry = &lut[i * 8]; - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), + ilk_lut_12p4_ldw(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), + ilk_lut_12p4_udw(entry)); } /* @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state) */ for (i = 0; i < 256; i++) { entry = &lut[i * 8 * 128]; - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry)); - I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), + ilk_lut_12p4_ldw(entry)); + intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe), + ilk_lut_12p4_udw(entry)); } /* The last entry in the LUT is to be programmed in GCMAX */ @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc)); dev_priv->display.load_luts(crtc_state); + intel_dsb_commit(dev_priv->dsb); + intel_dsb_put(dev_priv->dsb); } void intel_color_commit(const struct intel_crtc_state *crtc_state) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7aa11e3dd413..98f546c4ad49 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1760,6 +1760,8 @@ struct drm_i915_private { /* Mutex to protect the above hdcp component related values. */ struct mutex hdcp_comp_mutex; + struct intel_dsb *dsb; + /* * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch * will be rejected. Instead look for a better place.
Gamma lut programming can be programmed using DSB where bulk register programming can be done using indexed register write which takes number of data and the mmio offset to be written. v1: Initial version. v2: Directly call dsb-api at callsites. (Jani) Cc: Jani Nikula <jani.nikula@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Animesh Manna <animesh.manna@intel.com> --- drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++-------- drivers/gpu/drm/i915/i915_drv.h | 2 + 2 files changed, 43 insertions(+), 23 deletions(-)