<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 10, 2017 at 9:04 AM, Ville Syrjälä <span dir="ltr"><<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jan 09, 2017 at 11:20:57AM -0800, Jason Ekstrand wrote:<br>
> On Thu, Jan 5, 2017 at 7:14 AM, <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a><wbr>> wrote:<br>
><br>
> > From: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a><wbr>><br>
> ><br>
> > SKL+ display engine can scan out certain kinds of compressed surfaces<br>
> > produced by the render engine. This involved telling the display engine<br>
> > the location of the color control surfae (CCS) which describes<br>
> > which parts of the main surface are compressed and which are not. The<br>
> > location of CCS is provided by userspace as just another plane with its<br>
> > own offset.<br>
> ><br>
> > Add the required stuff to validate the user provided AUX plane metadata<br>
> > and convert the user provided linear offset into something the hardware<br>
> > can consume.<br>
> ><br>
> > Due to hardware limitations we require that the main surface and<br>
> > the AUX surface (CCS) be part of the same bo. The hardware also<br>
> > makes life hard by not allowing you to provide separate x/y offsets<br>
> > for the main and AUX surfaces (excpet with NV12), so finding suitable<br>
> > offsets for both requires a bit of work. Assuming we still want keep<br>
> > playing tricks with the offsets. I've just gone with a dumb "search<br>
> > backward for suitable offsets" approach, which is far from optimal,<br>
> > but it works.<br>
> ><br>
> > Also not all planes will be capable of scanning out compressed surfaces,<br>
> > and eg. 90/270 degree rotation is not supported in combination with<br>
> > decompression either.<br>
> ><br>
> > This patch may contain work from at least the following people:<br>
> > * Vandana Kannan <<a href="mailto:vandana.kannan@intel.com">vandana.kannan@intel.com</a>><br>
> > * Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>><br>
> > * Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>><br>
> ><br>
> > v2: Deal with display workarounds 0390, 0531, 1125 (Paulo)<br>
> ><br>
> > Cc: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>><br>
> > Cc: Vandana Kannan <<a href="mailto:vandana.kannan@intel.com">vandana.kannan@intel.com</a>><br>
> > Cc: Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>><br>
> > Cc: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>><br>
> > Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br>
> > Signed-off-by: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a><wbr>><br>
> > ---<br>
> >  drivers/gpu/drm/i915/i915_reg.<wbr>h      |  23 ++++<br>
> >  drivers/gpu/drm/i915/intel_<wbr>display.c | 234 ++++++++++++++++++++++++++++++<br>
> > ++---<br>
> >  drivers/gpu/drm/i915/intel_pm.<wbr>c      |  29 ++++-<br>
> >  drivers/gpu/drm/i915/intel_<wbr>sprite.c  |   5 +<br>
> >  4 files changed, 274 insertions(+), 17 deletions(-)<br>
> ><br>
> > diff --git a/drivers/gpu/drm/i915/i915_<wbr>reg.h b/drivers/gpu/drm/i915/i915_<br>
> > reg.h<br>
> > index 00970aa77afa..6849ba93f1d9 100644<br>
> > --- a/drivers/gpu/drm/i915/i915_<wbr>reg.h<br>
> > +++ b/drivers/gpu/drm/i915/i915_<wbr>reg.h<br>
> > @@ -6209,6 +6209,28 @@ enum {<br>
> >                         _ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \<br>
> >                         _ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B))<br>
> ><br>
> > +#define PLANE_AUX_DIST_1_A             0x701c0<br>
> > +#define PLANE_AUX_DIST_2_A             0x702c0<br>
> > +#define PLANE_AUX_DIST_1_B             0x711c0<br>
> > +#define PLANE_AUX_DIST_2_B             0x712c0<br>
> > +#define _PLANE_AUX_DIST_1(pipe) \<br>
> > +                       _PIPE(pipe, PLANE_AUX_DIST_1_A, PLANE_AUX_DIST_1_B)<br>
> > +#define _PLANE_AUX_DIST_2(pipe) \<br>
> > +                       _PIPE(pipe, PLANE_AUX_DIST_2_A, PLANE_AUX_DIST_2_B)<br>
> > +#define PLANE_AUX_DIST(pipe, plane)     \<br>
> > +       _MMIO_PLANE(plane, _PLANE_AUX_DIST_1(pipe),<br>
> > _PLANE_AUX_DIST_2(pipe))<br>
> > +<br>
> > +#define PLANE_AUX_OFFSET_1_A           0x701c4<br>
> > +#define PLANE_AUX_OFFSET_2_A           0x702c4<br>
> > +#define PLANE_AUX_OFFSET_1_B           0x711c4<br>
> > +#define PLANE_AUX_OFFSET_2_B           0x712c4<br>
> > +#define _PLANE_AUX_OFFSET_1(pipe)       \<br>
> > +               _PIPE(pipe, PLANE_AUX_OFFSET_1_A, PLANE_AUX_OFFSET_1_B)<br>
> > +#define _PLANE_AUX_OFFSET_2(pipe)       \<br>
> > +               _PIPE(pipe, PLANE_AUX_OFFSET_2_A, PLANE_AUX_OFFSET_2_B)<br>
> > +#define PLANE_AUX_OFFSET(pipe, plane)   \<br>
> > +       _MMIO_PLANE(plane, _PLANE_AUX_OFFSET_1(pipe),<br>
> > _PLANE_AUX_OFFSET_2(pipe))<br>
> > +<br>
> >  /* legacy palette */<br>
> >  #define _LGC_PALETTE_A           0x4a000<br>
> >  #define _LGC_PALETTE_B           0x4a800<br>
> > @@ -6433,6 +6455,7 @@ enum {<br>
> >  # define CHICKEN3_DGMG_DONE_FIX_DISABLE                (1 << 2)<br>
> ><br>
> >  #define CHICKEN_PAR1_1         _MMIO(0x42080)<br>
> > +#define  SKL_RC_HASH_OUTSIDE   (1 << 15)<br>
> >  #define  DPA_MASK_VBLANK_SRD   (1 << 15)<br>
> >  #define  FORCE_ARB_IDLE_PLANES (1 << 14)<br>
> >  #define  SKL_EDP_PSR_FIX_RDWRAP        (1 << 3)<br>
> > diff --git a/drivers/gpu/drm/i915/intel_<wbr>display.c<br>
> > b/drivers/gpu/drm/i915/intel_<wbr>display.c<br>
> > index 38de9df0ec60..2236abebd8bc 100644<br>
> > --- a/drivers/gpu/drm/i915/intel_<wbr>display.c<br>
> > +++ b/drivers/gpu/drm/i915/intel_<wbr>display.c<br>
> > @@ -2064,11 +2064,19 @@ intel_tile_width_bytes(const struct<br>
> > drm_framebuffer *fb, int plane)<br>
> >                         return 128;<br>
> >                 else<br>
> >                         return 512;<br>
> > +       case I915_FORMAT_MOD_Y_TILED_CCS:<br>
> > +               if (plane == 1)<br>
> > +                       return 64;<br>
> > +               /* fall through */<br>
> >         case I915_FORMAT_MOD_Y_TILED:<br>
> >                 if (IS_GEN2(dev_priv) || HAS_128_BYTE_Y_TILING(dev_<wbr>priv))<br>
> >                         return 128;<br>
> >                 else<br>
> >                         return 512;<br>
> > +       case I915_FORMAT_MOD_Yf_TILED_CCS:<br>
> > +               if (plane == 1)<br>
> > +                       return 64;<br>
> ><br>
><br>
> I still think a CCS tile is 128B wide. :-)<br>
<br>
</div></div>The spec kinda suggests the same. But I still couldn't figure out where<br>
that notion really came from, so I just went with the value that gave<br>
me the expected result on my screen. That is writing 64 bytes into the<br>
tile is exactly what's required to fill a single row/column, writing<br>
more would wrap.<br></blockquote><div><br></div><div>Yeah.  Ultimately it doesn't matter.  It's an arbitrary number userspace multiplies by and the kernel divides by.  We just need to settle on something sensible.  In userspace, we've more-or-less settled on 128 to match the docs (and what the hardware does for W-tiling and some other edge cases) but it's not a huge deal.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
><br>
><br>
> > +               /* fall through */<br>
> >         case I915_FORMAT_MOD_Yf_TILED:<br>
> >                 /*<br>
> >                  * Bspec seems to suggest that the Yf tile width would<br>
> > @@ -2156,7 +2164,7 @@ static unsigned int intel_surf_alignment(const<br>
> > struct drm_framebuffer *fb,<br>
> >         struct drm_i915_private *dev_priv = to_i915(fb->dev);<br>
> ><br>
> >         /* AUX_DIST needs only 4K alignment */<br>
> > -       if (fb->format->format == DRM_FORMAT_NV12 && plane == 1)<br>
> > +       if (plane == 1)<br>
> >                 return 4096;<br>
> ><br>
> >         switch (fb->modifier) {<br>
> > @@ -2166,6 +2174,8 @@ static unsigned int intel_surf_alignment(const<br>
> > struct drm_framebuffer *fb,<br>
> >                 if (INTEL_GEN(dev_priv) >= 9)<br>
> >                         return 256 * 1024;<br>
> >                 return 0;<br>
> > +       case I915_FORMAT_MOD_Y_TILED_CCS:<br>
> > +       case I915_FORMAT_MOD_Yf_TILED_CCS:<br>
> >         case I915_FORMAT_MOD_Y_TILED:<br>
> >         case I915_FORMAT_MOD_Yf_TILED:<br>
> >                 return 1 * 1024 * 1024;<br>
> > @@ -2472,6 +2482,7 @@ static unsigned int intel_fb_modifier_to_tiling(<wbr>uint64_t<br>
> > fb_modifier)<br>
> >         case I915_FORMAT_MOD_X_TILED:<br>
> >                 return I915_TILING_X;<br>
> >         case I915_FORMAT_MOD_Y_TILED:<br>
> > +       case I915_FORMAT_MOD_Y_TILED_CCS:<br>
> >                 return I915_TILING_Y;<br>
> >         default:<br>
> >                 return I915_TILING_NONE;<br>
> > @@ -2536,6 +2547,35 @@ intel_fill_fb_info(struct drm_i915_private<br>
> > *dev_priv,<br>
> ><br>
> >                 intel_fb_offset_to_xy(&x, &y, fb, i);<br>
> ><br>
> > +               if ((fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||<br>
> > +                    fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS) && i ==<br>
> > 1) {<br>
> > +                       int main_x, main_y;<br>
> > +                       int ccs_x, ccs_y;<br>
> > +<br>
> > +                       /*<br>
> > +                        * Each byte of CCS corresponds to a 16x8 area of<br>
> > the main surface, and<br>
> > +                        * each CCS tile is 64x64 bytes.<br>
> > +                        */<br>
> > +                       ccs_x = (x * 16) % (64 * 16);<br>
> > +                       ccs_y = (y * 8) % (64 * 8);<br>
> ><br>
><br>
> This makes me nervous.  Why are we multiplying CCS coordinates by something<br>
> before we do the modulus?  Why aren't coordinates in both surfaces in<br>
> pixels?<br>
<br>
</div></div>For converting the linear offset (which is in bytes) into x/y we just<br>
consider each pixel to be 1 byte. Hence to get the corresponding pixel<br>
coordinates we multiply the byte based coordinates by 16x8. We can't<br>
really deal with <1 byte pixels in most places.<span class=""><br></span></blockquote><div><br></div><div>So are the x/y offsets provided by userspace in bytes or pixels for regular color surfaces?  I had kind-of assumed pixels, but I guess I could also see bytes.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> So long as you keep things in pixesl and know that a CCS tile is<br>
> 1024x512px and a color tile is 32x32 pixels, you can safely do tile<br>
> offsetting and it all makes sense.  Having different units looks like a<br>
> recipe for some very confusing bugs.  Am I just completely misunderstanding<br>
> what's going on here?<br>
<br>
</span>Doing things in pixels would involve totally custom code for the CCS.<br>
By thinking of CCS as having 1 byte pixels we can share the code already<br>
used for everything else (apart from this one special check which is<br>
really only necessary because the HW ignores the AUX x/y offsets for CCS.<br>
<br>
I suppose it would be possible to rewrite a bunch of other things to<br>
allow <1 byte pixels but I couldn't be bothered to go there.<br>
</blockquote></div><br></div><div class="gmail_extra">I think I need a better mental model of what the X/Y offset API looks like before I can reply to that properly.<br></div></div>