On Wed, Oct 21, 2015 at 03:09:55PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 21, 2015 at 12:15:42PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 14, 2015 at 07:28:55PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Pull the tile width calculations from intel_fb_stride_alignment() into a
> > > new function intel_tile_width().
> > >
> > > Also take the opportunity to pass aroun dev_priv instead of dev to
> > > intel_fb_stride_alignment().
> > >
> > > v2: Reorder argumnents to be more consistent with other functions
> > > Change intel_fb_stride_alignment() to accept dev_priv instead of dev
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++-------------
> > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > drivers/gpu/drm/i915/intel_sprite.c | 2 +-
> > > 3 files changed, 51 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 6add8d1..c31fe47 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2214,6 +2214,41 @@ static bool need_vtd_wa(struct drm_device *dev)
> > > return false;
> > > }
> > >
> > > +static unsigned int intel_tile_width(const struct drm_i915_private *dev_priv,
> > > + uint64_t fb_modifier, unsigned int cpp)
> > > +{
> > > + switch (fb_modifier) {
> > > + case DRM_FORMAT_MOD_NONE:
> > > + return cpp;
> > > + case I915_FORMAT_MOD_X_TILED:
> > > + return IS_GEN2(dev_priv) ? 128 : 512;
> > > + case I915_FORMAT_MOD_Y_TILED:
> > > + /* No need to check for old gens and Y tiling since this is
> > > + * about the display engine and those will be blocked before
> > > + * we get here.
> > > + */
> > > + return 128;
> >
> > Imo just drop the comment and change this to
> >
> > return HAS_128_BYTE_Y_TILING(dev) 128 : 512;
> >
> > Just to avoid wtf moments. It's less code than the comment too.
>
> Yeah, can do. And we definitely need it if we want to use this for gem
> stuff. Although we need to change HAS_128_BYTE_Y_TILING() to include
> gen2. Currently that define actually means "has 128x32 Y tiling". Or
> we make intel_tile_width() check for 'IS_GEN2 || HAS_128_BYTE_Y_TILING'.
Oops, yeah.
> > Another one: Enforcing pot alignment for gen2/3 is fairly hapzardous - we
> > do it accidentally by requiring that for X-tiled the underlying obj is
> > also X-tiled and that strides match. Comment for this would be good here.
> >
> > > + case I915_FORMAT_MOD_Yf_TILED:
> > > + switch (cpp) {
> > > + case 1:
> > > + return 64;
> > > + case 2:
> > > + case 4:
> > > + return 128;
> >
> > With 4 cpp we have 4x4 pixels for the 64b cacheline. So 8x8 for the 256b
> > block with 1:1 aspect ratio, hence 64x64 pixel tile for the full page.
>
> 32x32 for the full page
>
> >
> > > + case 8:
> >
> > With 8 cpp we have 2x4 pixels for the 64b cacheline. So 8x4 for the 256b
> > block with 2:1 aspect ratio, hence 64x32 pixel tile for the full page.
>
> 32x16 for the full page
>
> >
> > > + case 16:
> > > + return 256;
> >
> > With 16 cpp we have 1x4 pixels for the 64b cacheline. So 4x4 for the 256b
> > block with 1:1 aspect ratio, hence 32x32 pixel tile for the full page.
>
> 16x16 for the full page
Yeah I can't compute. But the real trouble was that I thought this was in
pixels, but it's in bytes ;-) I clarified that later on and r-b with these
numbers here.
-Daniel
>
> >
> > Otherwise looks good to me, Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > once we close the above.
> >
> > Cheers, Daniel
> >
> > > + default:
> > > + MISSING_CASE(cpp);
> > > + return cpp;
> > > + }
> > > + break;
> > > + default:
> > > + MISSING_CASE(fb_modifier);
> > > + return cpp;
> > > + }
> > > +}
> > > +
> > > unsigned int
> > > intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
> > > uint64_t fb_format_modifier, unsigned int plane)
> > > @@ -2895,37 +2930,20 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > > POSTING_READ(reg);
> > > }
> > >
> > > -u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > > - uint32_t pixel_format)
> > > +u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> > > + uint64_t fb_modifier, uint32_t pixel_format)
> > > {
> > > - u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> > > -
> > > /*
> > > * The stride is either expressed as a multiple of 64 bytes
> > > * chunks for linear buffers or in number of tiles for tiled
> > > * buffers.
> > > */
> > > - switch (fb_modifier) {
> > > - case DRM_FORMAT_MOD_NONE:
> > > - return 64;
> > > - case I915_FORMAT_MOD_X_TILED:
> > > - if (INTEL_INFO(dev)->gen == 2)
> > > - return 128;
> > > - return 512;
> > > - case I915_FORMAT_MOD_Y_TILED:
> > > - /* No need to check for old gens and Y tiling since this is
> > > - * about the display engine and those will be blocked before
> > > - * we get here.
> > > - */
> > > - return 128;
> > > - case I915_FORMAT_MOD_Yf_TILED:
> > > - if (bits_per_pixel == 8)
> > > - return 64;
> > > - else
> > > - return 128;
> > > - default:
> > > - MISSING_CASE(fb_modifier);
> > > + if (fb_modifier == DRM_FORMAT_MOD_NONE) {
> > > return 64;
> > > + } else {
> > > + unsigned int cpp = drm_format_plane_cpp(pixel_format, 0);
> > > +
> > > + return intel_tile_width(dev_priv, fb_modifier, cpp);
> > > }
> > > }
> > >
> > > @@ -3106,7 +3124,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > > plane_ctl |= skl_plane_ctl_rotation(rotation);
> > >
> > > obj = intel_fb_obj(fb);
> > > - stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > > + stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > > fb->pixel_format);
> > > surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
> > >
> > > @@ -9066,7 +9084,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> > > fb->width = ((val >> 0) & 0x1fff) + 1;
> > >
> > > val = I915_READ(PLANE_STRIDE(pipe, 0));
> > > - stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> > > + stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > > fb->pixel_format);
> > > fb->pitches[0] = (val & 0x3ff) * stride_mult;
> > >
> > > @@ -11137,7 +11155,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> > > * linear buffers or in number of tiles for tiled buffers.
> > > */
> > > stride = fb->pitches[0] /
> > > - intel_fb_stride_alignment(dev, fb->modifier[0],
> > > + intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > > fb->pixel_format);
> > >
> > > /*
> > > @@ -14214,6 +14232,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > > struct drm_mode_fb_cmd2 *mode_cmd,
> > > struct drm_i915_gem_object *obj)
> > > {
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > unsigned int aligned_height;
> > > int ret;
> > > u32 pitch_limit, stride_alignment;
> > > @@ -14255,7 +14274,8 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > > return -EINVAL;
> > > }
> > >
> > > - stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> > > + stride_alignment = intel_fb_stride_alignment(dev_priv,
> > > + mode_cmd->modifier[0],
> > > mode_cmd->pixel_format);
> > > if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> > > DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 1152566..98a8d31 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1017,8 +1017,8 @@ unsigned int intel_fb_align_height(struct drm_device *dev,
> > > uint64_t fb_format_modifier);
> > > void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
> > > enum fb_op_origin origin);
> > > -u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > > - uint32_t pixel_format);
> > > +u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> > > + uint64_t fb_modifier, uint32_t pixel_format);
> > >
> > > /* intel_audio.c */
> > > void intel_init_audio(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 90e27c8..3d96871 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -215,7 +215,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> > > pixel_size, true,
> > > src_w != crtc_w || src_h != crtc_h);
> > >
> > > - stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > > + stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > > fb->pixel_format);
> > >
> > > scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> > > --
> > > 2.4.9
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
