From: "Sharma, Swati2" <swati2.sharma@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: jani.nikula@intel.com, daniel.vetter@ffwll.ch,
intel-gfx@lists.freedesktop.org, ankit.k.nautiyal@intel.com
Subject: Re: [PATCH 3/4] [v5] drm/i915/color: Extract icl_read_luts()
Date: Thu, 10 Oct 2019 16:20:04 +0530 [thread overview]
Message-ID: <a5ee3865-24e2-0f87-24be-577e66e80fe1@intel.com> (raw)
In-Reply-To: <20191009141611.GB1208@intel.com>
On 09-Oct-19 7:46 PM, Ville Syrjälä wrote:
> On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote:
>> For icl+, have hw read out to create hw blob of gamma
>> lut values. icl+ platforms supports multi segmented gamma
>> mode by default, add hw lut creation for this mode.
>>
>> This will be used to validate gamma programming using dsb
>> (display state buffer) which is a tgl specific feature.
>>
>> Major change done-removal of readouts of coarse and fine segments
>> because PAL_PREC_DATA register isn't giving propoer values.
>> State checker limited only to "fine segment"
>>
>> v2: -readout code for multisegmented gamma has to come
>> up with some intermediate entries that aren't preserved
>> in hardware (Jani N)
>> -linear interpolation (Ville)
>> -moved common code to check gamma_enable to specific funcs,
>> since icl doesn't support that
>> v3: -use u16 instead of __u16 [Jani N]
>> -used single lut [Jani N]
>> -improved and more readable for loops [Jani N]
>> -read values directly to actual locations and then fill gaps [Jani N]
>> -moved cleaning to patch 1 [Jani N]
>> -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to
>> make it similar to icl_load_luts()
>> -renamed icl_compute_interpolated_gamma_blob() to
>> icl_compute_interpolated_gamma_lut_values() more sensible, I guess
>> v4: -removed interpolated func for creating gamma lut values
>> -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA
>> correctly
>> v5: -added gamma_enable check inside read_luts()
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++---
>> drivers/gpu/drm/i915/i915_reg.h | 6 ++
>> 2 files changed, 108 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index fa44eb73d088..614e0ad386ca 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
>> }
>> }
>>
>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
>> + return 0;
>> +
>> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>> + case GAMMA_MODE_MODE_8BIT:
>> + return 8;
>> + case GAMMA_MODE_MODE_10BIT:
>> + return 10;
>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> + return 16;
>> + default:
>> + MISSING_CASE(crtc_state->gamma_mode);
>> + return 0;
>> + }
>> +
>> +}
>> +
>> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state)
>> {
>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
>> else
>> return i9xx_gamma_precision(crtc_state);
>> } else {
>> - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>> + if (INTEL_GEN(dev_priv) >= 11)
>> + return icl_gamma_precision(crtc_state);
>> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>> return glk_gamma_precision(crtc_state);
>> else if (IS_IRONLAKE(dev_priv))
>> return ilk_gamma_precision(crtc_state);
>> @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1,
>> return true;
>> }
>>
>> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1,
>> + struct drm_color_lut *lut2,
>> + int lut_size, u32 err)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < 9; i++) {
>> + if (!err_check(&lut1[i], &lut2[i], err))
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> struct drm_property_blob *blob2,
>> u32 gamma_mode, u32 bit_precision)
>> @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> lut_size2 = drm_color_lut_size(blob2);
>>
>> /* check sw and hw lut size */
>> - switch (gamma_mode) {
>> - case GAMMA_MODE_MODE_8BIT:
>> - case GAMMA_MODE_MODE_10BIT:
>> - if (lut_size1 != lut_size2)
>> - return false;
>> - break;
>> - default:
>> - MISSING_CASE(gamma_mode);
>> - return false;
>> - }
>> + if (lut_size1 != lut_size2)
>> + return false;
>>
>> lut1 = blob1->data;
>> lut2 = blob2->data;
>> @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> err = 0xffff >> bit_precision;
>>
>> /* check sw and hw lut entry to be equal */
>> - switch (gamma_mode) {
>> + switch (gamma_mode & GAMMA_MODE_MODE_MASK) {
>> case GAMMA_MODE_MODE_8BIT:
>> case GAMMA_MODE_MODE_10BIT:
>> if (!intel_color_lut_entry_equal(lut1, lut2,
>> lut_size2, err))
>> return false;
>> break;
>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> + if (!intel_color_lut_entry_multi_equal(lut1, lut2,
>> + lut_size2, err))
>
> I don't think you need a new function for that. Just pass 9 as the size
> to intel_color_lut_entry_equal() ?
I had made a separate function for multi-segmented gamma since there
will be 3 loops for comparing superfine, fine and course segments which
wont go with intel_lut_entry_equal() structure.
Right now we are limiting to superfine segment only but in future we
will add for other segments too (once we get fix from h/w)
Func() should look like this. Actually there is no need to passing
lut_size only in this function if we continue with this function only.
+static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1,
+ struct drm_color_lut *lut2,
+ int lut_size, u32 err)
+{
+ int i;
+
+ for (i = 0; i < 9; i++) {
+ if (!err_check(&lut1[i], &lut2[i], err))
+ return false;
+ }
+
+ for (i = 1; i < 257; i++) {
+ if (!err_check(&lut1[i * 8], &lut2[i * 8], err))
+ return false;
+ }
+
+ for (i = 0; i < 256; i++) {
+ if (!err_check(&lut1[i * 8 * 128], &lut2[i * 8 * 128], err))
+ return false;
+ }
+
+ return true;
+}
+
Please suggest.
>
> Hmm, should probably rename that to just intel_color_lut_equal() since
> it checks the entire LUT (or at least the specified subset) and not
> just a single entry...
This will be fine for this segment but for other two segments it won't
work. Right?
>
>> + return false;
>> + break;
>> default:
>> MISSING_CASE(gamma_mode);
>> return false;
>> @@ -1815,6 +1847,63 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state)
>> crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
>> }
>>
>> +static struct drm_property_blob *
>> +icl_read_lut_multi_segment(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);
>> + int lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>> + enum pipe pipe = crtc->pipe;
>> + struct drm_property_blob *blob;
>> + struct drm_color_lut *blob_data;
>> + u32 i, val1, val2;
>> +
>> + blob = drm_property_create_blob(&dev_priv->drm,
>> + sizeof(struct drm_color_lut) * lut_size,
>> + NULL);
>> + if (IS_ERR(blob))
>> + return NULL;
>> +
>> + blob_data = blob->data;
>> +
>> + I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> +
>> + for (i = 0; i < 9; i++) {
>> + val1 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe));
>> + val2 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe));
>> +
>> + blob_data[i].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2) << 6 |
>> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1);
>> + blob_data[i].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2) << 6 |
>> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1);
>> + blob_data[i].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2) << 6 |
>> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1);
>> + }
>> +
>> + /*
>> + * FIXME readouts from PAL_PREC_DATA register aren't giving correct values
>> + * in the case of fine and coarse segments. Restricting readouts only for
>> + * super fine segment as of now.
>> + */
>> +
>> + return blob;
>> +}
>> +
>> +static void icl_read_luts(struct intel_crtc_state *crtc_state)
>> +{
>> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
>> + return;
>> +
>> + if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
>> + GAMMA_MODE_MODE_8BIT)
>
> Please make this a switch statement.
>
> I pushed patches 1-2 to dinq. Thanks.
>
>> + crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
>> + else if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
>> + GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED)
>> + crtc_state->base.gamma_lut = icl_read_lut_multi_segment(crtc_state);
>> + else
>> + crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
>> +}
>> +
>> void intel_color_init(struct intel_crtc *crtc)
>> {
>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> @@ -1858,6 +1947,7 @@ void intel_color_init(struct intel_crtc *crtc)
>>
>> if (INTEL_GEN(dev_priv) >= 11) {
>> dev_priv->display.load_luts = icl_load_luts;
>> + dev_priv->display.read_luts = icl_read_luts;
>> } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
>> dev_priv->display.load_luts = glk_load_luts;
>> dev_priv->display.read_luts = glk_read_luts;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 1dc067fc57ab..06b6398b7d62 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -10584,6 +10584,12 @@ enum skl_power_gate {
>>
>> #define _PAL_PREC_MULTI_SEG_DATA_A 0x4A40C
>> #define _PAL_PREC_MULTI_SEG_DATA_B 0x4AC0C
>> +#define PAL_PREC_MULTI_SEG_RED_LDW_MASK REG_GENMASK(29, 24)
>> +#define PAL_PREC_MULTI_SEG_RED_UDW_MASK REG_GENMASK(29, 20)
>> +#define PAL_PREC_MULTI_SEG_GREEN_LDW_MASK REG_GENMASK(19, 14)
>> +#define PAL_PREC_MULTI_SEG_GREEN_UDW_MASK REG_GENMASK(19, 10)
>> +#define PAL_PREC_MULTI_SEG_BLUE_LDW_MASK REG_GENMASK(9, 4)
>> +#define PAL_PREC_MULTI_SEG_BLUE_UDW_MASK REG_GENMASK(9, 0)
>>
>> #define PREC_PAL_MULTI_SEG_INDEX(pipe) _MMIO_PIPE(pipe, \
>> _PAL_PREC_MULTI_SEG_INDEX_A, \
>> --
>> 2.23.0
>
--
~Swati Sharma
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-10-10 10:50 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 6:55 [PATCH 0/4] fix broken state checker and enable state checker for icl+ Swati Sharma
2019-10-09 6:55 ` [PATCH 1/4] [v3] drm/i915/color: fix broken gamma state-checker during boot Swati Sharma
2019-10-09 6:55 ` [PATCH 2/4] [v2] drm/i915/color: move check of gamma_enable to specific func/platform Swati Sharma
2019-10-09 6:55 ` [PATCH 3/4] [v5] drm/i915/color: Extract icl_read_luts() Swati Sharma
2019-10-09 14:16 ` Ville Syrjälä
2019-10-10 10:50 ` Sharma, Swati2 [this message]
2019-10-15 12:34 ` Ville Syrjälä
2019-10-15 14:04 ` Sharma, Swati2
2019-10-15 14:53 ` Ville Syrjälä
2019-10-15 7:59 ` Jani Nikula
2019-10-09 6:55 ` [PATCH 4/4] FOR_TESTING_ONLY: Print rgb values of hw and sw blobs Swati Sharma
2019-10-09 7:47 ` ✗ Fi.CI.CHECKPATCH: warning for fix broken state checker and enable state checker for icl+ (rev2) Patchwork
2019-10-09 8:10 ` ✓ Fi.CI.BAT: success " Patchwork
2019-10-09 11:57 ` ✓ Fi.CI.IGT: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2019-10-04 8:26 [PATCH 0/4] fix broken state checker and enable state checker for icl+ Swati Sharma
2019-10-04 8:26 ` [PATCH 3/4] [v5] drm/i915/color: Extract icl_read_luts() Swati Sharma
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a5ee3865-24e2-0f87-24be-577e66e80fe1@intel.com \
--to=swati2.sharma@intel.com \
--cc=ankit.k.nautiyal@intel.com \
--cc=daniel.vetter@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.