| Message ID | 20190215233541.16445-1-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: Fix checks for userspace ctm + ycbcr output | expand |
>-----Original Message----- >From: Roper, Matthew D >Sent: Saturday, February 16, 2019 5:06 AM >To: intel-gfx@lists.freedesktop.org >Cc: Roper, Matthew D <matthew.d.roper@intel.com>; Shankar, Uma ><uma.shankar@intel.com>; Maarten Lankhorst ><maarten.lankhorst@linux.intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com> >Subject: [PATCH] drm/i915/icl: Fix checks for userspace ctm + ycbcr output > >We recently added support for gen11's new "output csc" which can be used >independently of the regular pipe CSC. The idea is that this new output csc allows us >to use a userspace-provided color transformation matrix at the same time we drive a >YCBCR display mode requiring RGB->YUV conversion. However when landing that >support we only updated the color management code and overlooked that there was >an additional atomic check in the modeset path (in intel_crtc_compute_config()) that >still assumes we only have a single CSC unit to work with. Let's update that check to >only apply to pre-gen11 platforms and move it into the color management code to >ensure it gets called on all commits, not just modesets. > >Also, if we're *only* using the output CSC and not the pipe CSC, there's no need to set >crtc_state->csc_enable, so let's also not consider the output format when setting >csc_enable on gen11+ platforms. This looks good to me. Thanks for fixing this. Reviewed-by: Uma Shankar <uma.shankar@intel.com> >Fixes: a91de580541c ("drm/i915/icl: Enable pipe output csc") >Cc: Uma Shankar <uma.shankar@intel.com> >Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >--- >It might also be worth moving the full->limited range RGB conversion from the pipe >CSC to the output CSC on gen11 at some point. > > drivers/gpu/drm/i915/intel_color.c | 16 +++++++++++++--- > drivers/gpu/drm/i915/intel_display.c | 12 ------------ > 2 files changed, 13 insertions(+), 15 deletions(-) > >diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c >index da7a07d5ccea..b47e6d1aaaec 100644 >--- a/drivers/gpu/drm/i915/intel_color.c >+++ b/drivers/gpu/drm/i915/intel_color.c >@@ -780,9 +780,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) > limited_color_range = crtc_state->limited_color_range; > >- crtc_state->csc_enable = >- crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || >- crtc_state->base.ctm || limited_color_range; >+ crtc_state->csc_enable = crtc_state->base.ctm || limited_color_range; >+ if (INTEL_GEN(dev_priv) < 11) >+ crtc_state->csc_enable |= >+ crtc_state->output_format != >INTEL_OUTPUT_FORMAT_RGB; > > ret = intel_color_add_affected_planes(crtc_state); > if (ret) >@@ -822,6 +823,15 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > crtc_state->csc_mode |= ICL_CSC_ENABLE; >+ } else if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB && >+ crtc_state->base.ctm) { >+ /* >+ * There is only one pipe CSC unit per pipe on pre-gen11, and >+ * we need that for output conversion from RGB->YCBCR. So if >+ * CTM is already applied we can't support YCBCR output. >+ */ >+ DRM_DEBUG_KMS("YCBCR420 and CTM together are not >possible\n"); >+ return -EINVAL; > } > > return 0; >diff --git a/drivers/gpu/drm/i915/intel_display.c >b/drivers/gpu/drm/i915/intel_display.c >index afa21daaae51..81bfdbd99092 100644 >--- a/drivers/gpu/drm/i915/intel_display.c >+++ b/drivers/gpu/drm/i915/intel_display.c >@@ -6855,18 +6855,6 @@ static int intel_crtc_compute_config(struct intel_crtc >*crtc, > return -EINVAL; > } > >- if ((pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 || >- pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) && >- pipe_config->base.ctm) { >- /* >- * There is only one pipe CSC unit per pipe, and we need that >- * for output conversion from RGB->YCBCR. So if CTM is already >- * applied we can't support YCBCR420 output. >- */ >- DRM_DEBUG_KMS("YCBCR420 and CTM together are not >possible\n"); >- return -EINVAL; >- } >- > /* > * Pipe horizontal size must be even in: > * - DVO ganged mode >-- >2.14.5
On Fri, Feb 15, 2019 at 03:35:41PM -0800, Matt Roper wrote: > We recently added support for gen11's new "output csc" which can be used > independently of the regular pipe CSC. The idea is that this new output > csc allows us to use a userspace-provided color transformation matrix at > the same time we drive a YCBCR display mode requiring RGB->YUV > conversion. However when landing that support we only updated the color > management code and overlooked that there was an additional atomic check > in the modeset path (in intel_crtc_compute_config()) that still assumes > we only have a single CSC unit to work with. Let's update that check to > only apply to pre-gen11 platforms and move it into the color management > code to ensure it gets called on all commits, not just modesets. > > Also, if we're *only* using the output CSC and not the pipe CSC, there's > no need to set crtc_state->csc_enable, so let's also not consider the > output format when setting csc_enable on gen11+ platforms. > > Fixes: a91de580541c ("drm/i915/icl: Enable pipe output csc") > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > It might also be worth moving the full->limited range RGB conversion > from the pipe CSC to the output CSC on gen11 at some point. > > drivers/gpu/drm/i915/intel_color.c | 16 +++++++++++++--- > drivers/gpu/drm/i915/intel_display.c | 12 ------------ > 2 files changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index da7a07d5ccea..b47e6d1aaaec 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -780,9 +780,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) > limited_color_range = crtc_state->limited_color_range; > > - crtc_state->csc_enable = > - crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > - crtc_state->base.ctm || limited_color_range; > + crtc_state->csc_enable = crtc_state->base.ctm || limited_color_range; > + if (INTEL_GEN(dev_priv) < 11) > + crtc_state->csc_enable |= > + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB; > > ret = intel_color_add_affected_planes(crtc_state); > if (ret) > @@ -822,6 +823,15 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > crtc_state->csc_mode |= ICL_CSC_ENABLE; > + } else if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB && > + crtc_state->base.ctm) { > + /* > + * There is only one pipe CSC unit per pipe on pre-gen11, and > + * we need that for output conversion from RGB->YCBCR. So if > + * CTM is already applied we can't support YCBCR output. > + */ > + DRM_DEBUG_KMS("YCBCR420 and CTM together are not possible\n"); > + return -EINVAL; BTW we have quite a few illegal combinations we're not currently catching. I hacked together some checks for them here: https://github.com/vsyrjala/linux/commit/0bc55aef05489a7e782bc14b7d5070eea52ce5ea and then I'm lifting some of those restrictions for pre-glk here https://github.com/vsyrjala/linux/commit/8db929ee8104c1ee640157d4ea8be15ea27451c5 Sadly on glk lost the flexibility to reorder the csc vs. a single LUT. I think what we need to do there is to use the gamma LUT to do the limited range compression, and for the YCbCr case we might just have to load the software gamma LUT into the hw degamma LUT as best we can. Otherwise the user may just get a totally unexpected fail when we automagically decide that limited range/YCbCr output is required. > } > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index afa21daaae51..81bfdbd99092 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6855,18 +6855,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > return -EINVAL; > } > > - if ((pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 || > - pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) && > - pipe_config->base.ctm) { > - /* > - * There is only one pipe CSC unit per pipe, and we need that > - * for output conversion from RGB->YCBCR. So if CTM is already > - * applied we can't support YCBCR420 output. > - */ > - DRM_DEBUG_KMS("YCBCR420 and CTM together are not possible\n"); > - return -EINVAL; > - } > - > /* > * Pipe horizontal size must be even in: > * - DVO ganged mode > -- > 2.14.5
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index da7a07d5ccea..b47e6d1aaaec 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -780,9 +780,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) limited_color_range = crtc_state->limited_color_range; - crtc_state->csc_enable = - crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || - crtc_state->base.ctm || limited_color_range; + crtc_state->csc_enable = crtc_state->base.ctm || limited_color_range; + if (INTEL_GEN(dev_priv) < 11) + crtc_state->csc_enable |= + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB; ret = intel_color_add_affected_planes(crtc_state); if (ret) @@ -822,6 +823,15 @@ int intel_color_check(struct intel_crtc_state *crtc_state) crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; crtc_state->csc_mode |= ICL_CSC_ENABLE; + } else if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB && + crtc_state->base.ctm) { + /* + * There is only one pipe CSC unit per pipe on pre-gen11, and + * we need that for output conversion from RGB->YCBCR. So if + * CTM is already applied we can't support YCBCR output. + */ + DRM_DEBUG_KMS("YCBCR420 and CTM together are not possible\n"); + return -EINVAL; } return 0; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index afa21daaae51..81bfdbd99092 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6855,18 +6855,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, return -EINVAL; } - if ((pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 || - pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) && - pipe_config->base.ctm) { - /* - * There is only one pipe CSC unit per pipe, and we need that - * for output conversion from RGB->YCBCR. So if CTM is already - * applied we can't support YCBCR420 output. - */ - DRM_DEBUG_KMS("YCBCR420 and CTM together are not possible\n"); - return -EINVAL; - } - /* * Pipe horizontal size must be even in: * - DVO ganged mode
We recently added support for gen11's new "output csc" which can be used independently of the regular pipe CSC. The idea is that this new output csc allows us to use a userspace-provided color transformation matrix at the same time we drive a YCBCR display mode requiring RGB->YUV conversion. However when landing that support we only updated the color management code and overlooked that there was an additional atomic check in the modeset path (in intel_crtc_compute_config()) that still assumes we only have a single CSC unit to work with. Let's update that check to only apply to pre-gen11 platforms and move it into the color management code to ensure it gets called on all commits, not just modesets. Also, if we're *only* using the output CSC and not the pipe CSC, there's no need to set crtc_state->csc_enable, so let's also not consider the output format when setting csc_enable on gen11+ platforms. Fixes: a91de580541c ("drm/i915/icl: Enable pipe output csc") Cc: Uma Shankar <uma.shankar@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- It might also be worth moving the full->limited range RGB conversion from the pipe CSC to the output CSC on gen11 at some point. drivers/gpu/drm/i915/intel_color.c | 16 +++++++++++++--- drivers/gpu/drm/i915/intel_display.c | 12 ------------ 2 files changed, 13 insertions(+), 15 deletions(-)