Re: [PATCH 4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

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

On Wed, Sep 26, 2018 at 05:16:40PM -0700, Matt Roper wrote:
> On Fri, Sep 21, 2018 at 07:39:42PM +0200, Maarten Lankhorst wrote:
> > The first 3 planes (primary, sprite 0 and 1) have a dedicated chroma
> > upsampler to upscale YUV420 to YUV444 and the scaler should only be
> > used for upscaling. Because of this we shouldn't program the scalers
> > in planar mode if NV12 and the chroma upsampler are used. Instead
> > program the scalers like on normal planes.
> > 
> > Sprite 2 and 3 have no dedicated scaler, and need to program the
> > selected Y plane in the scaler mode.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
> >  drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
> >  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
> >  drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
> >  5 files changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index e7e6ca7f9665..1b59d15aaf59 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6872,6 +6872,8 @@ enum {
> >  #define PS_VADAPT_MODE_LEAST_ADAPT (0 << 5)
> >  #define PS_VADAPT_MODE_MOD_ADAPT   (1 << 5)
> >  #define PS_VADAPT_MODE_MOST_ADAPT  (3 << 5)
> > +#define PS_PLANE_Y_SEL_MASK  (7 << 5)
> > +#define PS_PLANE_Y_SEL(plane) (((plane) + 1) << 5)
> >  
> >  #define _PS_PWR_GATE_1A     0x68160
> >  #define _PS_PWR_GATE_2A     0x68260
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 20bfc89c652c..3c240ad0a8d3 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -235,9 +235,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >  		if (INTEL_GEN(dev_priv) == 9 &&
> >  		    !IS_GEMINILAKE(dev_priv))
> >  			mode = SKL_PS_SCALER_MODE_NV12;
> > -		else
> > +		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
> 
> Minor kernel coding standard violation here; we need to make the entire
> if/else block use braces once we add them to any branch.  Especially
> here where we've got nested if/else already.
> 
> >  			mode = PS_SCALER_MODE_PLANAR;
> >  
> > +			if (plane_state->linked_plane)
> > +				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
> > +		} else
> > +			mode = PS_SCALER_MODE_PACKED;
> 
> While this is correct, it looks really strange to have the code do
> "if nv12...set mode=packed" -- maybe we should change this to definition
> to _NORMAL rather than _PACKED; that's actually what the bspec calls
> this bit on gen11 anyway.
> 
> It might also be worth adding a comment around here explaining how the
> hardware is supposed to work since it's somewhat non-obvious:
> 
>   If NV12 on Planes 0-2: Uses chroma upsampler; scaler only used (in
>   normal mode) if actual scaling is necessary
>   NV12 on Planes 3-4: Scaler required (in planar mode) regardless of
>   whether real scaling is necessary

Rather that talking about specific plane numbers I suggest we stick to
the HDR vs. SDR plane terminology. Makes things more future proof at
the very least.

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