| Message ID | 1518584256-25253-11-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
My previous review comments are not addressed on this patch. I made a comment which is about programming bits *29:28* on *PS_CTRL for (GEN>=9) *blindly*. * This is not valid for all GEN9 and above, in fact bit 29:28 as scalar mode is only valid for SKL. Bit 28 is not supported for GLK, bspec clearly mentions do not program this. Also from GEN10 onward bit 28 is NOT for scaler mode, its for adaptive filtering. Regards Shashank On 2/14/2018 10:27 AM, Vidya Srinivas wrote: > From: Chandra Konduru <chandra.konduru@intel.com> > > This patch sets appropriate scaler mode for NV12 format. > In this mode, skylake scaler does either chroma-upsampling or > chroma-upsampling and resolution scaling > > v2: Review comments from Ville addressed > NV12 case to be checked first for setting > the scaler > > v3: Rebased (me) > > v4: Rebased (me) > > v5: Missed the Tested-by/Reviewed-by in the previous series > Adding the same to commit message in this version. > > v6: Rebased (me) > > v7: Rebased (me) > > v8: Rebased (me) > Restricting the NV12 change for scaler to BXT and KBL > in this series. > > v9: Rebased (me) > > v10: As of now, NV12 has been tested on Gen9 and Gen10. However, > code is applicable to all GEN >= 9. Hence making > that change to keep it generic. > Comments under v8 is not valid anymore. > > v11: Addressed review comments by Shashank Sharma. > For Gen10+, the scaler mode to be set it planar or normal > (single bit). Changed the code to be applicable to all > Gen. > > Tested-by: Clinton Taylor <clinton.a.taylor@intel.com> > Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_atomic.c | 8 ++++++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index f6afa5e..10edacc 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6732,6 +6732,7 @@ enum { > #define PS_SCALER_MODE_MASK (3 << 28) > #define PS_SCALER_MODE_DYN (0 << 28) > #define PS_SCALER_MODE_HQ (1 << 28) > +#define PS_SCALER_MODE_PLANAR (1 << 29) > #define PS_PLANE_SEL_MASK (7 << 25) > #define PS_PLANE_SEL(plane) (((plane) + 1) << 25) > #define PS_FILTER_MASK (3 << 23) > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index d452c32..7ae4f48 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -327,8 +327,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, > } > > /* set scaler mode */ > - if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { > - scaler_state->scalers[*scaler_id].mode = 0; > + if ((INTEL_GEN(dev_priv) >= 9) && > + plane_state && plane_state->base.fb && > + plane_state->base.fb->format->format == > + DRM_FORMAT_NV12) { > + scaler_state->scalers[*scaler_id].mode = > + PS_SCALER_MODE_PLANAR; > } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) { > /* > * when only 1 scaler is in use on either pipe A or B,
I addressed it with PS_SCALER_MODE_PLANAR (1 << 29) to keep it generic. But as you mentioned that wouldn't be the right way. I need to keep GEN9 and GEN10+ separate. Will fix it in the next push. Thank you. Vidya From: Sharma, Shashank Sent: Thursday, February 15, 2018 12:03 PM To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx@lists.freedesktop.org Cc: maarten.lankhorst@linux.intel.com; Kamath, Sunil <sunil.kamath@intel.com>; Shankar, Uma <uma.shankar@intel.com>; Konduru, Chandra <chandra.konduru@intel.com>; Maiti, Nabendu Bikash <nabendu.bikash.maiti@intel.com> Subject: Re: [PATCH 10/16] drm/i915: Set scaler mode for NV12 My previous review comments are not addressed on this patch. I made a comment which is about programming bits 29:28 on PS_CTRL for (GEN>=9) blindly. This is not valid for all GEN9 and above, in fact bit 29:28 as scalar mode is only valid for SKL. Bit 28 is not supported for GLK, bspec clearly mentions do not program this. Also from GEN10 onward bit 28 is NOT for scaler mode, its for adaptive filtering. Regards Shashank On 2/14/2018 10:27 AM, Vidya Srinivas wrote: From: Chandra Konduru <chandra.konduru@intel.com><mailto:chandra.konduru@intel.com> This patch sets appropriate scaler mode for NV12 format. In this mode, skylake scaler does either chroma-upsampling or chroma-upsampling and resolution scaling v2: Review comments from Ville addressed NV12 case to be checked first for setting the scaler v3: Rebased (me) v4: Rebased (me) v5: Missed the Tested-by/Reviewed-by in the previous series Adding the same to commit message in this version. v6: Rebased (me) v7: Rebased (me) v8: Rebased (me) Restricting the NV12 change for scaler to BXT and KBL in this series. v9: Rebased (me) v10: As of now, NV12 has been tested on Gen9 and Gen10. However, code is applicable to all GEN >= 9. Hence making that change to keep it generic. Comments under v8 is not valid anymore. v11: Addressed review comments by Shashank Sharma. For Gen10+, the scaler mode to be set it planar or normal (single bit). Changed the code to be applicable to all Gen. Tested-by: Clinton Taylor <clinton.a.taylor@intel.com><mailto:clinton.a.taylor@intel.com> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com><mailto:clinton.a.taylor@intel.com> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com><mailto:chandra.konduru@intel.com> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com><mailto:nabendu.bikash.maiti@intel.com> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com><mailto:vidya.srinivas@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_atomic.c | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f6afa5e..10edacc 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6732,6 +6732,7 @@ enum { #define PS_SCALER_MODE_MASK (3 << 28) #define PS_SCALER_MODE_DYN (0 << 28) #define PS_SCALER_MODE_HQ (1 << 28) +#define PS_SCALER_MODE_PLANAR (1 << 29) #define PS_PLANE_SEL_MASK (7 << 25) #define PS_PLANE_SEL(plane) (((plane) + 1) << 25) #define PS_FILTER_MASK (3 << 23) diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index d452c32..7ae4f48 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -327,8 +327,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, } /* set scaler mode */ - if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { - scaler_state->scalers[*scaler_id].mode = 0; + if ((INTEL_GEN(dev_priv) >= 9) && + plane_state && plane_state->base.fb && + plane_state->base.fb->format->format == + DRM_FORMAT_NV12) { + scaler_state->scalers[*scaler_id].mode = + PS_SCALER_MODE_PLANAR; } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) { /* * when only 1 scaler is in use on either pipe A or B,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f6afa5e..10edacc 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6732,6 +6732,7 @@ enum { #define PS_SCALER_MODE_MASK (3 << 28) #define PS_SCALER_MODE_DYN (0 << 28) #define PS_SCALER_MODE_HQ (1 << 28) +#define PS_SCALER_MODE_PLANAR (1 << 29) #define PS_PLANE_SEL_MASK (7 << 25) #define PS_PLANE_SEL(plane) (((plane) + 1) << 25) #define PS_FILTER_MASK (3 << 23) diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index d452c32..7ae4f48 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -327,8 +327,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, } /* set scaler mode */ - if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { - scaler_state->scalers[*scaler_id].mode = 0; + if ((INTEL_GEN(dev_priv) >= 9) && + plane_state && plane_state->base.fb && + plane_state->base.fb->format->format == + DRM_FORMAT_NV12) { + scaler_state->scalers[*scaler_id].mode = + PS_SCALER_MODE_PLANAR; } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) { /* * when only 1 scaler is in use on either pipe A or B,