[Intel-gfx] [PATCH 3/3] drm/i915/icl: Add Multi-segmented gamma support

[Sharma, Shashank]

On 4/29/2019 5:19 PM, Ville Syrjälä wrote:
> On Mon, Apr 29, 2019 at 05:00:21PM +0530, Sharma, Shashank wrote:
>> On 4/26/2019 11:59 PM, Ville Syrjälä wrote:
>>> On Fri, Apr 26, 2019 at 11:31:51PM +0530, Shashank Sharma wrote:
>>>> ICL introduces a new gamma correction mode in display engine, called
>>>> multi-segmented-gamma mode. This mode allows users to program the
>>>> darker region of the gamma curve with sueprfine precision. An
>>>> example use case for this is HDR curves (like PQ ST-2084).
>>>>
>>>> If we plot a gamma correction curve from value range between 0.0 to 1.0,
>>>> ICL's multi-segment has 3 different sections:
>>>> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
>>>> - fine segment: 257 values, ranges between 0 - 1/(128)
>>>> - corase segment: 257 values, ranges between 0 - 1
>>>>
>>>> This patch:
>>>> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
>>>>     so that userspace can program with highest precision supported.
>>>> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
>>>> - Adds functions to program/detect multi-segment gamma.
>>>>
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>>
>>>> Suggested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_pci.c    |   3 +-
>>>>    drivers/gpu/drm/i915/intel_color.c | 155 ++++++++++++++++++++++++++++-
>>>>    2 files changed, 156 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>>>> index ffa2ee70a03d..83698951760b 100644
>>>> --- a/drivers/gpu/drm/i915/i915_pci.c
>>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>>>> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
>>>>    	GEN(11), \
>>>>    	.ddb_size = 2048, \
>>>>    	.has_logical_ring_elsq = 1, \
>>>> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
>>>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
>>>> +
>>>>    
>>>>    static const struct intel_device_info intel_icelake_11_info = {
>>>>    	GEN11_FEATURES,
>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>> index ca341a9e47e6..d1fb79a5d764 100644
>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>> @@ -41,6 +41,7 @@
>>>>    #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>>>>    
>>>>    #define LEGACY_LUT_LENGTH		256
>>>> +#define ICL_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
>>>>    /*
>>>>     * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>>>>     * format). This macro takes the coefficient we want transformed and the
>>>> @@ -58,6 +59,12 @@
>>>>    
>>>>    #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
>>>>    
>>>> +enum icl_ms_gamma_segments {
>>>> +	ICL_MS_GAMMA_SEG_SUPERFINE,
>>>> +	ICL_MS_GAMMA_SEG_FINE,
>>>> +	ICL_MS_GAMMA_SEG_COARSE,
>>>> +};
>>>> +
>>>>    static const u16 ilk_csc_off_zero[3] = {};
>>>>    
>>>>    static const u16 ilk_csc_coeff_identity[9] = {
>>>> @@ -767,6 +774,149 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>>>>    	}
>>>>    }
>>>>    
>>>> +/* ilk+ "12.4" interpolated format (high 10 bits) */
>>>> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>>>> +{
>>>> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
>>>> +		(color->blue >> 6);
>>>> +}
>>>> +
>>>> +/* ilk+ "12.4" interpolated format (low 6 bits) */
>>>> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
>>>> +{
>>>> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
>>>> +		(color->blue & 0x3f);
>>>> +}
>>>> +
>>>> +static void
>>>> +icl_program_gamma_gcmax(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);
>>>> +	enum pipe pipe = crtc->pipe;
>>>> +
>>>> +	/*
>>>> +	 * Program the max register to clamp values > 1.0.
>>>> +	 * ToDo: Extend the ABI to be able to program values
>>>> +	 * from 1.0
>>>> +	 */
>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16));
>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16));
>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16));
>>> This one I think we want to program based on the provide LUT. It's the
>>> last entry that still gets used in interpolation for <1.0 values.
>>> Or at least that's the way it works with the 12p4 mode IIRC. I don't
>>> actually remember how it goes with the multi segment mode.
>> I am a bit skeptical if its a good idea to program from the LUT, as you
>> know, the drm_color_lut elements are 16bit per channel, and in order to
>> program 1.0 we need 17 bits. So even if userspace wants, it cant set the
>> value of 1.0 as of now. Also, wouldn't it would be good for the
>> interpolation to have the accurate max value (1.0) of the curve ?
> If userspace doesn't want the max to be 1.0 then we will do the wrong
> thing here. But I suppose that wouldn't be a very common thing to do.
My point exactly. Most of the time userspace wants to apply a known curve.
> The better arguemnt for doing what I suggest is that we already do it
> like that on the other platforms with interpolated gamma (gen4/vlv/chv).

Ok, lets keep it the last value of the LUT then.

- Shashank