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.
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.
> + 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.
> + 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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
