On Tue, Apr 30, 2019 at 07:13:36PM +0530, Sharma, Shashank wrote:
>
> 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@xxxxxxxxxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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 ?
Not super convinved that really helps the reader in any significant way,
but I changed it anyway.
Series pushed to dinq. Thanks for the reviews.
>
> - Shashank
>
> >> With that change, this patch is
> >>
> >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> >>
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
