[Intel-gfx] [PATCH 03/22] drm/i915: Factor out intel_tile_width()
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Oct 21 05:09:55 PDT 2015
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 at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > 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 at linux.intel.com>
> > ---
> > 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'.
>
> 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
>
> Otherwise looks good to me, Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list