| Message ID | 20221026113906.10551-12-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: More gamma work | expand |
Patch looks good to me. Minor suggestions inline: On 10/26/2022 5:09 PM, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently when opeating in split gamma mode we do the nitpick: 'operating' typo. > "skip ever other sw LUT entry" trick in the low level > LUT programming/readout functions. That is very annoying > and a big hinderance to revamping the color management > uapi. > > Let's get rid of that problem by making half sized copies > of the software LUTs and plugging those into the internal > {pre,post}_csc_lut attachment points (instead of the sticking > the uapi provide sw LUTs there directly). > > With this the low level stuff will operate purely in terms > the hardware LUT sizes, and all uapi nonsense is contained > to the atomic check phase. The one thing we do lose is > intel_color_assert_luts() since we no longer have a way to > check that the uapi LUTs were correctly used when generating > the internal copies. But that seems like a price worth paying. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 81 +++++++++++++++++----- > 1 file changed, 64 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index 33871bfacee7..d48904f90e3a 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -597,6 +597,30 @@ create_linear_lut(struct drm_i915_private *i915, int lut_size) > return blob; > } > > +static struct drm_property_blob * > +create_resized_lut(struct drm_i915_private *i915, > + const struct drm_property_blob *blob_in, int lut_out_size) > +{ > + int i, lut_in_size = drm_color_lut_size(blob_in); > + struct drm_property_blob *blob_out; > + const struct drm_color_lut *lut_in; > + struct drm_color_lut *lut_out; > + > + blob_out = drm_property_create_blob(&i915->drm, > + sizeof(lut_out[0]) * lut_out_size, > + NULL); > + if (IS_ERR(blob_out)) > + return blob_out; > + > + lut_in = blob_in->data; > + lut_out = blob_out->data; > + > + for (i = 0; i < lut_out_size; i++) > + lut_out[i] = lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)]; > + > + return blob_out; > +} > + > static void i9xx_load_lut_8(struct intel_crtc *crtc, > const struct drm_property_blob *blob) > { > @@ -723,19 +747,14 @@ static void ivb_load_lut_10(struct intel_crtc *crtc, > u32 prec_index) > { > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > - int hw_lut_size = ivb_lut_10_size(prec_index); > const struct drm_color_lut *lut = blob->data; > int i, lut_size = drm_color_lut_size(blob); > enum pipe pipe = crtc->pipe; > > - 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)]; > - > + for (i = 0; i < lut_size; i++) { > intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++); > intel_de_write_fw(i915, PREC_PAL_DATA(pipe), > - ilk_lut_10(entry)); > + ilk_lut_10(&lut[i])); > } > > /* > @@ -751,7 +770,6 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > u32 prec_index) > { > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > - int hw_lut_size = ivb_lut_10_size(prec_index); > const struct drm_color_lut *lut = blob->data; > int i, lut_size = drm_color_lut_size(blob); > enum pipe pipe = crtc->pipe; > @@ -759,14 +777,9 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > intel_de_write_fw(i915, 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)]; > - > + for (i = 0; i < lut_size; i++) > intel_de_write_fw(i915, PREC_PAL_DATA(pipe), > - ilk_lut_10(entry)); > - } > + ilk_lut_10(&lut[i])); > > /* > * Reset the index, otherwise it prevents the legacy palette to be > @@ -1343,7 +1356,7 @@ void intel_color_assert_luts(const struct intel_crtc_state *crtc_state) > crtc_state->pre_csc_lut != i915->display.color.glk_linear_degamma_lut); > drm_WARN_ON(&i915->drm, > crtc_state->post_csc_lut != crtc_state->hw.gamma_lut); > - } else { > + } else if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { > drm_WARN_ON(&i915->drm, > crtc_state->pre_csc_lut != crtc_state->hw.degamma_lut && > crtc_state->pre_csc_lut != crtc_state->hw.gamma_lut); > @@ -1564,6 +1577,38 @@ static u32 ivb_csc_mode(const struct intel_crtc_state *crtc_state) > return CSC_POSITION_BEFORE_GAMMA; > } > > +static int ivb_assign_luts(struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > + struct drm_property_blob *degamma_lut, *gamma_lut; > + > + if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { > + ilk_assign_luts(crtc_state); > + return 0; > + } > + > + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.degamma_lut) != 1024); > + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.gamma_lut) != 1024); Does it make sense to use some macro for LUT size for split gamma case and regular case? Same thing perhaps can be used in ivb_lut_10_size? Regards, Ankit > + > + degamma_lut = create_resized_lut(i915, crtc_state->hw.degamma_lut, 512); > + if (IS_ERR(degamma_lut)) > + return PTR_ERR(degamma_lut); > + > + gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut, 512); > + if (IS_ERR(gamma_lut)) { > + drm_property_blob_put(degamma_lut); > + return PTR_ERR(gamma_lut); > + } > + > + drm_property_replace_blob(&crtc_state->pre_csc_lut, degamma_lut); > + drm_property_replace_blob(&crtc_state->post_csc_lut, gamma_lut); > + > + drm_property_blob_put(degamma_lut); > + drm_property_blob_put(gamma_lut); > + > + return 0; > +} > + > static int ivb_color_check(struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > @@ -1599,7 +1644,9 @@ static int ivb_color_check(struct intel_crtc_state *crtc_state) > if (ret) > return ret; > > - ilk_assign_luts(crtc_state); > + ret = ivb_assign_luts(crtc_state); > + if (ret) > + return ret; > > crtc_state->preload_luts = intel_can_preload_luts(crtc_state); >
On Fri, Nov 04, 2022 at 10:49:39AM +0530, Nautiyal, Ankit K wrote: > Patch looks good to me. > > Minor suggestions inline: > > On 10/26/2022 5:09 PM, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently when opeating in split gamma mode we do the > nitpick: 'operating' typo. > > "skip ever other sw LUT entry" trick in the low level > > LUT programming/readout functions. That is very annoying > > and a big hinderance to revamping the color management > > uapi. > > > > Let's get rid of that problem by making half sized copies > > of the software LUTs and plugging those into the internal > > {pre,post}_csc_lut attachment points (instead of the sticking > > the uapi provide sw LUTs there directly). > > > > With this the low level stuff will operate purely in terms > > the hardware LUT sizes, and all uapi nonsense is contained > > to the atomic check phase. The one thing we do lose is > > intel_color_assert_luts() since we no longer have a way to > > check that the uapi LUTs were correctly used when generating > > the internal copies. But that seems like a price worth paying. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_color.c | 81 +++++++++++++++++----- > > 1 file changed, 64 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > index 33871bfacee7..d48904f90e3a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > @@ -597,6 +597,30 @@ create_linear_lut(struct drm_i915_private *i915, int lut_size) > > return blob; > > } > > > > +static struct drm_property_blob * > > +create_resized_lut(struct drm_i915_private *i915, > > + const struct drm_property_blob *blob_in, int lut_out_size) > > +{ > > + int i, lut_in_size = drm_color_lut_size(blob_in); > > + struct drm_property_blob *blob_out; > > + const struct drm_color_lut *lut_in; > > + struct drm_color_lut *lut_out; > > + > > + blob_out = drm_property_create_blob(&i915->drm, > > + sizeof(lut_out[0]) * lut_out_size, > > + NULL); > > + if (IS_ERR(blob_out)) > > + return blob_out; > > + > > + lut_in = blob_in->data; > > + lut_out = blob_out->data; > > + > > + for (i = 0; i < lut_out_size; i++) > > + lut_out[i] = lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)]; > > + > > + return blob_out; > > +} > > + > > static void i9xx_load_lut_8(struct intel_crtc *crtc, > > const struct drm_property_blob *blob) > > { > > @@ -723,19 +747,14 @@ static void ivb_load_lut_10(struct intel_crtc *crtc, > > u32 prec_index) > > { > > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > - int hw_lut_size = ivb_lut_10_size(prec_index); > > const struct drm_color_lut *lut = blob->data; > > int i, lut_size = drm_color_lut_size(blob); > > enum pipe pipe = crtc->pipe; > > > > - 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)]; > > - > > + for (i = 0; i < lut_size; i++) { > > intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++); > > intel_de_write_fw(i915, PREC_PAL_DATA(pipe), > > - ilk_lut_10(entry)); > > + ilk_lut_10(&lut[i])); > > } > > > > /* > > @@ -751,7 +770,6 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > > u32 prec_index) > > { > > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > - int hw_lut_size = ivb_lut_10_size(prec_index); > > const struct drm_color_lut *lut = blob->data; > > int i, lut_size = drm_color_lut_size(blob); > > enum pipe pipe = crtc->pipe; > > @@ -759,14 +777,9 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > > intel_de_write_fw(i915, 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)]; > > - > > + for (i = 0; i < lut_size; i++) > > intel_de_write_fw(i915, PREC_PAL_DATA(pipe), > > - ilk_lut_10(entry)); > > - } > > + ilk_lut_10(&lut[i])); > > > > /* > > * Reset the index, otherwise it prevents the legacy palette to be > > @@ -1343,7 +1356,7 @@ void intel_color_assert_luts(const struct intel_crtc_state *crtc_state) > > crtc_state->pre_csc_lut != i915->display.color.glk_linear_degamma_lut); > > drm_WARN_ON(&i915->drm, > > crtc_state->post_csc_lut != crtc_state->hw.gamma_lut); > > - } else { > > + } else if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { > > drm_WARN_ON(&i915->drm, > > crtc_state->pre_csc_lut != crtc_state->hw.degamma_lut && > > crtc_state->pre_csc_lut != crtc_state->hw.gamma_lut); > > @@ -1564,6 +1577,38 @@ static u32 ivb_csc_mode(const struct intel_crtc_state *crtc_state) > > return CSC_POSITION_BEFORE_GAMMA; > > } > > > > +static int ivb_assign_luts(struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > > + struct drm_property_blob *degamma_lut, *gamma_lut; > > + > > + if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { > > + ilk_assign_luts(crtc_state); > > + return 0; > > + } > > + > > + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.degamma_lut) != 1024); > > + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.gamma_lut) != 1024); > > Does it make sense to use some macro for LUT size for split gamma case > and regular case? > > Same thing perhaps can be used in ivb_lut_10_size? I don't think macros would be really helpful. I guess I could have used ivb_lut_10_size() for the create_resized_lut() calls below. And these WARNs I guess could have just used device info stuff instead. Or I could just drop them entirely since they aren't really checking anything super important, and the create_resized_lut() would work with any input LUT size anyway. Thinking a bit further we could certainly consider extending the ivb_lut_10_size()/glk_degamma_lut_size() approach to cover all the gamma modes. Though I think it would probably make sense to implement that as some kind of struct based approach where we describe each LUT format in a struct. Would also be more in line with what we've been thinking for the uapi revamp.
On 11/4/2022 3:12 PM, Ville Syrjälä wrote: > On Fri, Nov 04, 2022 at 10:49:39AM +0530, Nautiyal, Ankit K wrote: >> Patch looks good to me. >> >> Minor suggestions inline: >> >> On 10/26/2022 5:09 PM, Ville Syrjala wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> Currently when opeating in split gamma mode we do the >> nitpick: 'operating' typo. >>> "skip ever other sw LUT entry" trick in the low level >>> LUT programming/readout functions. That is very annoying >>> and a big hinderance to revamping the color management >>> uapi. >>> >>> Let's get rid of that problem by making half sized copies >>> of the software LUTs and plugging those into the internal >>> {pre,post}_csc_lut attachment points (instead of the sticking >>> the uapi provide sw LUTs there directly). >>> >>> With this the low level stuff will operate purely in terms >>> the hardware LUT sizes, and all uapi nonsense is contained >>> to the atomic check phase. The one thing we do lose is >>> intel_color_assert_luts() since we no longer have a way to >>> check that the uapi LUTs were correctly used when generating >>> the internal copies. But that seems like a price worth paying. >>> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_color.c | 81 +++++++++++++++++----- >>> 1 file changed, 64 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >>> index 33871bfacee7..d48904f90e3a 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_color.c >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c >>> @@ -597,6 +597,30 @@ create_linear_lut(struct drm_i915_private *i915, int lut_size) >>> return blob; >>> } >>> >>> +static struct drm_property_blob * >>> +create_resized_lut(struct drm_i915_private *i915, >>> + const struct drm_property_blob *blob_in, int lut_out_size) >>> +{ >>> + int i, lut_in_size = drm_color_lut_size(blob_in); >>> + struct drm_property_blob *blob_out; >>> + const struct drm_color_lut *lut_in; >>> + struct drm_color_lut *lut_out; >>> + >>> + blob_out = drm_property_create_blob(&i915->drm, >>> + sizeof(lut_out[0]) * lut_out_size, >>> + NULL); >>> + if (IS_ERR(blob_out)) >>> + return blob_out; >>> + >>> + lut_in = blob_in->data; >>> + lut_out = blob_out->data; >>> + >>> + for (i = 0; i < lut_out_size; i++) >>> + lut_out[i] = lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)]; >>> + >>> + return blob_out; >>> +} >>> + >>> static void i9xx_load_lut_8(struct intel_crtc *crtc, >>> const struct drm_property_blob *blob) >>> { >>> @@ -723,19 +747,14 @@ static void ivb_load_lut_10(struct intel_crtc *crtc, >>> u32 prec_index) >>> { >>> struct drm_i915_private *i915 = to_i915(crtc->base.dev); >>> - int hw_lut_size = ivb_lut_10_size(prec_index); >>> const struct drm_color_lut *lut = blob->data; >>> int i, lut_size = drm_color_lut_size(blob); >>> enum pipe pipe = crtc->pipe; >>> >>> - 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)]; >>> - >>> + for (i = 0; i < lut_size; i++) { >>> intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++); >>> intel_de_write_fw(i915, PREC_PAL_DATA(pipe), >>> - ilk_lut_10(entry)); >>> + ilk_lut_10(&lut[i])); >>> } >>> >>> /* >>> @@ -751,7 +770,6 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, >>> u32 prec_index) >>> { >>> struct drm_i915_private *i915 = to_i915(crtc->base.dev); >>> - int hw_lut_size = ivb_lut_10_size(prec_index); >>> const struct drm_color_lut *lut = blob->data; >>> int i, lut_size = drm_color_lut_size(blob); >>> enum pipe pipe = crtc->pipe; >>> @@ -759,14 +777,9 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, >>> intel_de_write_fw(i915, 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)]; >>> - >>> + for (i = 0; i < lut_size; i++) >>> intel_de_write_fw(i915, PREC_PAL_DATA(pipe), >>> - ilk_lut_10(entry)); >>> - } >>> + ilk_lut_10(&lut[i])); >>> >>> /* >>> * Reset the index, otherwise it prevents the legacy palette to be >>> @@ -1343,7 +1356,7 @@ void intel_color_assert_luts(const struct intel_crtc_state *crtc_state) >>> crtc_state->pre_csc_lut != i915->display.color.glk_linear_degamma_lut); >>> drm_WARN_ON(&i915->drm, >>> crtc_state->post_csc_lut != crtc_state->hw.gamma_lut); >>> - } else { >>> + } else if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { >>> drm_WARN_ON(&i915->drm, >>> crtc_state->pre_csc_lut != crtc_state->hw.degamma_lut && >>> crtc_state->pre_csc_lut != crtc_state->hw.gamma_lut); >>> @@ -1564,6 +1577,38 @@ static u32 ivb_csc_mode(const struct intel_crtc_state *crtc_state) >>> return CSC_POSITION_BEFORE_GAMMA; >>> } >>> >>> +static int ivb_assign_luts(struct intel_crtc_state *crtc_state) >>> +{ >>> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); >>> + struct drm_property_blob *degamma_lut, *gamma_lut; >>> + >>> + if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { >>> + ilk_assign_luts(crtc_state); >>> + return 0; >>> + } >>> + >>> + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.degamma_lut) != 1024); >>> + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.gamma_lut) != 1024); >> Does it make sense to use some macro for LUT size for split gamma case >> and regular case? >> >> Same thing perhaps can be used in ivb_lut_10_size? > I don't think macros would be really helpful. I guess I > could have used ivb_lut_10_size() for the create_resized_lut() > calls below. And these WARNs I guess could have just used > device info stuff instead. Using ivb_lut_10_size() should be good enough, I think. In any case, this is a just a minor suggestion. Patch looks good to me. With the small typo fixed in commit message: Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> Regards, Ankit > Or I could just drop them entirely > since they aren't really checking anything super important, and > the create_resized_lut() would work with any input LUT size anyway. > > Thinking a bit further we could certainly consider extending > the ivb_lut_10_size()/glk_degamma_lut_size() approach to cover > all the gamma modes. Though I think it would probably make sense > to implement that as some kind of struct based approach where we > describe each LUT format in a struct. Would also be more in line > with what we've been thinking for the uapi revamp. >
On Thu, Nov 10, 2022 at 09:35:28AM +0530, Nautiyal, Ankit K wrote: > > On 11/4/2022 3:12 PM, Ville Syrjälä wrote: > > On Fri, Nov 04, 2022 at 10:49:39AM +0530, Nautiyal, Ankit K wrote: > >> Patch looks good to me. > >> > >> Minor suggestions inline: > >> > >> On 10/26/2022 5:09 PM, Ville Syrjala wrote: > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> > >>> Currently when opeating in split gamma mode we do the > >> nitpick: 'operating' typo. > >>> "skip ever other sw LUT entry" trick in the low level > >>> LUT programming/readout functions. That is very annoying > >>> and a big hinderance to revamping the color management > >>> uapi. > >>> > >>> Let's get rid of that problem by making half sized copies > >>> of the software LUTs and plugging those into the internal > >>> {pre,post}_csc_lut attachment points (instead of the sticking > >>> the uapi provide sw LUTs there directly). > >>> > >>> With this the low level stuff will operate purely in terms > >>> the hardware LUT sizes, and all uapi nonsense is contained > >>> to the atomic check phase. The one thing we do lose is > >>> intel_color_assert_luts() since we no longer have a way to > >>> check that the uapi LUTs were correctly used when generating > >>> the internal copies. But that seems like a price worth paying. > >>> > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> --- > >>> drivers/gpu/drm/i915/display/intel_color.c | 81 +++++++++++++++++----- > >>> 1 file changed, 64 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > >>> index 33871bfacee7..d48904f90e3a 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_color.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >>> @@ -597,6 +597,30 @@ create_linear_lut(struct drm_i915_private *i915, int lut_size) > >>> return blob; > >>> } > >>> > >>> +static struct drm_property_blob * > >>> +create_resized_lut(struct drm_i915_private *i915, > >>> + const struct drm_property_blob *blob_in, int lut_out_size) > >>> +{ > >>> + int i, lut_in_size = drm_color_lut_size(blob_in); > >>> + struct drm_property_blob *blob_out; > >>> + const struct drm_color_lut *lut_in; > >>> + struct drm_color_lut *lut_out; > >>> + > >>> + blob_out = drm_property_create_blob(&i915->drm, > >>> + sizeof(lut_out[0]) * lut_out_size, > >>> + NULL); > >>> + if (IS_ERR(blob_out)) > >>> + return blob_out; > >>> + > >>> + lut_in = blob_in->data; > >>> + lut_out = blob_out->data; > >>> + > >>> + for (i = 0; i < lut_out_size; i++) > >>> + lut_out[i] = lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)]; > >>> + > >>> + return blob_out; > >>> +} > >>> + > >>> static void i9xx_load_lut_8(struct intel_crtc *crtc, > >>> const struct drm_property_blob *blob) > >>> { > >>> @@ -723,19 +747,14 @@ static void ivb_load_lut_10(struct intel_crtc *crtc, > >>> u32 prec_index) > >>> { > >>> struct drm_i915_private *i915 = to_i915(crtc->base.dev); > >>> - int hw_lut_size = ivb_lut_10_size(prec_index); > >>> const struct drm_color_lut *lut = blob->data; > >>> int i, lut_size = drm_color_lut_size(blob); > >>> enum pipe pipe = crtc->pipe; > >>> > >>> - 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)]; > >>> - > >>> + for (i = 0; i < lut_size; i++) { > >>> intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++); > >>> intel_de_write_fw(i915, PREC_PAL_DATA(pipe), > >>> - ilk_lut_10(entry)); > >>> + ilk_lut_10(&lut[i])); > >>> } > >>> > >>> /* > >>> @@ -751,7 +770,6 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > >>> u32 prec_index) > >>> { > >>> struct drm_i915_private *i915 = to_i915(crtc->base.dev); > >>> - int hw_lut_size = ivb_lut_10_size(prec_index); > >>> const struct drm_color_lut *lut = blob->data; > >>> int i, lut_size = drm_color_lut_size(blob); > >>> enum pipe pipe = crtc->pipe; > >>> @@ -759,14 +777,9 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, > >>> intel_de_write_fw(i915, 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)]; > >>> - > >>> + for (i = 0; i < lut_size; i++) > >>> intel_de_write_fw(i915, PREC_PAL_DATA(pipe), > >>> - ilk_lut_10(entry)); > >>> - } > >>> + ilk_lut_10(&lut[i])); > >>> > >>> /* > >>> * Reset the index, otherwise it prevents the legacy palette to be > >>> @@ -1343,7 +1356,7 @@ void intel_color_assert_luts(const struct intel_crtc_state *crtc_state) > >>> crtc_state->pre_csc_lut != i915->display.color.glk_linear_degamma_lut); > >>> drm_WARN_ON(&i915->drm, > >>> crtc_state->post_csc_lut != crtc_state->hw.gamma_lut); > >>> - } else { > >>> + } else if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { > >>> drm_WARN_ON(&i915->drm, > >>> crtc_state->pre_csc_lut != crtc_state->hw.degamma_lut && > >>> crtc_state->pre_csc_lut != crtc_state->hw.gamma_lut); > >>> @@ -1564,6 +1577,38 @@ static u32 ivb_csc_mode(const struct intel_crtc_state *crtc_state) > >>> return CSC_POSITION_BEFORE_GAMMA; > >>> } > >>> > >>> +static int ivb_assign_luts(struct intel_crtc_state *crtc_state) > >>> +{ > >>> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > >>> + struct drm_property_blob *degamma_lut, *gamma_lut; > >>> + > >>> + if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { > >>> + ilk_assign_luts(crtc_state); > >>> + return 0; > >>> + } > >>> + > >>> + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.degamma_lut) != 1024); > >>> + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.gamma_lut) != 1024); > >> Does it make sense to use some macro for LUT size for split gamma case > >> and regular case? > >> > >> Same thing perhaps can be used in ivb_lut_10_size? > > I don't think macros would be really helpful. I guess I > > could have used ivb_lut_10_size() for the create_resized_lut() > > calls below. And these WARNs I guess could have just used > > device info stuff instead. > > Using ivb_lut_10_size() should be good enough, I think. > > In any case, this is a just a minor suggestion. Patch looks good to me. I've left it as is for now. We can certainly improve this when we return to topic of the color uapi redesign. > > With the small typo fixed in commit message: Doh. Accidentally pulled the trigger before fixing the typo. Oh well. > > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> Thanks for the reviews. Entire series merged now.
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 33871bfacee7..d48904f90e3a 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -597,6 +597,30 @@ create_linear_lut(struct drm_i915_private *i915, int lut_size) return blob; } +static struct drm_property_blob * +create_resized_lut(struct drm_i915_private *i915, + const struct drm_property_blob *blob_in, int lut_out_size) +{ + int i, lut_in_size = drm_color_lut_size(blob_in); + struct drm_property_blob *blob_out; + const struct drm_color_lut *lut_in; + struct drm_color_lut *lut_out; + + blob_out = drm_property_create_blob(&i915->drm, + sizeof(lut_out[0]) * lut_out_size, + NULL); + if (IS_ERR(blob_out)) + return blob_out; + + lut_in = blob_in->data; + lut_out = blob_out->data; + + for (i = 0; i < lut_out_size; i++) + lut_out[i] = lut_in[i * (lut_in_size - 1) / (lut_out_size - 1)]; + + return blob_out; +} + static void i9xx_load_lut_8(struct intel_crtc *crtc, const struct drm_property_blob *blob) { @@ -723,19 +747,14 @@ static void ivb_load_lut_10(struct intel_crtc *crtc, u32 prec_index) { struct drm_i915_private *i915 = to_i915(crtc->base.dev); - int hw_lut_size = ivb_lut_10_size(prec_index); const struct drm_color_lut *lut = blob->data; int i, lut_size = drm_color_lut_size(blob); enum pipe pipe = crtc->pipe; - 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)]; - + for (i = 0; i < lut_size; i++) { intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++); intel_de_write_fw(i915, PREC_PAL_DATA(pipe), - ilk_lut_10(entry)); + ilk_lut_10(&lut[i])); } /* @@ -751,7 +770,6 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, u32 prec_index) { struct drm_i915_private *i915 = to_i915(crtc->base.dev); - int hw_lut_size = ivb_lut_10_size(prec_index); const struct drm_color_lut *lut = blob->data; int i, lut_size = drm_color_lut_size(blob); enum pipe pipe = crtc->pipe; @@ -759,14 +777,9 @@ static void bdw_load_lut_10(struct intel_crtc *crtc, intel_de_write_fw(i915, 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)]; - + for (i = 0; i < lut_size; i++) intel_de_write_fw(i915, PREC_PAL_DATA(pipe), - ilk_lut_10(entry)); - } + ilk_lut_10(&lut[i])); /* * Reset the index, otherwise it prevents the legacy palette to be @@ -1343,7 +1356,7 @@ void intel_color_assert_luts(const struct intel_crtc_state *crtc_state) crtc_state->pre_csc_lut != i915->display.color.glk_linear_degamma_lut); drm_WARN_ON(&i915->drm, crtc_state->post_csc_lut != crtc_state->hw.gamma_lut); - } else { + } else if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { drm_WARN_ON(&i915->drm, crtc_state->pre_csc_lut != crtc_state->hw.degamma_lut && crtc_state->pre_csc_lut != crtc_state->hw.gamma_lut); @@ -1564,6 +1577,38 @@ static u32 ivb_csc_mode(const struct intel_crtc_state *crtc_state) return CSC_POSITION_BEFORE_GAMMA; } +static int ivb_assign_luts(struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); + struct drm_property_blob *degamma_lut, *gamma_lut; + + if (crtc_state->gamma_mode != GAMMA_MODE_MODE_SPLIT) { + ilk_assign_luts(crtc_state); + return 0; + } + + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.degamma_lut) != 1024); + drm_WARN_ON(&i915->drm, drm_color_lut_size(crtc_state->hw.gamma_lut) != 1024); + + degamma_lut = create_resized_lut(i915, crtc_state->hw.degamma_lut, 512); + if (IS_ERR(degamma_lut)) + return PTR_ERR(degamma_lut); + + gamma_lut = create_resized_lut(i915, crtc_state->hw.gamma_lut, 512); + if (IS_ERR(gamma_lut)) { + drm_property_blob_put(degamma_lut); + return PTR_ERR(gamma_lut); + } + + drm_property_replace_blob(&crtc_state->pre_csc_lut, degamma_lut); + drm_property_replace_blob(&crtc_state->post_csc_lut, gamma_lut); + + drm_property_blob_put(degamma_lut); + drm_property_blob_put(gamma_lut); + + return 0; +} + static int ivb_color_check(struct intel_crtc_state *crtc_state) { struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); @@ -1599,7 +1644,9 @@ static int ivb_color_check(struct intel_crtc_state *crtc_state) if (ret) return ret; - ilk_assign_luts(crtc_state); + ret = ivb_assign_luts(crtc_state); + if (ret) + return ret; crtc_state->preload_luts = intel_can_preload_luts(crtc_state);