On Wed, Aug 10, 2016 at 12:23:15PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
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@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
