[Intel-gfx] [PATCH 5/6] drm/i915: check for the supported strides on HSW+ FBC

[Paulo Zanoni]

2015-07-09 14:15 GMT-03:00 Daniel Vetter <daniel at ffwll.ch>:
> On Wed, Jul 08, 2015 at 05:58:58PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> I could only find the restrictions for HSW+, but I think it's safe to
>> assume that the older platforms also can't support the configurations
>> HSW can't support. The older platforms probably have additional
>> restrictions, so we need to figure out those and implement them later.
>> Let's not block HSW+ FBC based on missing information for the older
>> platforms.
>>
>> This may fix kms_frontbuffer_tracking failures depending on your
>> monitor configuration.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>  drivers/gpu/drm/i915/intel_fbc.c | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 52d07fb..4a4b2a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -936,6 +936,7 @@ struct i915_fbc {
>>               FBC_CHIP_DEFAULT, /* disabled by default on this chip */
>>               FBC_ROTATION, /* rotation is not supported */
>>               FBC_IN_DBG_MASTER, /* kernel debugger is active */
>> +             FBC_BAD_STRIDE, /* stride is not supported */
>>       } no_fbc_reason;
>>
>>       bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 0a24e96..7b65b00 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -501,6 +501,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
>>               return "rotation unsupported";
>>       case FBC_IN_DBG_MASTER:
>>               return "Kernel debugger is active";
>> +     case FBC_BAD_STRIDE:
>> +             return "framebuffer stride not supported";
>>       default:
>>               MISSING_CASE(reason);
>>               return "unknown reason";
>> @@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>>               goto out_disable;
>>       }
>>
>> +     /* TODO: we need to figure out what are the stride restrictions for the
>> +      * older platforms. */
>> +     if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 ||
>
> < 512 is already take care of by the X-tiling restriction (requires a
> multiple of 512 bytes stride)
. > 16kb might be the general stride limit,
> we should probably reject that in intel_framebuffer_init. 64b alignment is
> strange since X-tiled is stricter, and we that already in
> intel_fb_stride_alignment.

16384 bytes is just  4096 32bit pixels wide. If you have 2 modern
monitors configured in wide mode you already beat that (or maybe 3
not-so-fancy monitors). So if you want FBC you'll have to "xrandr
--output DP1 --above eDP1 --auto", insead of "--left-of" (also, pray
that the guys developing your desktop environment actually tested this
case). So this is certainly a check that we want in the FBC code but
not in other places. I can remove the other checks if you want.

I also did find that gen3 requires specifically 4k or 8k strides:
nothing except for that. But I'll only write this patch much later
(gen3 is not a priority for now, as you know).

>
> So summary: Sounds like we need to add per-platform stride limit checks to
> fb create code.

As mentioned above: no. FBC can be more strict.

> Plus igt testcases to make sure we check for this (since
> right now it seems like we don't).

It's on the TODO list but it's not a priority since the Kernel checks
are very straightforward. One of the problems is that different
platforms have different requirements, so I'll have to hardcode those
requirements on IGT too. And I'll have to stop using igt_create_fb for
that case since it only use powers of 2 for stride.

So yeah, one day we may have the test, but not a priority right now
since it's very easy to look at the Kernel code and make sure it's
correct.

> Additional checks here in the fbc code
> don't seem required. But if you want to I guess you could convert them to
> WARN_ON (without bailing out).
> -Daniel
>
>> +         (fb->pitches[0] & (64 - 1)) != 0) {
>> +             set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE);
>> +             goto out_disable;
>> +     }
>> +
>>       /* If the kernel debugger is active, always disable compression */
>>       if (in_dbg_master()) {
>>               set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);
>> --
>> 2.1.4
>>
>> _______________________________________________
>> 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



-- 
Paulo Zanoni