[Intel-gfx] [PATCH 2/2] drm/i915: Enable pipe HDR mode on ICL if only HDR planes are used

[Sharma, Shashank]

On 4/30/2019 4:09 PM, Ville Syrjälä wrote:
> On Tue, Apr 30, 2019 at 10:22:40AM +0530, Sharma, Shashank wrote:
>> On 4/26/2019 8:07 PM, Ville Syrjälä wrote:
>>> On Fri, Apr 26, 2019 at 06:40:11PM +0530, Sharma, Shashank wrote:
>>>> On 4/13/2019 12:00 AM, Ville Syrjala wrote:
>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>
>>>>> The pipe has a special HDR mode with higher precision when only
>>>>> HDR planes are active. Let's use it.
>>>>>
>>>>> Curiously this fixes the kms_color gamma/degamma tests when
>>>>> using a HDR plane, which is always the case unless one hacks
>>>>> the test to use an SDR plane. If one does hack the test to use
>>>>> an SDR plane it does pass already.
>>>>>
>>>>> I have no actual explanation how the output after the gamma
>>>>> LUT can be different between the two modes. The way the tests
>>>>> are written should mean that the output should be identical
>>>>> between the solid color vs. the gradient. But clearly that
>>>>> somehow doesn't hold true for the HDR planes in non-HDR pipe
>>>>> mode. Anyways, as long as we stick to one type of plane the
>>>>> test should produce sensible results now.
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_reg.h      |  1 +
>>>>>     drivers/gpu/drm/i915/intel_display.c |  7 +++++++
>>>>>     drivers/gpu/drm/i915/intel_sprite.h  | 12 ++++++++----
>>>>>     3 files changed, 16 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index 8ad2f0a03f28..90d60ecd3317 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -5767,6 +5767,7 @@ enum {
>>>>>     #define _PIPE_MISC_B			0x71030
>>>>>     #define   PIPEMISC_YUV420_ENABLE	(1 << 27)
>>>>>     #define   PIPEMISC_YUV420_MODE_FULL_BLEND (1 << 26)
>>>>> +#define   PIPEMISC_HDR_MODE		(1 << 23) /* icl+ */
>>>>>     #define   PIPEMISC_OUTPUT_COLORSPACE_YUV  (1 << 11)
>>>>>     #define   PIPEMISC_DITHER_BPC_MASK	(7 << 5)
>>>>>     #define   PIPEMISC_DITHER_8_BPC		(0 << 5)
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 490bd49ff42a..d0dbdbd5db3f 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -4055,6 +4055,9 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
>>>>>     			ironlake_pfit_disable(old_crtc_state);
>>>>>     	}
>>>>>     
>>>>> +	if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>>>>> +		bdw_set_pipemisc(new_crtc_state);
>>>>> +
>>>>>     	if (INTEL_GEN(dev_priv) >= 11)
>>>>>     		icl_set_pipe_chicken(crtc);
>>>>>     }
>>>>> @@ -8869,6 +8872,10 @@ static void bdw_set_pipemisc(const struct intel_crtc_state *crtc_state)
>>>>>     		val |= PIPEMISC_YUV420_ENABLE |
>>>>>     			PIPEMISC_YUV420_MODE_FULL_BLEND;
>>>>>     
>>>>> +	if (INTEL_GEN(dev_priv) >= 11 &&
>>>>> +	    (crtc_state->active_planes & ~icl_hdr_plane_mask()) == 0)
>>>>> +		val |= PIPEMISC_HDR_MODE;
>>>>> +
>>>> Shouldn't we check if the content being played on plane is HDR before
>>>> enabling this bit (even though I am not sure if there is any harm in
>>>> doing that)? Or maybe check the connector->output_hdr_metadata ? Most of
>>>> the times we would be sending SDR buffers on this plane. What happens
>>>> exactly when we set this bit ? The bspec says:
>>>>
>>>> "This field enables the HDR mode, allowing for higher precision output
>>>> from the HDR supporting planes and bypassing the SDR planes in blending. "
>>> I think the bit is just misnamed (like most things with "HDR" in their
>>> name). It's just a "gimme moar precision" bit.
>> Lets make this a bit more clear, may be rename the bit to
>> PIPEMISC_HDR_PRECISION_MODE instead?
> Then it won't match the spec.

Well we are keeping HDR_MODE aren't we ? may be 
PIPEMISC_HDR_MODE_PRECISION if that makes us closer to spec ?

- Shashank

>> With that change, this patch is
>>
>> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>>