Re: [PATCH v3 06/12] drm/i915: Adjust obj tiling vs. fb modifier rules

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 10, 2016 at 12:23:15PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we require the object to be X tiled if the fb is X
> tiled. The argument is supposedly FBC GTT tracking. But
> actually that no longer holds water since FBC supports
> Y tiling as well on SKL+.
> 
> A better rule IMO is to require that if there is a fence, the
> fb modifier match the object tiling mode. But if the object is linear,
> we can allow the fb modifier to be anything. The idea being that
> if the user set the tiling mode on the object, presumably the intention
> is to actually use the fence for CPU access. But if the tiling mode is
> not set, the user has no intention of using a fence (and can't actually
> since we disallow tiling mode changes when there are framebuffers
> associated with the object).
> 
> On gen2/3 we must keep to the rule that the object and fb
> must be either both linear or both X tiled. No mixing allowed
> since the display engine itself will use the fence if it's present.
> 
> v2: Fix typos
> v3: Rebase due to i915_gem_object_get_tiling() & co.
> 
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

So the reason why I didn't like this is that it makes the fbc checking
more tricky. Someone might think that checking for X-tiling on just the
drm_framebuffer is good enough and then we'd have some potentially funny
bugs. But the fbc code still only looks at the gem bo tiling, so all is
still fine.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d8855ba969be..ad9f8b303fbc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15150,23 +15150,26 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct drm_i915_gem_object *obj)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	unsigned int tiling = i915_gem_object_get_tiling(obj);
>  	int ret;
>  	u32 pitch_limit, stride_alignment;
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
>  	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 (!!(i915_gem_object_get_tiling(obj) == I915_TILING_X) !=
> -		    !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
> +		/*
> +		 * If there's a fence, enforce that
> +		 * the fb modifier and tiling mode match.
> +		 */
> +		if (tiling != I915_TILING_NONE &&
> +		    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
>  			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
>  			return -EINVAL;
>  		}
>  	} else {
> -		if (i915_gem_object_get_tiling(obj) == I915_TILING_X)
> +		if (tiling == I915_TILING_X) {
>  			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> -		else if (i915_gem_object_get_tiling(obj) == I915_TILING_Y) {
> +		} else if (tiling == I915_TILING_Y) {
>  			DRM_DEBUG("No Y tiling for legacy addfb\n");
>  			return -EINVAL;
>  		}
> @@ -15190,6 +15193,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * gen2/3 display engine uses the fence if present,
> +	 * so the tiling mode must match the fb modifier exactly.
> +	 */
> +	if (INTEL_INFO(dev_priv)->gen < 4 &&
> +	    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
> +		DRM_DEBUG("tiling_mode must match fb modifier exactly on gen2/3\n");
> +		return -EINVAL;
> +	}
> +
>  	stride_alignment = intel_fb_stride_alignment(dev_priv,
>  						     mode_cmd->modifier[0],
>  						     mode_cmd->pixel_format);
> @@ -15209,7 +15222,11 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> -	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED &&
> +	/*
> +	 * If there's a fence, enforce that
> +	 * the fb pitch and fence stride match.
> +	 */
> +	if (tiling != I915_TILING_NONE &&
>  	    mode_cmd->pitches[0] != i915_gem_object_get_stride(obj)) {
>  		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
>  			  mode_cmd->pitches[0],
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx