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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
