Re: [PATCH v4 2/2] drm/i915: Validate userspace-provided color management LUT's (v4)

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 18, 2018 at 09:51:58AM -0800, Matt Roper wrote:
> We currently program userspace-provided gamma and degamma LUT's into our
> hardware without really checking to see whether they satisfy our
> hardware's rules.  We should try to catch tables that are invalid for
> our hardware early and reject the atomic transaction.
> 
> All of our platforms that accept a degamma LUT expect that the entries
> in the LUT are always flat or increasing, never decreasing.  Also, our
> GLK and ICL platforms only accept degamma tables with r=g=b entries; so
> we should also add the relevant checks for that in anticipation of
> degamma support landing for those platforms.
> 
> v2:
>  - Use new API (single check function with bitmask of tests to apply)
>  - Call helper for our gamma table as well (with no additional tests
>    specified) so that the table size will be validated.
> 
> v3:
>  - Don't call on the gamma table since the LUT size is already tested at
>    property blob upload and we don't have any additional hardware
>    constraints for that LUT.
> 
> v4:
>  - Apply equal color channel check on gen10 as well; the bspec has some
>    strange tagging for CNL platforms, but this appears to apply there as
>    well.  (Ville)
> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 37fd9ddf762e..e3ffbb0ad9a6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -609,10 +609,26 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  	size_t gamma_length, degamma_length;
> +	uint32_t tests = DRM_COLOR_LUT_NON_DECREASING;
>  
>  	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
>  	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>  
> +	/*
> +	 * All of our platforms mandate that the degamma curve be
> +	 * non-decreasing. 

This is actually not true. Only interpolated gamma modes require a
non-decreasing curve. The split gamma mode used on pre-glk is not
interpolated.

Also both CHV CGM gamma and degamma are interpolated, so we should
rather be checking gamma as well here.

> Additionally, GLK and gen11 only accept a single
> +	 * value for red, green, and blue in the degamma table.  Make sure
> +	 * userspace didn't try to pass us something we can't handle.
> +	 *
> +	 * We don't have any extra hardware constraints on the gamma table,
> +	 * so no need to explicitly check it.
> +	 */
> +	if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10)
> +		tests |= DRM_COLOR_LUT_EQUAL_CHANNELS;
> +
> +	if (drm_color_lut_check(crtc_state->base.degamma_lut, tests) != 0)
> +		return -EINVAL;
> +
>  	/*
>  	 * We allow both degamma & gamma luts at the right size or
>  	 * NULL.
> -- 
> 2.14.4

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx