Re: [PATCH 11/11] drm/i915: Create resized LUTs for ivb+ split gamma mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx>
---
   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@xxxxxxxxx>


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.




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux