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

[Shankar, Uma]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Ville
>Syrjälä
>Sent: Friday, April 26, 2019 8:07 PM
>To: Sharma, Shashank <shashank.sharma at intel.com>
>Cc: intel-gfx at lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Enable pipe HDR mode on ICL if only
>HDR planes are used
>
>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.

Yeah AFAIU this bit just enables pipe to work at higher precision mode which should be ok
if we actually require lower precision (SDR cases) and shouldn't cause any problem. And will
be a must if actual HDR data is processed on the planes which will require higher precision. 

However enabling this always for HDR planes irrespective of content is actually fixing the crc
errors.

This patch is 
Reviewed-by: Uma Shankar <uma.shankar at intel.com>
And
Tested-by: Uma Shankar <uma.shankar at intel.com>

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