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