Re: [PATCH 15/16] drm/i915: Repack fence tiling mode and stride into a single integer

Joonas Lahtinen
2016-08-04T11:17:25Z

On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2215,13 +2215,10 @@ struct drm_i915_gem_object {
>
>   atomic_t frontbuffer_bits;
>
> - /**
> -  * Current tiling mode for the object.
> -  */
> - unsigned int tiling_mode;
> -
>   /** Current tiling stride for the object, if it's tiled. */
> - uint32_t stride;
> + unsigned int tiling_and_stride;
> +#define TILING_MASK 0x3

No magics and add appropriate BUILD_BUG_ON too when stuffing bits
inside same variable.

#define TILING_MASK (BIT(31 - __builtin_clz(I915_TILING_LAST)) -1)

or something :P

> +#define STRIDE_MASK (~TILING_MASK)
>
>   unsigned int has_wc_mmap;
>   /** Count of VMA actually bound by this object */
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2465,9 +2465,8 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
>   return false;
>   }
>
> - obj->tiling_mode = plane_config->tiling;
> - if (obj->tiling_mode == I915_TILING_X)
> - obj->stride = fb->pitches[0];
> + if (plane_config->tiling == I915_TILING_X)
> + obj->tiling_and_stride = fb->pitches[0] | I915_TILING_X;

This is not equivalent code.

>   } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> @@ -14919,15 +14918,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
>   if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>   /* Enforce that fb modifier and tiling mode match, but only for
>    * X-tiled. This is needed for FBC. */
> - if (!!(obj->tiling_mode == I915_TILING_X) !=
> + if (!!(i915_gem_object_get_tiling(obj) == I915_TILING_X) !=

A note, !! are redundant as == returns bool already. Could remove while
touching.

Regards, Joonas

>       !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
>   DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
>   return -EINVAL;
>   }
>   } else {

--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation