On Thu, Mar 28, 2019 at 01:56:18PM -0700, Matt Roper wrote:
> On Thu, Mar 28, 2019 at 12:03:48PM +0530, Swati Sharma wrote:
> > Added state checker to validate gamma_lut values. This
> > reads hardware state, and compares the originally requested
> > state to the state read from hardware.
> >
> > v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani)
> > -Added inverse function of drm_color_lut_extract to convert hardware
> > read values back to user values (code written by Jani)
> > -Renamed get_config() to color_config() (Jani)
> > -Placed all platform specific shifts and masks in i915_reg.h (Jani)
> > -Renamed i9xx_get_config to i9xx_color_config and all related
> > functions (Jani)
> > -Removed debug logs from compare function (Jani)
> > -Renamed intel_compare_blob to intel_compare_lut and added platform specific
> > bit precision of the readout into the function (Jani)
> > -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani)
> > -Added check if blobs can be NULL (Jani)
> > -Added function in intel_color.c that returns the bit precision (Jani),
> > didn't add in device info since its gonna die soon (Ville)
> >
> > TODO:
> > -Add a separate function to log errors at the higher level
> > -Haven't moved intel_compare_lut() from intel_display.c to intel_color.c
> > Since all the comparison functions are placed in intel_display, isn't
> > it the right place (or) we want to move to consolidate color related functions
> > together? Opinion? Please correct me if I am wrong.
> > -Optimizations and refractoring
> >
> > Signed-off-by: Swati Sharma <swati2.sharma@xxxxxxxxx>
>
> I agree with Jani's feedback and have a couple other comments inline below.
>
> Also, since I don't see it on the TODO list here, do you intend to also
> readout and compare degamma and CTM eventually?
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_reg.h | 12 +++
> > drivers/gpu/drm/i915/intel_color.c | 186 +++++++++++++++++++++++++++++++++--
> > drivers/gpu/drm/i915/intel_display.c | 48 +++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 2 +
> > 5 files changed, 243 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c4ffe19..b422ea6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -334,6 +334,7 @@ struct drm_i915_display_funcs {
> > * involved with the same commit.
> > */
> > void (*load_luts)(const struct intel_crtc_state *crtc_state);
> > + void (*color_config)(struct intel_crtc_state *crtc_state);
> > };
> >
> > #define CSR_VERSION(major, minor) ((major) << 16 | (minor))
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c0cd7a8..2813033 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7156,6 +7156,10 @@ enum {
> > #define _LGC_PALETTE_B 0x4a800
> > #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
> >
> > +#define LGC_PALETTE_RED_MASK (0xFF << 16)
> > +#define LGC_PALETTE_GREEN_MASK (0xFF << 8)
> > +#define LGC_PALETTE_BLUE_MASK (0xFF << 0)
> > +
> > #define _GAMMA_MODE_A 0x4a480
> > #define _GAMMA_MODE_B 0x4ac80
> > #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
> > @@ -10102,6 +10106,10 @@ enum skl_power_gate {
> > #define PRE_CSC_GAMC_INDEX(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
> > #define PRE_CSC_GAMC_DATA(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
> >
> > +#define PREC_PAL_DATA_RED_MASK (0x3FF << 20)
> > +#define PREC_PAL_DATA_GREEN_MASK (0x3FF << 10)
> > +#define PREC_PAL_DATA_BLUE_MASK (0x3FF << 0)
> > +
> > /* pipe CSC & degamma/gamma LUTs on CHV */
> > #define _CGM_PIPE_A_CSC_COEFF01 (VLV_DISPLAY_BASE + 0x67900)
> > #define _CGM_PIPE_A_CSC_COEFF23 (VLV_DISPLAY_BASE + 0x67904)
> > @@ -10133,6 +10141,10 @@ enum skl_power_gate {
> > #define CGM_PIPE_GAMMA(pipe, i, w) _MMIO(_PIPE(pipe, _CGM_PIPE_A_GAMMA, _CGM_PIPE_B_GAMMA) + (i) * 8 + (w) * 4)
> > #define CGM_PIPE_MODE(pipe) _MMIO_PIPE(pipe, _CGM_PIPE_A_MODE, _CGM_PIPE_B_MODE)
> >
> > +#define CGM_PIPE_GAMMA_RED_MASK (0x3FF << 0)
> > +#define CGM_PIPE_GAMMA_GREEN_MASK (0x3FF << 16)
> > +#define CGM_PIPE_GAMMA_BLUE_MASK (0x3FF << 0)
> > +
> > /* MIPI DSI registers */
> >
> > #define _MIPI_PORT(port, a, c) (((port) == PORT_A) ? a : c) /* ports A and C only */
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index da7a07d..bd4f1b1 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -679,6 +679,172 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
> > dev_priv->display.load_luts(crtc_state);
> > }
> >
> > +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv)
> > +{
> > + if (INTEL_GEN(dev_priv) >= 9)
> > + return 10;
> > + else
> > + return 8;
> > +}
>
> Doesn't bdw (gen8) also use a 10 bit palette (PREC_PAL_DATA)?
>
> I think we probably want to figure out the number of bits to compare
> based on gamma_mode rather than the platform we're running on. E.g., a
> platform with 10-bit precision palettes might still be using legacy
> tables with 8 bits of precision in some cases. And some platforms have
> even more potential gamma modes that we might enable in the future too.
We'll need to look at .gamma_mode, .gamma_enable (on pre-icl), and
.cgm_mode (on chv). Oh and .c8_planes too I suppose. Unfortunately
that's a bit of a problem since we have no real plane readout code
at the moment. Hmm, I guess we can ignore .c8_planes (and .gamma_enable
too I suppose) and just blindly read out the legacy LUT whenever
.gamma_mode says so, but we'll have to be prepared for the case where
the software state had no gamma LUT at all (since we program
.gamma_mode to 8bit in that case too).
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
