On Fri, Jul 20, 2018 at 08:28:01PM +0000, Srivatsa, Anusha wrote:
>
>
> >-----Original Message-----
> >From: Navare, Manasi D
> >Sent: Friday, July 20, 2018 1:29 PM
> >To: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> >Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
> >gfx at lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [v2] drm/i915/dsc: Add missing _MMIO() from PPS
> >registers
> >
> >On Fri, Jul 20, 2018 at 01:13:07PM -0700, Rodrigo Vivi wrote:
> >> On Fri, Jul 20, 2018 at 12:25:47PM -0700, Manasi Navare wrote:
> >> > Looks good to me now and also tested with the patches that use these
> >registers.
> >> >
> >> > Reviewed-by: Manasi Navare <manasi.d.navare at intel.com>
> >> >
> >> > Manasi
> >> >
> >> > On Fri, Jul 20, 2018 at 12:10:39PM -0700, Anusha Srivatsa wrote:
> >> > > FIXME: This fixes the patch:
> >>
> >> "FIXME:" tag is misleading here...
> >> It looks like this patch needs to be fixed.
> >>
> >> > > Link:
> >> > > https://patchwork.freedesktop.org/patch/msgid/1531861861-10950-2-g
> >> > > it-send-email-anusha.srivatsa at intel.com
> >>
> >> "Link:" is misleading here... Scripts might think this is the link to
> >> this patch here...
> >>
> >> Please replace these 2 above lines with a direct text that explains
> >> the issue
> >>
> >> Whenever mentioning another commit please use the kernel style:
> >>
> >> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
> >>
> >> "If you want to refer to a specific commit, don’t just refer to the SHA-1 ID of
> >the commit. Please also include the oneline summary of the commit, to make it
> >easier for reviewers to know what it is about"
> >>
> >> In this case: commit 2efbb2f099fb ("i915/dp/dsc: Add DSC PPS register
> >> definitions")
> >
> >Yes I agree, it should mention the comit ID that is getting fixed.
> >
> >>
> >> > >
> >> > > Which did not have _MMIO() for DSCA and DSCC.
> >> > >
> >> > > v2: Fix typos. (manasi)
> >>
> >> Maybe instead of "FIXME:" you wanted the "Fixes:"
> >>
> >> In this case please make sure you follow the "Fixes:" rules:
> >>
> >> https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html
> >>
> >> On this case I'm not 100% confident we want the "Fixes:"
> >>
> >> Part of me says no, we don't need Fixes because ICL is on
> >> alpha_support protection mode, and these patches are just adding the
> >> definition and not in use yet.
> >>
> >> Part of me says yes, we need because I just sent the initial patches
> >> to 4.19 and Fixme tag would make sure we get this on
> >> drm-intel-next-fixes still for 4.19. So whatever works use this on
> >> 4.20 could be easily backported to 4.19 by OSVs...
> >>
> >> So, please read the rules and help me to decide. ;)
> >
> >May be a good idea to just say in the commit message that it fixes the reg defs
> >and typos etc.. instead of a formal FIXES tag.
> Thinking....
> Rodrigo, having FIXES tag is going to help you cherry-pick though, correct?
Yes, if "Fixes:" (non capital) is there, dim scripts will get this patch
for drm-intel-next-fixes and it will probably be part of 4.19.
Otherwise I won't cherry-pick and it will be only part of 4.20
along with the rest of DSC patches.
But have in mind that On a regular fixes flow I'd prefer without the tag so
we minimize the non-functional-fixes on the flow... After all this is not
fixing any real 4.19 issue. ;)
>
> Anusha
> >Manasi
> >
> >>
> >> Thanks,
> >> Rodrigo.
> >>
> >> > >
> >> > > Cc: Rodrigi Vivi <rodrigo.vivi at intel.com>
> >> > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> >> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> >> > > ---
> >> > > drivers/gpu/drm/i915/i915_reg.h | 76
> >> > > ++++++++++++++++++++---------------------
> >> > > 1 file changed, 38 insertions(+), 38 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> > > b/drivers/gpu/drm/i915/i915_reg.h index 8af945d..7394605 100644
> >> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > > @@ -10349,8 +10349,8 @@ enum skl_power_gate {
> >> > > #define ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN (1 << 23)
> >> > >
> >> > > /* Icelake Display Stream Compression Registers */
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_0 0x6B200
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_0 0x6BA00
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_0
> > _MMIO(0x6B200)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_0
> > _MMIO(0x6BA00)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB 0x78270
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB 0x78370
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC 0x78470
> >> > > @@ -10370,8 +10370,8 @@ enum skl_power_gate {
> >> > > #define DSC_VER_MIN_SHIFT 4
> >> > > #define DSC_VER_MAJ (0x1 << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_1 0x6B204
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_1 0x6BA04
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_1
> > _MMIO(0x6B204)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_1
> > _MMIO(0x6BA04)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_1_PB 0x78274
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_1_PB 0x78374
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_1_PC 0x78474
> >> > > @@ -10384,8 +10384,8 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_1_PC)
> >> > > #define DSC_BPP(bpp) ((bpp) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_2 0x6B208
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_2 0x6BA08
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_2
> > _MMIO(0x6B208)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_2
> > _MMIO(0x6BA08)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_2_PB 0x78278
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_2_PB 0x78378
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_2_PC 0x78478
> >> > > @@ -10399,8 +10399,8 @@ enum skl_power_gate {
> >> > > #define DSC_PIC_WIDTH(pic_width) ((pic_width) << 16)
> >> > > #define DSC_PIC_HEIGHT(pic_height) ((pic_height) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_3 0x6B20C
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_3 0x6BA0C
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_3
> > _MMIO(0x6B20C)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_3
> > _MMIO(0x6BA0C)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_3_PB 0x7827C
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_3_PB 0x7837C
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_3_PC 0x7847C
> >> > > @@ -10414,8 +10414,8 @@ enum skl_power_gate {
> >> > > #define DSC_SLICE_WIDTH(slice_width) ((slice_width) << 16)
> >> > > #define DSC_SLICE_HEIGHT(slice_height) ((slice_height) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_4 0x6B210
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_4 0x6BA10
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_4
> > _MMIO(0x6B210)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_4
> > _MMIO(0x6BA10)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_4_PB 0x78280
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_4_PB 0x78380
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_4_PC 0x78480
> >> > > @@ -10429,8 +10429,8 @@ enum skl_power_gate {
> >> > > #define DSC_INITIAL_DEC_DELAY(dec_delay) ((dec_delay) << 16)
> >> > > #define DSC_INITIAL_XMIT_DELAY(xmit_delay) ((xmit_delay) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_5 0x6B214
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_5 0x6BA14
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_5
> > _MMIO(0x6B214)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_5
> > _MMIO(0x6BA14)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_5_PB 0x78284
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_5_PB 0x78384
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_5_PC 0x78484
> >> > > @@ -10441,11 +10441,11 @@ enum skl_power_gate {
> >> > > #define ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe)
> > _MMIO_PIPE((pipe) - PIPE_B, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_5_PC, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_5_PC)
> >> > > -#define DSC_SCALE_DEC_INTINT(scale_dec) ((scale_dec) << 16)
> >> > > +#define DSC_SCALE_DEC_INT(scale_dec) ((scale_dec) << 16)
> >> > > #define DSC_SCALE_INC_INT(scale_inc) ((scale_inc) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_6 0x6B218
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_6 0x6BA18
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_6
> > _MMIO(0x6B218)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_6
> > _MMIO(0x6BA18)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_6_PB 0x78288
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_6_PB 0x78388
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_6_PC 0x78488
> >> > > @@ -10456,13 +10456,13 @@ enum skl_power_gate {
> >> > > #define ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe)
> > _MMIO_PIPE((pipe) - PIPE_B, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_6_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_6_PC)
> >> > > -#define DSC_FLATNESS_MAX_QP(max_qp) (qp << 24)
> >> > > -#define DSC_FLATNESS_MIN_QP(min_qp) (qp << 16)
> >> > > +#define DSC_FLATNESS_MAX_QP(max_qp) ((max_qp) <<
> >24)
> >> > > +#define DSC_FLATNESS_MIN_QP(min_qp) ((min_qp) << 16)
> >> > > #define DSC_FIRST_LINE_BPG_OFFSET(offset) ((offset) << 8)
> >> > > #define DSC_INITIAL_SCALE_VALUE(value) ((value) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_7 0x6B21C
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_7 0x6BA1C
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_7
> > _MMIO(0x6B21C)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_7
> > _MMIO(0x6BA1C)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_7_PB 0x7828C
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_7_PB 0x7838C
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_7_PC 0x7848C
> >> > > @@ -10476,8 +10476,8 @@ enum skl_power_gate {
> >> > > #define DSC_NFL_BPG_OFFSET(bpg_offset) ((bpg_offset)
> ><< 16)
> >> > > #define DSC_SLICE_BPG_OFFSET(bpg_offset) ((bpg_offset) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_8 0x6B220
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_8 0x6BA20
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_8
> > _MMIO(0x6B220)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_8
> > _MMIO(0x6BA20)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_8_PB 0x78290
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_8_PB 0x78390
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_8_PC 0x78490
> >> > > @@ -10491,8 +10491,8 @@ enum skl_power_gate {
> >> > > #define DSC_INITIAL_OFFSET(initial_offset) ((initial_offset)
> ><< 16)
> >> > > #define DSC_FINAL_OFFSET(final_offset) ((final_offset)
> ><< 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_9 0x6B224
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_9 0x6BA24
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_9
> > _MMIO(0x6B224)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_9
> > _MMIO(0x6BA24)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_9_PB 0x78294
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_9_PB 0x78394
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_9_PC 0x78494
> >> > > @@ -10506,8 +10506,8 @@ enum skl_power_gate {
> >> > > #define DSC_RC_EDGE_FACTOR(rc_edge_fact) ((rc_edge_fact) << 16)
> >> > > #define DSC_RC_MODEL_SIZE(rc_model_size) ((rc_model_size) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_10 0x6B228
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_10 0x6BA28
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_10
> > _MMIO(0x6B228)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_10
> > _MMIO(0x6BA28)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_10_PB 0x78298
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_10_PB 0x78398
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_10_PC 0x78498
> >> > > @@ -10523,8 +10523,8 @@ enum skl_power_gate {
> >> > > #define DSC_RC_QUANT_INC_LIMIT1(lim) ((lim) << 8)
> >> > > #define DSC_RC_QUANT_INC_LIMIT0(lim) ((lim) << 0)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_11 0x6B22C
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_11 0x6BA2C
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_11
> > _MMIO(0x6B22C)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_11
> > _MMIO(0x6BA2C)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_11_PB 0x7829C
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_11_PB 0x7839C
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_11_PC 0x7849C
> >> > > @@ -10536,8 +10536,8 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_11_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_11_PC)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_12 0x6B260
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_12 0x6BA60
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_12
> > _MMIO(0x6B260)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_12
> > _MMIO(0x6BA60)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_12_PB 0x782A0
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_12_PB 0x783A0
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_12_PC 0x784A0
> >> > > @@ -10549,8 +10549,8 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_12_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_12_PC)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_13 0x6B264
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_13 0x6BA64
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_13
> > _MMIO(0x6B264)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_13
> > _MMIO(0x6BA64)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_13_PB 0x782A4
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_13_PB 0x783A4
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_13_PC 0x784A4
> >> > > @@ -10562,8 +10562,8 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_13_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_13_PC)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_14 0x6B268
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_14 0x6BA68
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_14
> > _MMIO(0x6B268)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_14
> > _MMIO(0x6BA68)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_14_PB 0x782A8
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_14_PB 0x783A8
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_14_PC 0x784A8
> >> > > @@ -10575,8 +10575,8 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_14_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_14_PC)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_15 0x6B26C
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_15 0x6BA6C
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_15
> > _MMIO(0x6B26C)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_15
> > _MMIO(0x6BA6C)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_15_PB 0x782AC
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_15_PB 0x783AC
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_15_PC 0x784AC
> >> > > @@ -10588,8 +10588,8 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_15_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_15_PC)
> >> > >
> >> > > -#define DSCA_PICTURE_PARAMETER_SET_16 0x6B270
> >> > > -#define DSCC_PICTURE_PARAMETER_SET_16 0x6BA70
> >> > > +#define DSCA_PICTURE_PARAMETER_SET_16
> > _MMIO(0x6B270)
> >> > > +#define DSCC_PICTURE_PARAMETER_SET_16
> > _MMIO(0x6BA70)
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_16_PB 0x782B0
> >> > > #define _ICL_DSC1_PICTURE_PARAMETER_SET_16_PB 0x783B0
> >> > > #define _ICL_DSC0_PICTURE_PARAMETER_SET_16_PC 0x784B0
> >> > > @@ -10601,7 +10601,7 @@ enum skl_power_gate {
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_16_PB, \
> >> > >
> >_ICL_DSC1_PICTURE_PARAMETER_SET_16_PC)
> >> > > #define DSC_SLICE_PER_LINE(slice_per_line) ((slice_per_line)
> ><< 16)
> >> > > -#define DSC_SLICE_CHUNK_SIZE(slice_chunk_aize)
> > (slice_chunk_size << 0)
> >> > > +#define DSC_SLICE_CHUNK_SIZE(slice_chunk_size)
> > ((slice_chunk_size) << 0)
> >> > >
> >> > > /* Icelake Rate Control Buffer Threshold Registers */
> >> > > #define DSCA_RC_BUF_THRESH_0 _MMIO(0x6B230)
> >> > > --
> >> > > 2.7.4
> >> > >
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx at lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx