diff mbox

[10/16] drm/i915: Set scaler mode for NV12

Message ID 1518584256-25253-11-git-send-email-vidya.srinivas@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vidya Srinivas Feb. 14, 2018, 4:57 a.m. UTC
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(-)

Comments

Sharma, Shashank Feb. 15, 2018, 6:32 a.m. UTC | #1
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,
Vidya Srinivas Feb. 15, 2018, 9:28 a.m. UTC | #2
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 mbox

Patch

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,