[Intel-gfx] [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 at intel.com>
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > Fixes: 7af2be6d54d4 ("drm/i915/icl: Add VIDEO_DIP
> > > > > > registers")
> > > > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > > > dhinakaran.pandiyan at 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 at intel.com>
> 
> Manasi
> 
> > > > > >  
> > > > > >  /* Panel power sequencing */
> > > > > >  #define PPS_BASE			0x61200
> > > > > > -- 
> > > > > > 2.14.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx