diff mbox series

drm/i915: Fix VIDEO_DIP_CTL bit shifts

Message ID 20181004205149.9286-1-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Fix VIDEO_DIP_CTL bit shifts | expand

Commit Message

Dhinakaran Pandiyan Oct. 4, 2018, 8:51 p.m. UTC 
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(-)

Comments

Lucas De Marchi Oct. 4, 2018, 11:03 p.m. UTC | #1 
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
Dhinakaran Pandiyan Oct. 4, 2018, 11:13 p.m. UTC | #2 
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
Rodrigo Vivi Oct. 4, 2018, 11:24 p.m. UTC | #3 
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
Navare, Manasi Oct. 4, 2018, 11:28 p.m. UTC | #4 
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
>
Dhinakaran Pandiyan Oct. 5, 2018, midnight UTC | #5 
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
Navare, Manasi Oct. 5, 2018, 12:27 a.m. UTC | #6 
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
>
Dhinakaran Pandiyan Oct. 5, 2018, 2:56 a.m. UTC | #7 
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
Navare, Manasi Oct. 5, 2018, 6:10 p.m. UTC | #8 
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
>
Dhinakaran Pandiyan Oct. 5, 2018, 6:33 p.m. UTC | #9 
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
diff mbox series

Patch

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