[PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Dhinakaran Pandiyan]

The shifts for VSC_SELECT bits are wrong, fix it. Good thing is the
definitions are unused.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 27e650fe591b..a0ad77b9212b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4584,6 +4584,15 @@ enum {
 #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
 #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
 /* HSW and later: */
+#define   DRM_DIP_ENABLE		(1 << 28)
+#define   PSR_VSC_BIT_7_SET		(1 << 27)
+#define   VSC_SELECT_MASK		(0x3 << 25)
+#define   VSC_SELECT_SHIFT		25
+#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
+#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
+#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
+#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
+#define   VDIP_ENABLE_PPS		(1 << 24)
 #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
 #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
 #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
@@ -4591,15 +4600,6 @@ enum {
 #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
 #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
 
-#define  DRM_DIP_ENABLE			(1 << 28)
-#define  PSR_VSC_BIT_7_SET		(1 << 27)
-#define  VSC_SELECT_MASK		(0x3 << 26)
-#define  VSC_SELECT_SHIFT		26
-#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
-#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
-#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
-#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
-#define  VDIP_ENABLE_PPS		(1 << 24)
 
 /* Panel power sequencing */
 #define PPS_BASE			0x61200
-- 
2.14.1

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

✓ Fi.CI.BAT: success for drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Patchwork]

== Series Details ==

Series: drm/i915: Fix VIDEO_DIP_CTL bit shifts
URL   : https://patchwork.freedesktop.org/series/50573/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4932 -> Patchwork_10365 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50573/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10365 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-dpms:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927


== Participating hosts (48 -> 39) ==

  Missing    (9): fi-hsw-4770r fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bwr-2160 fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


== Build changes ==

    * Linux: CI_DRM_4932 -> Patchwork_10365

  CI_DRM_4932: 21f90148bf7adb33d82580013a5697a6bbb88248 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4669: 5f40e617cd9c1e089f4a2d79c53a417d891e3e3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10365: dabd694408ac19e4f366640c15665211e4de0e53 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

dabd694408ac drm/i915: Fix VIDEO_DIP_CTL bit shifts

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10365/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Lucas De Marchi]

On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan wrote:
> The shifts for VSC_SELECT bits are wrong, fix it. Good thing is the
> definitions are unused.

If they are unused why are we fixing them instead of removing? Or better,
why did we add them?

Lucas De Marchi

> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 27e650fe591b..a0ad77b9212b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4584,6 +4584,15 @@ enum {
>  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
>  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
>  /* HSW and later: */
> +#define   DRM_DIP_ENABLE		(1 << 28)
> +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> +#define   VSC_SELECT_MASK		(0x3 << 25)
> +#define   VSC_SELECT_SHIFT		25
> +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> +#define   VDIP_ENABLE_PPS		(1 << 24)
>  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
>  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
>  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> @@ -4591,15 +4600,6 @@ enum {
>  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
>  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
>  
> -#define  DRM_DIP_ENABLE			(1 << 28)
> -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> -#define  VSC_SELECT_MASK		(0x3 << 26)
> -#define  VSC_SELECT_SHIFT		26
> -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> -#define  VDIP_ENABLE_PPS		(1 << 24)
>  
>  /* Panel power sequencing */
>  #define PPS_BASE			0x61200
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Dhinakaran Pandiyan]

On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan wrote:
> > The shifts for VSC_SELECT bits are wrong, fix it. Good thing is the
> > definitions are unused.
> 
> If they are unused why are we fixing them instead of removing? Or
> better,
> why did we add them?
I guess there are plans to make use of them, no idea.

Cc: Anusha, Manasi

> 
> Lucas De Marchi
> 
> > 
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 27e650fe591b..a0ad77b9212b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4584,6 +4584,15 @@ enum {
> >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> >  /* HSW and later: */
> > +#define   DRM_DIP_ENABLE		(1 << 28)
> > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > +#define   VSC_SELECT_SHIFT		25
> > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > +#define   VDIP_ENABLE_PPS		(1 << 24)
> >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > @@ -4591,15 +4600,6 @@ enum {
> >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> >  
> > -#define  DRM_DIP_ENABLE			(1 << 28)
> > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > -#define  VSC_SELECT_SHIFT		26
> > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > -#define  VDIP_ENABLE_PPS		(1 << 24)
> >  
> >  /* Panel power sequencing */
> >  #define PPS_BASE			0x61200
> > -- 
> > 2.14.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Manasi Navare]

On Thu, Oct 04, 2018 at 04:13:26PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> > On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan wrote:
> > > The shifts for VSC_SELECT bits are wrong, fix it. Good thing is the
> > > definitions are unused.
> > 
> > If they are unused why are we fixing them instead of removing? Or
> > better,
> > why did we add them?
> I guess there are plans to make use of them, no idea.
>

Yes, the VDIP_RNABLE_PPS and DIP enables get used in the DSC patch series:

https://patchwork.freedesktop.org/series/47514/

If you want I can combine this fixes patch with the new revision of DSC patchseries
I am about to send out

Manasi
 
> Cc: Anusha, Manasi
> 
> > 
> > Lucas De Marchi
> > 
> > > 
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 27e650fe591b..a0ad77b9212b 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4584,6 +4584,15 @@ enum {
> > >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> > >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> > >  /* HSW and later: */
> > > +#define   DRM_DIP_ENABLE		(1 << 28)
> > > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > > +#define   VSC_SELECT_SHIFT		25
> > > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > > +#define   VDIP_ENABLE_PPS		(1 << 24)
> > >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> > >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> > >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > > @@ -4591,15 +4600,6 @@ enum {
> > >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> > >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> > >  
> > > -#define  DRM_DIP_ENABLE			(1 << 28)
> > > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > > -#define  VSC_SELECT_SHIFT		26
> > > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > > -#define  VDIP_ENABLE_PPS		(1 << 24)
> > >  
> > >  /* Panel power sequencing */
> > >  #define PPS_BASE			0x61200
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Dhinakaran Pandiyan]

On Thu, 2018-10-04 at 16:28 -0700, Manasi Navare wrote:
> On Thu, Oct 04, 2018 at 04:13:26PM -0700, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> > > On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > The shifts for VSC_SELECT bits are wrong, fix it. Good thing is
> > > > the
> > > > definitions are unused.
> > > 
> > > If they are unused why are we fixing them instead of removing? Or
> > > better,
> > > why did we add them?
> > 
> > I guess there are plans to make use of them, no idea.
> > 
> 
> Yes, the VDIP_RNABLE_PPS and DIP enables get used in the DSC patch
> series:
> 
> https://patchwork.freedesktop.org/series/47514/
> 
> If you want I can combine this fixes patch with the new revision of
> DSC patchseries
> I am about to send out

That might create an unnecessary dependency on the series getting
merged. We'll have to program these bits for PSR2 from the looks of it.
Let's get this into the tree soon, can you please review the fix?

-DK

> 
> Manasi
>  
> > Cc: Anusha, Manasi
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > > 
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
> > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 27e650fe591b..a0ad77b9212b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -4584,6 +4584,15 @@ enum {
> > > >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> > > >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> > > >  /* HSW and later: */
> > > > +#define   DRM_DIP_ENABLE		(1 << 28)
> > > > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > > > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > > > +#define   VSC_SELECT_SHIFT		25
> > > > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > > > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > > > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > > > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > > > +#define   VDIP_ENABLE_PPS		(1 << 24)
> > > >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> > > >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> > > >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > > > @@ -4591,15 +4600,6 @@ enum {
> > > >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> > > >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> > > >  
> > > > -#define  DRM_DIP_ENABLE			(1 << 28)
> > > > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > > > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > > > -#define  VSC_SELECT_SHIFT		26
> > > > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > > > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > > > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > > > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > > > -#define  VDIP_ENABLE_PPS		(1 << 24)
> > > >  
> > > >  /* Panel power sequencing */
> > > >  #define PPS_BASE			0x61200
> > > > -- 
> > > > 2.14.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Manasi Navare]

On Thu, Oct 04, 2018 at 05:00:06PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-10-04 at 16:28 -0700, Manasi Navare wrote:
> > On Thu, Oct 04, 2018 at 04:13:26PM -0700, Dhinakaran Pandiyan wrote:
> > > On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> > > > On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan
> > > > wrote:
> > > > > The shifts for VSC_SELECT bits are wrong, fix it. Good thing is
> > > > > the
> > > > > definitions are unused.

No need to mention that defs are unused in the commit since that will not make sense
once the patches get merged that start using these.

More comments below

> > > > 
> > > > If they are unused why are we fixing them instead of removing? Or
> > > > better,
> > > > why did we add them?
> > > 
> > > I guess there are plans to make use of them, no idea.
> > > 
> > 
> > Yes, the VDIP_RNABLE_PPS and DIP enables get used in the DSC patch
> > series:
> > 
> > https://patchwork.freedesktop.org/series/47514/
> > 
> > If you want I can combine this fixes patch with the new revision of
> > DSC patchseries
> > I am about to send out
> 
> That might create an unnecessary dependency on the series getting
> merged. We'll have to program these bits for PSR2 from the looks of it.
> Let's get this into the tree soon, can you please review the fix?
> 
> -DK
> 
> > 
> > Manasi
> >  
> > > Cc: Anusha, Manasi
> > > 
> > > > 
> > > > Lucas De Marchi
> > > > 
> > > > > 
> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
> > > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > > dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> > > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 27e650fe591b..a0ad77b9212b 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -4584,6 +4584,15 @@ enum {
> > > > >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> > > > >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> > > > >  /* HSW and later: */
> > > > > +#define   DRM_DIP_ENABLE		(1 << 28)
> > > > > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > > > > +#define   VSC_SELECT_SHIFT		25
> > > > > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > > > > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > > > > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > > > > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > > > > +#define   VDIP_ENABLE_PPS		(1 << 24)
> > > > >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> > > > >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> > > > >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > > > > @@ -4591,15 +4600,6 @@ enum {
> > > > >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> > > > >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> > > > >  
> > > > > -#define  DRM_DIP_ENABLE			(1 << 28)
> > > > > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > > > > -#define  VSC_SELECT_SHIFT		26
> > > > > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > > > > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > > > > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > > > > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > > > > -#define  VDIP_ENABLE_PPS		(1 << 24)

Why do you need to remove all of these defs?
From the spec, the only shift thats wrong is :
VSC_SELECT_MASK             (0x3 << 26)
#define  VSC_SELECT_SHIFT            26
#define  VSC_DIP_HW_HEA_DATA         (0 << 26)
define  VSC_DIP_HW_HEA_SW_DATA              (1 << 26)
define  VSC_DIP_HW_DATA_SW_HEA              (2 << 26)
VSC_DIP_SW_HEA_DATA         (3 << 26)

Removing others and redefining to be the same is misleading

Other than that the actual fixes look good, double checked with the spec.
So after the above fix and the commit message fix

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> > > > >  
> > > > >  /* Panel power sequencing */
> > > > >  #define PPS_BASE			0x61200
> > > > > -- 
> > > > > 2.14.1
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Dhinakaran Pandiyan]

On Thu, 2018-10-04 at 17:27 -0700, Manasi Navare wrote:
> On Thu, Oct 04, 2018 at 05:00:06PM -0700, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-10-04 at 16:28 -0700, Manasi Navare wrote:
> > > On Thu, Oct 04, 2018 at 04:13:26PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> > > > > On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan
> > > > > wrote:
> > > > > > The shifts for VSC_SELECT bits are wrong, fix it. Good
> > > > > > thing is
> > > > > > the
> > > > > > definitions are unused.
> 
> No need to mention that defs are unused in the commit since that will
> not make sense
> once the patches get merged that start using these.
Well, the patch is based on the current state of the code base.

> 
> More comments below
> 
> > > > > 
> > > > > If they are unused why are we fixing them instead of
> > > > > removing? Or
> > > > > better,
> > > > > why did we add them?
> > > > 
> > > > I guess there are plans to make use of them, no idea.
> > > > 
> > > 
> > > Yes, the VDIP_RNABLE_PPS and DIP enables get used in the DSC
> > > patch
> > > series:
> > > 
> > > https://patchwork.freedesktop.org/series/47514/
> > > 
> > > If you want I can combine this fixes patch with the new revision
> > > of
> > > DSC patchseries
> > > I am about to send out
> > 
> > That might create an unnecessary dependency on the series getting
> > merged. We'll have to program these bits for PSR2 from the looks of
> > it.
> > Let's get this into the tree soon, can you please review the fix?
> > 
> > -DK
> > 
> > > 
> > > Manasi
> > >  
> > > > Cc: Anusha, Manasi
> > > > 
> > > > > 
> > > > > Lucas De Marchi
> > > > > 
> > > > > > 
> > > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP
> > > > > > registers")
> > > > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > > > dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> > > > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 27e650fe591b..a0ad77b9212b 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -4584,6 +4584,15 @@ enum {
> > > > > >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> > > > > >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> > > > > >  /* HSW and later: */
> > > > > > +#define   DRM_DIP_ENABLE		(1 << 28)
> > > > > > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > > > > > +#define   VSC_SELECT_SHIFT		25
> > > > > > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > > > > > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > > > > > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > > > > > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > > > > > +#define   VDIP_ENABLE_PPS		(1 << 24)
> > > > > >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> > > > > >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> > > > > >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > > > > > @@ -4591,15 +4600,6 @@ enum {
> > > > > >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> > > > > >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> > > > > >  
> > > > > > -#define  DRM_DIP_ENABLE			(1 << 28)
> > > > > > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > > > > > -#define  VSC_SELECT_SHIFT		26
> > > > > > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > > > > > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > > > > > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > > > > > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > > > > > -#define  VDIP_ENABLE_PPS		(1 << 24)
> 
> Why do you need to remove all of these defs?
> From the spec, the only shift thats wrong is :
> VSC_SELECT_MASK             (0x3 << 26)
> #define  VSC_SELECT_SHIFT            26
> #define  VSC_DIP_HW_HEA_DATA         (0 << 26)
> define  VSC_DIP_HW_HEA_SW_DATA              (1 << 26)
> define  VSC_DIP_HW_DATA_SW_HEA              (2 << 26)
> VSC_DIP_SW_HEA_DATA         (3 << 26)
> 
> Removing others and redefining to be the same is misleading

Please see other definitions in the file and the documentation above,
we generally organize bit definitions in the descending order of their
position.


> 
> Other than that the actual fixes look good, double checked with the
> spec.
> So after the above fix and the commit message fix
> 
> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> 
> Manasi
> 
> > > > > >  
> > > > > >  /* Panel power sequencing */
> > > > > >  #define PPS_BASE			0x61200
> > > > > > -- 
> > > > > > 2.14.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Manasi Navare]

On Thu, Oct 04, 2018 at 07:56:37PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-10-04 at 17:27 -0700, Manasi Navare wrote:
> > On Thu, Oct 04, 2018 at 05:00:06PM -0700, Dhinakaran Pandiyan wrote:
> > > On Thu, 2018-10-04 at 16:28 -0700, Manasi Navare wrote:
> > > > On Thu, Oct 04, 2018 at 04:13:26PM -0700, Dhinakaran Pandiyan
> > > > wrote:
> > > > > On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> > > > > > On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan
> > > > > > wrote:
> > > > > > > The shifts for VSC_SELECT bits are wrong, fix it. Good
> > > > > > > thing is
> > > > > > > the
> > > > > > > definitions are unused.
> > 
> > No need to mention that defs are unused in the commit since that will
> > not make sense
> > once the patches get merged that start using these.
> Well, the patch is based on the current state of the code base.
>

Well IMHO that statement is still redundant and more expressive than informative.
Mentioning that this is
a fix for VSC SELECT should suffice. But thats a nitcpick, so its your call.
  
> > 
> > More comments below
> > 
> > > > > > 
> > > > > > If they are unused why are we fixing them instead of
> > > > > > removing? Or
> > > > > > better,
> > > > > > why did we add them?
> > > > > 
> > > > > I guess there are plans to make use of them, no idea.
> > > > > 
> > > > 
> > > > Yes, the VDIP_RNABLE_PPS and DIP enables get used in the DSC
> > > > patch
> > > > series:
> > > > 
> > > > https://patchwork.freedesktop.org/series/47514/
> > > > 
> > > > If you want I can combine this fixes patch with the new revision
> > > > of
> > > > DSC patchseries
> > > > I am about to send out
> > > 
> > > That might create an unnecessary dependency on the series getting
> > > merged. We'll have to program these bits for PSR2 from the looks of
> > > it.
> > > Let's get this into the tree soon, can you please review the fix?
> > > 
> > > -DK
> > > 
> > > > 
> > > > Manasi
> > > >  
> > > > > Cc: Anusha, Manasi
> > > > > 
> > > > > > 
> > > > > > Lucas De Marchi
> > > > > > 
> > > > > > > 
> > > > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP
> > > > > > > registers")
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > > > > dhinakaran.pandiyan@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> > > > > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > index 27e650fe591b..a0ad77b9212b 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > @@ -4584,6 +4584,15 @@ enum {
> > > > > > >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> > > > > > >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> > > > > > >  /* HSW and later: */
> > > > > > > +#define   DRM_DIP_ENABLE		(1 << 28)
> > > > > > > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > > > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > > > > > > +#define   VSC_SELECT_SHIFT		25
> > > > > > > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > > > > > > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > > > > > > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > > > > > > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > > > > > > +#define   VDIP_ENABLE_PPS		(1 << 24)
> > > > > > >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> > > > > > >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> > > > > > >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > > > > > > @@ -4591,15 +4600,6 @@ enum {
> > > > > > >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> > > > > > >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> > > > > > >  
> > > > > > > -#define  DRM_DIP_ENABLE			(1 << 28)
> > > > > > > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > > > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > > > > > > -#define  VSC_SELECT_SHIFT		26
> > > > > > > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > > > > > > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > > > > > > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > > > > > > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > > > > > > -#define  VDIP_ENABLE_PPS		(1 << 24)
> > 
> > Why do you need to remove all of these defs?
> > From the spec, the only shift thats wrong is :
> > VSC_SELECT_MASK             (0x3 << 26)
> > #define  VSC_SELECT_SHIFT            26
> > #define  VSC_DIP_HW_HEA_DATA         (0 << 26)
> > define  VSC_DIP_HW_HEA_SW_DATA              (1 << 26)
> > define  VSC_DIP_HW_DATA_SW_HEA              (2 << 26)
> > VSC_DIP_SW_HEA_DATA         (3 << 26)
> > 
> > Removing others and redefining to be the same is misleading
> 
> Please see other definitions in the file and the documentation above,
> we generally organize bit definitions in the descending order of their
> position.
>

I still dont see why you had remove earlier defs and redefine since
it was already organized in descending order of their position in the originl
patches starting with DRM_DIP_ENABLE with 1<< 28 all the way to VDIP_ENABLE_PPS with
1 << 24 and your changes keeps the same order with the fix of VSC HSIFT from 26 to 25.
So only those VSC defs should be changed not the entire set.

Manasi
 
> 
> > 
> > Other than that the actual fixes look good, double checked with the
> > spec.
> > So after the above fix and the commit message fix
> > 
> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> > 
> > Manasi
> > 
> > > > > > >  
> > > > > > >  /* Panel power sequencing */
> > > > > > >  #define PPS_BASE			0x61200
> > > > > > > -- 
> > > > > > > 2.14.1
> > > > > > > 
> > > > > > > _______________________________________________
> > > > > > > Intel-gfx mailing list
> > > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 11:10 -0700, Manasi Navare wrote:
> On Thu, Oct 04, 2018 at 07:56:37PM -0700, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-10-04 at 17:27 -0700, Manasi Navare wrote:
> > > On Thu, Oct 04, 2018 at 05:00:06PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > On Thu, 2018-10-04 at 16:28 -0700, Manasi Navare wrote:
> > > > > On Thu, Oct 04, 2018 at 04:13:26PM -0700, Dhinakaran Pandiyan
> > > > > wrote:
> > > > > > On Thu, 2018-10-04 at 16:03 -0700, Lucas De Marchi wrote:
> > > > > > > On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran
> > > > > > > Pandiyan
> > > > > > > wrote:
> > > > > > > > The shifts for VSC_SELECT bits are wrong, fix it. Good
> > > > > > > > thing is
> > > > > > > > the
> > > > > > > > definitions are unused.
> > > 
> > > No need to mention that defs are unused in the commit since that
> > > will
> > > not make sense
> > > once the patches get merged that start using these.
> > 
> > Well, the patch is based on the current state of the code base.
> > 
> 
> Well IMHO that statement is still redundant and more expressive than
> informative.
> Mentioning that this is
> a fix for VSC SELECT should suffice. But thats a nitcpick, so its
> your call.
>   
> > > 
> > > More comments below
> > > 
> > > > > > > 
> > > > > > > If they are unused why are we fixing them instead of
> > > > > > > removing? Or
> > > > > > > better,
> > > > > > > why did we add them?
> > > > > > 
> > > > > > I guess there are plans to make use of them, no idea.
> > > > > > 
> > > > > 
> > > > > Yes, the VDIP_RNABLE_PPS and DIP enables get used in the DSC
> > > > > patch
> > > > > series:
> > > > > 
> > > > > https://patchwork.freedesktop.org/series/47514/
> > > > > 
> > > > > If you want I can combine this fixes patch with the new
> > > > > revision
> > > > > of
> > > > > DSC patchseries
> > > > > I am about to send out
> > > > 
> > > > That might create an unnecessary dependency on the series
> > > > getting
> > > > merged. We'll have to program these bits for PSR2 from the
> > > > looks of
> > > > it.
> > > > Let's get this into the tree soon, can you please review the
> > > > fix?
> > > > 
> > > > -DK
> > > > 
> > > > > 
> > > > > Manasi
> > > > >  
> > > > > > Cc: Anusha, Manasi
> > > > > > 
> > > > > > > 
> > > > > > > Lucas De Marchi
> > > > > > > 
> > > > > > > > 
> > > > > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP
> > > > > > > > registers")
> > > > > > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > > > > > dhinakaran.pandiyan@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++------
> > > > > > > > ---
> > > > > > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > index 27e650fe591b..a0ad77b9212b 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > @@ -4584,6 +4584,15 @@ enum {
> > > > > > > >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 <<
> > > > > > > > 16)
> > > > > > > >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> > > > > > > >  /* HSW and later: */
> > > > > > > > +#define   DRM_DIP_ENABLE		(1 << 28)
> > > > > > > > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > > > > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > > > > > > > +#define   VSC_SELECT_SHIFT		25
> > > > > > > > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > > > > > > > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > > > > > > > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > > > > > > > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > > > > > > > +#define   VDIP_ENABLE_PPS		(1 << 24)
> > > > > > > >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> > > > > > > >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> > > > > > > >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > > > > > > > @@ -4591,15 +4600,6 @@ enum {
> > > > > > > >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> > > > > > > >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> > > > > > > >  
							->
> > > > > > > > -#define  DRM_DIP_ENABLE			(1 <<
> > > > > > > > 28)
> > > > > > > > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > > > > > > > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > > > > > > > -#define  VSC_SELECT_SHIFT		26
> > > > > > > > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > > > > > > > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 <<
> > > > > > > > 26)
> > > > > > > > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 <<
> > > > > > > > 26)
> > > > > > > > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > > > > > > > -#define  VDIP_ENABLE_PPS		(1 << 24)
> > > 
> > > Why do you need to remove all of these defs?
> > > From the spec, the only shift thats wrong is :
> > > VSC_SELECT_MASK             (0x3 << 26)
> > > #define  VSC_SELECT_SHIFT            26
> > > #define  VSC_DIP_HW_HEA_DATA         (0 << 26)
> > > define  VSC_DIP_HW_HEA_SW_DATA              (1 << 26)
> > > define  VSC_DIP_HW_DATA_SW_HEA              (2 << 26)
> > > VSC_DIP_SW_HEA_DATA         (3 << 26)
> > > 
> > > Removing others and redefining to be the same is misleading
> > 
> > Please see other definitions in the file and the documentation
> > above,
> > we generally organize bit definitions in the descending order of
> > their
> > position.
> > 
> 
> I still dont see why you had remove earlier defs and redefine since
> it was already organized in descending order of their position in the
> originl
> patches starting with DRM_DIP_ENABLE with 1<< 28 all the way to
> VDIP_ENABLE_PPS with
> 1 << 24 and your changes keeps the same order with the fix of VSC
> HSIFT from 26 to 25.
> So only those VSC defs should be changed not the entire set.
It was defined below (1 << 0)



> 
> Manasi
>  
> > 
> > > 
> > > Other than that the actual fixes look good, double checked with
> > > the
> > > spec.
> > > So after the above fix and the commit message fix
> > > 
> > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> > > 
> > > Manasi
> > > 
> > > > > > > >  
> > > > > > > >  /* Panel power sequencing */
> > > > > > > >  #define PPS_BASE			0x61200
> > > > > > > > -- 
> > > > > > > > 2.14.1
> > > > > > > > 
> > > > > > > > _______________________________________________
> > > > > > > > Intel-gfx mailing list
> > > > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > > > 
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Rodrigo Vivi]

On Thu, Oct 04, 2018 at 04:03:05PM -0700, Lucas De Marchi wrote:
> On Thu, Oct 04, 2018 at 01:51:49PM -0700, Dhinakaran Pandiyan wrote:
> > The shifts for VSC_SELECT bits are wrong, fix it. Good thing is the
> > definitions are unused.
> 
> If they are unused why are we fixing them instead of removing? Or better,
> why did we add them?

Sorry, my bad... I assumed Manasi's work depending on this would land sooner.
But this should actually be part of Manasi's series instead of merging.

Manasi is still working on that. So let's fix. Maybe it was a good thing
having in the tree... got more attention and fix than during reviews ;)
/me runs

> 
> Lucas De Marchi
> 
> > 
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP registers")
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 27e650fe591b..a0ad77b9212b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4584,6 +4584,15 @@ enum {
> >  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
> >  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
> >  /* HSW and later: */
> > +#define   DRM_DIP_ENABLE		(1 << 28)
> > +#define   PSR_VSC_BIT_7_SET		(1 << 27)
> > +#define   VSC_SELECT_MASK		(0x3 << 25)
> > +#define   VSC_SELECT_SHIFT		25
> > +#define   VSC_DIP_HW_HEA_DATA		(0 << 25)
> > +#define   VSC_DIP_HW_HEA_SW_DATA	(1 << 25)
> > +#define   VSC_DIP_HW_DATA_SW_HEA	(2 << 25)
> > +#define   VSC_DIP_SW_HEA_DATA		(3 << 25)
> > +#define   VDIP_ENABLE_PPS		(1 << 24)
> >  #define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
> >  #define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
> >  #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
> > @@ -4591,15 +4600,6 @@ enum {
> >  #define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
> >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> >  
> > -#define  DRM_DIP_ENABLE			(1 << 28)
> > -#define  PSR_VSC_BIT_7_SET		(1 << 27)
> > -#define  VSC_SELECT_MASK		(0x3 << 26)
> > -#define  VSC_SELECT_SHIFT		26
> > -#define  VSC_DIP_HW_HEA_DATA		(0 << 26)
> > -#define  VSC_DIP_HW_HEA_SW_DATA		(1 << 26)
> > -#define  VSC_DIP_HW_DATA_SW_HEA		(2 << 26)
> > -#define  VSC_DIP_SW_HEA_DATA		(3 << 26)
> > -#define  VDIP_ENABLE_PPS		(1 << 24)
> >  
> >  /* Panel power sequencing */
> >  #define PPS_BASE			0x61200
> > -- 
> > 2.14.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Fix VIDEO_DIP_CTL bit shifts

[Patchwork]

== Series Details ==

Series: drm/i915: Fix VIDEO_DIP_CTL bit shifts
URL   : https://patchwork.freedesktop.org/series/50573/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4932_full -> Patchwork_10365_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10365_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10365_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10365_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_10365_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956) +3

    igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-b:
      shard-apl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +1

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-glk:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-128x42-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-apl:          PASS -> FAIL (fdo#103191, fdo#103232) +1
      shard-kbl:          PASS -> FAIL (fdo#103191, fdo#103232)

    igt@kms_fbcon_fbt@psr:
      shard-skl:          NOTRUN -> FAIL (fdo#107882)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-skl:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@plain-flip-fb-recreate:
      shard-skl:          NOTRUN -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-wc:
      shard-skl:          PASS -> FAIL (fdo#105682)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
      shard-glk:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-move:
      shard-skl:          PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    {igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb}:
      shard-apl:          PASS -> FAIL (fdo#108145)
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +3

    {igt@kms_plane_alpha_blend@pipe-a-coverage-7efc}:
      shard-skl:          PASS -> FAIL (fdo#108145)

    {igt@kms_plane_alpha_blend@pipe-c-alpha-7efc}:
      shard-skl:          NOTRUN -> FAIL (fdo#108146)

    {igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb}:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166) +1

    igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)
      shard-hsw:          PASS -> FAIL (fdo#99912)

    igt@kms_vblank@pipe-c-wait-busy:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +10

    igt@pm_rpm@cursor-dpms:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807) +1

    
    ==== Possible fixes ====

    igt@drv_suspend@fence-restore-untiled:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@gem_cpu_reloc@full:
      shard-skl:          INCOMPLETE (fdo#108073) -> PASS

    igt@gem_userptr_blits@readonly-unsync:
      shard-skl:          INCOMPLETE (fdo#108074) -> PASS

    igt@kms_color@pipe-a-ctm-max:
      shard-apl:          FAIL (fdo#108147) -> PASS

    igt@kms_cursor_crc@cursor-64x21-sliding:
      shard-apl:          FAIL (fdo#103232) -> PASS +2

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          DMESG-WARN (fdo#106538, fdo#105763) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-gtt:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
      shard-glk:          FAIL (fdo#103167) -> PASS +1

    igt@kms_plane@plane-position-covered-pipe-b-planes:
      shard-glk:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@pm_rpm@modeset-lpsp-stress-no-wait:
      shard-skl:          INCOMPLETE (fdo#107807) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107882 https://bugs.freedesktop.org/show_bug.cgi?id=107882
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108073 https://bugs.freedesktop.org/show_bug.cgi?id=108073
  fdo#108074 https://bugs.freedesktop.org/show_bug.cgi?id=108074
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
  fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4932 -> Patchwork_10365

  CI_DRM_4932: 21f90148bf7adb33d82580013a5697a6bbb88248 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4669: 5f40e617cd9c1e089f4a2d79c53a417d891e3e3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10365: dabd694408ac19e4f366640c15665211e4de0e53 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10365/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx