| Message ID | 20211125202750.3263848-3-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable pipe color support on D13 platform | expand |
On Fri, 26 Nov 2021, Uma Shankar <uma.shankar@intel.com> wrote: > Enable Pipe Degamma for XE_LPD. Extend the legacy implementation > to incorparate the extended lut size for XE_LPD. > > v2: Added a helper for degamma lut size (Ville) > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index 42fe549ef6fe..de3ded1e327a 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -808,6 +808,14 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state) > } > } > > +static int glk_degamma_lut_size(struct drm_i915_private *i915) > +{ > + if (DISPLAY_VER(i915) >= 13) > + return 131; > + else > + return 35; > +} > + Why do we have both a function with hardcoded values and device info members for this? BR, Jani. > static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > @@ -827,8 +835,8 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) > > for (i = 0; i < lut_size; i++) { > /* > - * First 33 entries represent range from 0 to 1.0 > - * 34th and 35th entry will represent extended range > + * First lut_size entries represent range from 0 to 1.0 > + * 3 additional lut entries will represent extended range > * inputs 3.0 and 7.0 respectively, currently clamped > * at 1.0. Since the precision is 16bit, the user > * value can be directly filled to register. > @@ -844,7 +852,7 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) > } > > /* Clamp values > 1.0. */ > - while (i++ < 35) > + while (i++ < glk_degamma_lut_size(dev_priv)) > intel_de_write_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe), 1 << 16); > > intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
On Mon, Nov 29, 2021 at 06:19:52PM +0200, Jani Nikula wrote: > On Fri, 26 Nov 2021, Uma Shankar <uma.shankar@intel.com> wrote: > > Enable Pipe Degamma for XE_LPD. Extend the legacy implementation > > to incorparate the extended lut size for XE_LPD. > > > > v2: Added a helper for degamma lut size (Ville) > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_color.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > index 42fe549ef6fe..de3ded1e327a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > @@ -808,6 +808,14 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state) > > } > > } > > > > +static int glk_degamma_lut_size(struct drm_i915_private *i915) > > +{ > > + if (DISPLAY_VER(i915) >= 13) > > + return 131; > > + else > > + return 35; > > +} > > + > > Why do we have both a function with hardcoded values and device info > members for this? The device info stuff just needs to get nuked. The size of the LUTs depends on the gamma mode which we already select dynamically (and if/when we get thre new uapi ironed out it'll become even more dynamic), so trying to represent it with a single number in device info is futile.
On Fri, Nov 26, 2021 at 01:57:49AM +0530, Uma Shankar wrote: > Enable Pipe Degamma for XE_LPD. Extend the legacy implementation > to incorparate the extended lut size for XE_LPD. > > v2: Added a helper for degamma lut size (Ville) > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index 42fe549ef6fe..de3ded1e327a 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -808,6 +808,14 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state) > } > } > > +static int glk_degamma_lut_size(struct drm_i915_private *i915) > +{ > + if (DISPLAY_VER(i915) >= 13) > + return 131; > + else > + return 35; > +} > + > static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > @@ -827,8 +835,8 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) > > for (i = 0; i < lut_size; i++) { > /* > - * First 33 entries represent range from 0 to 1.0 > - * 34th and 35th entry will represent extended range > + * First lut_size entries represent range from 0 to 1.0 > + * 3 additional lut entries will represent extended range > * inputs 3.0 and 7.0 respectively, currently clamped > * at 1.0. Since the precision is 16bit, the user > * value can be directly filled to register. > @@ -844,7 +852,7 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) > } > > /* Clamp values > 1.0. */ > - while (i++ < 35) > + while (i++ < glk_degamma_lut_size(dev_priv)) > intel_de_write_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe), 1 << 16); > > intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0); > -- > 2.25.1
On Tue, 30 Nov 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Nov 29, 2021 at 06:19:52PM +0200, Jani Nikula wrote: >> On Fri, 26 Nov 2021, Uma Shankar <uma.shankar@intel.com> wrote: >> > Enable Pipe Degamma for XE_LPD. Extend the legacy implementation >> > to incorparate the extended lut size for XE_LPD. >> > >> > v2: Added a helper for degamma lut size (Ville) >> > >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_color.c | 14 +++++++++++--- >> > 1 file changed, 11 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >> > index 42fe549ef6fe..de3ded1e327a 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_color.c >> > +++ b/drivers/gpu/drm/i915/display/intel_color.c >> > @@ -808,6 +808,14 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state) >> > } >> > } >> > >> > +static int glk_degamma_lut_size(struct drm_i915_private *i915) >> > +{ >> > + if (DISPLAY_VER(i915) >= 13) >> > + return 131; >> > + else >> > + return 35; >> > +} >> > + >> >> Why do we have both a function with hardcoded values and device info >> members for this? > > The device info stuff just needs to get nuked. The size of the LUTs > depends on the gamma mode which we already select dynamically (and > if/when we get thre new uapi ironed out it'll become even more > dynamic), so trying to represent it with a single number in device > info is futile. Works for me, I just like to have the single point of truth instead of split all over the place. Not against adding this now, but let's not forget to follow up with the cleanup. BR, Jani.
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Tuesday, November 30, 2021 3:36 PM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/xelpd: Enable Pipe Degamma > > On Tue, 30 Nov 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Mon, Nov 29, 2021 at 06:19:52PM +0200, Jani Nikula wrote: > >> On Fri, 26 Nov 2021, Uma Shankar <uma.shankar@intel.com> wrote: > >> > Enable Pipe Degamma for XE_LPD. Extend the legacy implementation to > >> > incorparate the extended lut size for XE_LPD. > >> > > >> > v2: Added a helper for degamma lut size (Ville) > >> > > >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_color.c | 14 +++++++++++--- > >> > 1 file changed, 11 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c > >> > b/drivers/gpu/drm/i915/display/intel_color.c > >> > index 42fe549ef6fe..de3ded1e327a 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_color.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_color.c > >> > @@ -808,6 +808,14 @@ static void bdw_load_luts(const struct intel_crtc_state > *crtc_state) > >> > } > >> > } > >> > > >> > +static int glk_degamma_lut_size(struct drm_i915_private *i915) { > >> > + if (DISPLAY_VER(i915) >= 13) > >> > + return 131; > >> > + else > >> > + return 35; > >> > +} > >> > + > >> > >> Why do we have both a function with hardcoded values and device info > >> members for this? > > > > The device info stuff just needs to get nuked. The size of the LUTs > > depends on the gamma mode which we already select dynamically (and > > if/when we get thre new uapi ironed out it'll become even more > > dynamic), so trying to represent it with a single number in device > > info is futile. > > Works for me, I just like to have the single point of truth instead of split all over the > place. Not against adding this now, but let's not forget to follow up with the > cleanup. Yeah, device info may not be needed once we have the new UAPI's. Will clean it up, once we get that closed. Thanks Jani and Ville for the review and feedbacks. Regards, Uma Shankar > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 42fe549ef6fe..de3ded1e327a 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -808,6 +808,14 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state) } } +static int glk_degamma_lut_size(struct drm_i915_private *i915) +{ + if (DISPLAY_VER(i915) >= 13) + return 131; + else + return 35; +} + static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); @@ -827,8 +835,8 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) for (i = 0; i < lut_size; i++) { /* - * First 33 entries represent range from 0 to 1.0 - * 34th and 35th entry will represent extended range + * First lut_size entries represent range from 0 to 1.0 + * 3 additional lut entries will represent extended range * inputs 3.0 and 7.0 respectively, currently clamped * at 1.0. Since the precision is 16bit, the user * value can be directly filled to register. @@ -844,7 +852,7 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state) } /* Clamp values > 1.0. */ - while (i++ < 35) + while (i++ < glk_degamma_lut_size(dev_priv)) intel_de_write_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe), 1 << 16); intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
Enable Pipe Degamma for XE_LPD. Extend the legacy implementation to incorparate the extended lut size for XE_LPD. v2: Added a helper for degamma lut size (Ville) Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/i915/display/intel_color.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)