| Message ID | 1546933053-10731-6-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Gen 11 pipe color features | expand |
On Tue, Jan 08, 2019 at 01:07:32PM +0530, Uma Shankar wrote: > Enable ICL pipe csc hardware. CSC block is enabled > in CSC_MODE register instead of PLANE_COLOR_CTL. > > ToDO: Extend the ABI to accept 32 bit coefficient values > instead of 16bit for future platforms. > > v2: Addressed Maarten's review comments. > > v3: Addressed Matt's review comments. Removed rmw patterns > as suggested by Matt. > > v4: Addressed Matt's review comments. > > v5: Addressed Ville's review comments. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 10 +++++++--- > drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++-- > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index f29eef7..5a262c0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9861,10 +9861,14 @@ enum skl_power_gate { > #define _PIPE_A_CSC_COEFF_BU 0x4901c > #define _PIPE_A_CSC_COEFF_RV_GV 0x49020 > #define _PIPE_A_CSC_COEFF_BV 0x49024 > + > #define _PIPE_A_CSC_MODE 0x49028 > -#define CSC_BLACK_SCREEN_OFFSET (1 << 2) > -#define CSC_POSITION_BEFORE_GAMMA (1 << 1) > -#define CSC_MODE_YUV_TO_RGB (1 << 0) > +#define ICL_CSC_ENABLE (1 << 31) > +#define ICL_OUTPUT_CSC_ENABLE (1 << 30) > +#define CSC_BLACK_SCREEN_OFFSET (1 << 2) > +#define CSC_POSITION_BEFORE_GAMMA (1 << 1) > +#define CSC_MODE_YUV_TO_RGB (1 << 0) > + > #define _PIPE_A_CSC_PREOFF_HI 0x49030 > #define _PIPE_A_CSC_PREOFF_ME 0x49034 > #define _PIPE_A_CSC_PREOFF_LO 0x49038 > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index 9cd4646..c3e4ff6 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -134,7 +134,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc) > I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); > I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); > I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); > - I915_WRITE(PIPE_CSC_MODE(pipe), 0); > + > + if (INTEL_GEN(dev_priv) >= 11) > + I915_WRITE(PIPE_CSC_MODE(pipe), ICL_OUTPUT_CSC_ENABLE); For gen11+, shouldn't we be programming OUTPUT_CSC_COEFF instead of CSC_COEFF if we set this bit? > + else > + I915_WRITE(PIPE_CSC_MODE(pipe), 0); > } > > static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state) > @@ -242,7 +246,10 @@ static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state) > I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff); > I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff); > > - I915_WRITE(PIPE_CSC_MODE(pipe), 0); > + if (INTEL_GEN(dev_priv) >= 11) > + I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE); > + else > + I915_WRITE(PIPE_CSC_MODE(pipe), 0); I might be misinterpreting how the hardware works, but my impression was that we had two distinct CSC units now on gen11+. So we could use the traditional CSC registers to hold the userspace-provided CTM and the new output CSC registers to hold the fixed RGB->YUV matrix, and they could both be used at the same time if necessary. It looks like this function is still assuming that the two are mutually exclusive and that if we need RGB->YUV we never bother programming the userspace matrix. Is that intentional or an oversight? Aside from the register definitions themselves, the bspec seems to be pretty sparse on how the whole pipeline goes together... Matt > } else { > uint32_t mode = CSC_MODE_YUV_TO_RGB; > > @@ -700,6 +707,7 @@ void intel_color_init(struct intel_crtc *crtc) > dev_priv->display.load_csc_matrix = ilk_load_csc_matrix; > dev_priv->display.load_luts = glk_load_luts; > } else if (IS_ICELAKE(dev_priv)) { > + dev_priv->display.load_csc_matrix = ilk_load_csc_matrix; > dev_priv->display.load_luts = icl_load_luts; > } else { > dev_priv->display.load_luts = i9xx_load_luts; > -- > 1.9.1 >
>-----Original Message----- >From: Roper, Matthew D >Sent: Saturday, January 12, 2019 4:30 AM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma, >Shashank <shashank.sharma@intel.com> >Subject: Re: [v5 5/6] drm/i915/icl: Enable ICL Pipe CSC block > >On Tue, Jan 08, 2019 at 01:07:32PM +0530, Uma Shankar wrote: >> Enable ICL pipe csc hardware. CSC block is enabled in CSC_MODE >> register instead of PLANE_COLOR_CTL. >> >> ToDO: Extend the ABI to accept 32 bit coefficient values instead of >> 16bit for future platforms. >> >> v2: Addressed Maarten's review comments. >> >> v3: Addressed Matt's review comments. Removed rmw patterns as >> suggested by Matt. >> >> v4: Addressed Matt's review comments. >> >> v5: Addressed Ville's review comments. >> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 10 +++++++--- >> drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++-- >> 2 files changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h index f29eef7..5a262c0 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -9861,10 +9861,14 @@ enum skl_power_gate { >> #define _PIPE_A_CSC_COEFF_BU 0x4901c >> #define _PIPE_A_CSC_COEFF_RV_GV 0x49020 >> #define _PIPE_A_CSC_COEFF_BV 0x49024 >> + >> #define _PIPE_A_CSC_MODE 0x49028 >> -#define CSC_BLACK_SCREEN_OFFSET (1 << 2) >> -#define CSC_POSITION_BEFORE_GAMMA (1 << 1) >> -#define CSC_MODE_YUV_TO_RGB (1 << 0) >> +#define ICL_CSC_ENABLE (1 << 31) >> +#define ICL_OUTPUT_CSC_ENABLE (1 << 30) >> +#define CSC_BLACK_SCREEN_OFFSET (1 << 2) >> +#define CSC_POSITION_BEFORE_GAMMA (1 << 1) >> +#define CSC_MODE_YUV_TO_RGB (1 << 0) >> + >> #define _PIPE_A_CSC_PREOFF_HI 0x49030 >> #define _PIPE_A_CSC_PREOFF_ME 0x49034 >> #define _PIPE_A_CSC_PREOFF_LO 0x49038 >> diff --git a/drivers/gpu/drm/i915/intel_color.c >> b/drivers/gpu/drm/i915/intel_color.c >> index 9cd4646..c3e4ff6 100644 >> --- a/drivers/gpu/drm/i915/intel_color.c >> +++ b/drivers/gpu/drm/i915/intel_color.c >> @@ -134,7 +134,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct >intel_crtc *crtc) >> I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); >> I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), >POSTOFF_RGB_TO_YUV_ME); >> I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); >> - I915_WRITE(PIPE_CSC_MODE(pipe), 0); >> + >> + if (INTEL_GEN(dev_priv) >= 11) >> + I915_WRITE(PIPE_CSC_MODE(pipe), >ICL_OUTPUT_CSC_ENABLE); > >For gen11+, shouldn't we be programming OUTPUT_CSC_COEFF instead of >CSC_COEFF if we set this bit? Yeah you are right, ideally output CSC coeff should be programmed to utilize this. Will add that support. >> + else >> + I915_WRITE(PIPE_CSC_MODE(pipe), 0); >> } >> >> static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state) >> @@ -242,7 +246,10 @@ static void ilk_load_csc_matrix(struct intel_crtc_state >*crtc_state) >> I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff); >> I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff); >> >> - I915_WRITE(PIPE_CSC_MODE(pipe), 0); >> + if (INTEL_GEN(dev_priv) >= 11) >> + I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE); >> + else >> + I915_WRITE(PIPE_CSC_MODE(pipe), 0); > >I might be misinterpreting how the hardware works, but my impression was that >we had two distinct CSC units now on gen11+. So we could use the traditional >CSC registers to hold the userspace-provided CTM and the new output CSC >registers to hold the fixed RGB->YUV matrix, and they could both be used at the >same time if necessary. It looks like this function is still assuming that the two are >mutually exclusive and that if we need RGB->YUV we never bother programming >the userspace matrix. Is that intentional or an oversight? Aside from the register >definitions themselves, the bspec seems to be pretty sparse on how the whole >pipeline goes together... Ideally both CSC blocks can co-exist, current design is indeed limiting this. Will modify to handle this properly. Regards, Uma Shankar > >Matt > >> } else { >> uint32_t mode = CSC_MODE_YUV_TO_RGB; >> >> @@ -700,6 +707,7 @@ void intel_color_init(struct intel_crtc *crtc) >> dev_priv->display.load_csc_matrix = ilk_load_csc_matrix; >> dev_priv->display.load_luts = glk_load_luts; >> } else if (IS_ICELAKE(dev_priv)) { >> + dev_priv->display.load_csc_matrix = ilk_load_csc_matrix; >> dev_priv->display.load_luts = icl_load_luts; >> } else { >> dev_priv->display.load_luts = i9xx_load_luts; >> -- >> 1.9.1 >> > >-- >Matt Roper >Graphics Software Engineer >IoTG Platform Enabling & Development >Intel Corporation >(916) 356-2795
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f29eef7..5a262c0 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9861,10 +9861,14 @@ enum skl_power_gate { #define _PIPE_A_CSC_COEFF_BU 0x4901c #define _PIPE_A_CSC_COEFF_RV_GV 0x49020 #define _PIPE_A_CSC_COEFF_BV 0x49024 + #define _PIPE_A_CSC_MODE 0x49028 -#define CSC_BLACK_SCREEN_OFFSET (1 << 2) -#define CSC_POSITION_BEFORE_GAMMA (1 << 1) -#define CSC_MODE_YUV_TO_RGB (1 << 0) +#define ICL_CSC_ENABLE (1 << 31) +#define ICL_OUTPUT_CSC_ENABLE (1 << 30) +#define CSC_BLACK_SCREEN_OFFSET (1 << 2) +#define CSC_POSITION_BEFORE_GAMMA (1 << 1) +#define CSC_MODE_YUV_TO_RGB (1 << 0) + #define _PIPE_A_CSC_PREOFF_HI 0x49030 #define _PIPE_A_CSC_PREOFF_ME 0x49034 #define _PIPE_A_CSC_PREOFF_LO 0x49038 diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 9cd4646..c3e4ff6 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -134,7 +134,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc) I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); - I915_WRITE(PIPE_CSC_MODE(pipe), 0); + + if (INTEL_GEN(dev_priv) >= 11) + I915_WRITE(PIPE_CSC_MODE(pipe), ICL_OUTPUT_CSC_ENABLE); + else + I915_WRITE(PIPE_CSC_MODE(pipe), 0); } static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state) @@ -242,7 +246,10 @@ static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state) I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff); I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff); - I915_WRITE(PIPE_CSC_MODE(pipe), 0); + if (INTEL_GEN(dev_priv) >= 11) + I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE); + else + I915_WRITE(PIPE_CSC_MODE(pipe), 0); } else { uint32_t mode = CSC_MODE_YUV_TO_RGB; @@ -700,6 +707,7 @@ void intel_color_init(struct intel_crtc *crtc) dev_priv->display.load_csc_matrix = ilk_load_csc_matrix; dev_priv->display.load_luts = glk_load_luts; } else if (IS_ICELAKE(dev_priv)) { + dev_priv->display.load_csc_matrix = ilk_load_csc_matrix; dev_priv->display.load_luts = icl_load_luts; } else { dev_priv->display.load_luts = i9xx_load_luts;
Enable ICL pipe csc hardware. CSC block is enabled in CSC_MODE register instead of PLANE_COLOR_CTL. ToDO: Extend the ABI to accept 32 bit coefficient values instead of 16bit for future platforms. v2: Addressed Maarten's review comments. v3: Addressed Matt's review comments. Removed rmw patterns as suggested by Matt. v4: Addressed Matt's review comments. v5: Addressed Ville's review comments. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 10 +++++++--- drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++-- 2 files changed, 17 insertions(+), 5 deletions(-)