Am Mi, 23.02.2011, 07:59 schrieb Indan Zupancic:
> On Tue, February 22, 2011 18:25, Daniel Vetter wrote:
>> It looks like gen2 has a peculiar interleaved 2-row inter-tile
>> layout. Probably inherited from i81x which had 2kb tiles (which
>> naturally fit an even-number-of-tile-rows scheme to fit onto 4kb
>> pages). There is no other mention of this in any docs (also not
>> in the Intel internal documention according to Chris Wilson).
>>
>> Problem manifests itself in corruptions in the second half of the
>> last tile row (if the bo has an odd number of tiles). Which can
>> only happen with relaxed tiling (introduced in a00b10c360b35d6431a9).
>>
>> So reject set_tiling calls that don't satisfy this constrain to
>> prevent broken userspace from causing havoc. While at it, also
>> check the size for newer chipsets.
>>
>> LKML: https://lkml.org/lkml/2011/2/19/5
>> Reported-by: Indan Zupancic <[email protected]>
>> Signed-off-by: Daniel Vetter <[email protected]>
>> ---
>> drivers/gpu/drm/i915/i915_gem_tiling.c | 16 +++++++++++++++-
>> 1 files changed, 15 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
>> b/drivers/gpu/drm/i915/i915_gem_tiling.c
>> index 22a32b9..79a04fd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
>> @@ -184,7 +184,7 @@ i915_gem_detect_bit_6_swizzle(struct drm_device
>> *dev)
>> static bool
>> i915_tiling_ok(struct drm_device *dev, int stride, int size, int
>> tiling_mode)
>> {
>> - int tile_width;
>> + int tile_width, tile_height;
>>
>> /* Linear is always fine */
>> if (tiling_mode == I915_TILING_NONE)
>> @@ -215,6 +215,20 @@ i915_tiling_ok(struct drm_device *dev, int stride,
>> int size, int
>> tiling_mode)
>> }
>> }
>>
>> + if (IS_GEN2(dev) ||
>> + (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
>> + tile_height = 32;
>> + else
>> + tile_height = 8;
>> + /* i8xx is strange: It has 2 interleaved rows of tiles, so needs an
>> even
>> + * number of tile rows. */
>> + if (IS_GEN2(dev))
>> + tile_height *= 2;
>> +
>> + /* Size needs to be aligned to a full tile row */
>> + if (size & (tile_height * stride - 1))
>> + return false;
>> +
>> /* 965+ just needs multiples of tile width */
>> if (INTEL_INFO(dev)->gen >= 4) {
>> if (stride & (tile_width - 1))
>
> Tested-by: Indan Zupancic <[email protected]>
>
> I tested with this patch and without the other ones you send and the
> corruption
> is indeed gone.
>
> Not sure why you dropped lkml from CC, now people who stuble upon it don't
> see
> the ending...
Random incoherency in my brain. Re-added to cc.
- Daniel
Hi,
On Wed, February 23, 2011 08:10, Daniel Vetter wrote:
> Am Mi, 23.02.2011, 07:59 schrieb Indan Zupancic:
>> On Tue, February 22, 2011 18:25, Daniel Vetter wrote:
>>> It looks like gen2 has a peculiar interleaved 2-row inter-tile
>>> layout. Probably inherited from i81x which had 2kb tiles (which
>>> naturally fit an even-number-of-tile-rows scheme to fit onto 4kb
>>> pages). There is no other mention of this in any docs (also not
>>> in the Intel internal documention according to Chris Wilson).
>>>
>>> Problem manifests itself in corruptions in the second half of the
>>> last tile row (if the bo has an odd number of tiles). Which can
>>> only happen with relaxed tiling (introduced in a00b10c360b35d6431a9).
>>>
>>> So reject set_tiling calls that don't satisfy this constrain to
>>> prevent broken userspace from causing havoc. While at it, also
>>> check the size for newer chipsets.
>>>
>>> LKML: https://lkml.org/lkml/2011/2/19/5
>>> Reported-by: Indan Zupancic <[email protected]>
>>> Signed-off-by: Daniel Vetter <[email protected]>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem_tiling.c | 16 +++++++++++++++-
>>> 1 files changed, 15 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
>>> b/drivers/gpu/drm/i915/i915_gem_tiling.c
>>> index 22a32b9..79a04fd 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
>>> @@ -184,7 +184,7 @@ i915_gem_detect_bit_6_swizzle(struct drm_device
>>> *dev)
>>> static bool
>>> i915_tiling_ok(struct drm_device *dev, int stride, int size, int
>>> tiling_mode)
>>> {
>>> - int tile_width;
>>> + int tile_width, tile_height;
>>>
>>> /* Linear is always fine */
>>> if (tiling_mode == I915_TILING_NONE)
>>> @@ -215,6 +215,20 @@ i915_tiling_ok(struct drm_device *dev, int stride,
>>> int size, int
>>> tiling_mode)
>>> }
>>> }
>>>
>>> + if (IS_GEN2(dev) ||
>>> + (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
>>> + tile_height = 32;
>>> + else
>>> + tile_height = 8;
>>> + /* i8xx is strange: It has 2 interleaved rows of tiles, so needs an
>>> even
>>> + * number of tile rows. */
>>> + if (IS_GEN2(dev))
>>> + tile_height *= 2;
>>> +
>>> + /* Size needs to be aligned to a full tile row */
>>> + if (size & (tile_height * stride - 1))
>>> + return false;
>>> +
>>> /* 965+ just needs multiples of tile width */
>>> if (INTEL_INFO(dev)->gen >= 4) {
>>> if (stride & (tile_width - 1))
>>
>> Tested-by: Indan Zupancic <[email protected]>
>>
>> I tested with this patch and without the other ones you send and the
>> corruption is indeed gone.
>>
>> Not sure why you dropped lkml from CC, now people who stuble upon it
>> don't see the ending...
>
> Random incoherency in my brain. Re-added to cc.
This isn't in rc8, can someone make sure it gets into 2.6.38-rc9/2.6.38?
Thanks,
Indan
Am Do, 10.03.2011, 06:06 schrieb Indan Zupancic:
> This isn't in rc8, can someone make sure it gets into 2.6.38-rc9/2.6.38?
The patch unfortunately broke gen4+ (more precisely: the unwritten abi
guarantee that userspace can tile buffers that don't have a complete last
tile row, as long as it promises not to touch it). Hence it got reverted.
It was just a enforcement check to prevent broken userspace from fooling
itself. The proper fix is to upgrade your userspace (libdrm +
xf86-video-intel).
[Aside: This only happens if you have new enough userspace that supports
relaxed tiling. Old userspace and new kernels are _not_ broken.]
Cheers, Daniel
On Thu, March 10, 2011 08:52, Daniel Vetter wrote:
> Am Do, 10.03.2011, 06:06 schrieb Indan Zupancic:
>> This isn't in rc8, can someone make sure it gets into 2.6.38-rc9/2.6.38?
>
> The patch unfortunately broke gen4+ (more precisely: the unwritten abi
> guarantee that userspace can tile buffers that don't have a complete last
> tile row, as long as it promises not to touch it). Hence it got reverted.
> It was just a enforcement check to prevent broken userspace from fooling
> itself. The proper fix is to upgrade your userspace (libdrm +
> xf86-video-intel).
>
> [Aside: This only happens if you have new enough userspace that supports
> relaxed tiling. Old userspace and new kernels are _not_ broken.]
Yes, I noticed it got reverted when digging into git log, but the
commit message only said it broke gen4+, not how gen2 should be
unbroken after the revert.
Which versions fix this, just for reference?
I got libdrm 2.4.23 and xf86-video-intel 2.14.0.
(As a side note, do you have version checking on the kernel side?
If so, you could return 0 for the relaxed fencing feature check
if the driver is the wrong version.)
To keep things manageable I either upgrade the kernel, or userspace,
but never both at once. It seems that that doesn't work with graphics.
Thanks,
Indan
Am Do, 10.03.2011, 11:36 schrieb Indan Zupancic:
> On Thu, March 10, 2011 08:52, Daniel Vetter wrote:
>> [Aside: This only happens if you have new enough userspace that supports
>> relaxed tiling. Old userspace and new kernels are _not_ broken.]
>
> Yes, I noticed it got reverted when digging into git log, but the
> commit message only said it broke gen4+, not how gen2 should be
> unbroken after the revert.
>
> Which versions fix this, just for reference?
git master branch of libdrm and xf86-video-intel newer than 2011-02-22.
-Daniel
On Thu, March 10, 2011 14:31, Daniel Vetter wrote:
> Am Do, 10.03.2011, 11:36 schrieb Indan Zupancic:
>> Which versions fix this, just for reference?
>
> git master branch of libdrm and xf86-video-intel newer than 2011-02-22.
Thank you. If there will be no new releases of those packages within
a couple weeks it might be better to temporarily add those checks back
for gen 2 only, I think. Otherwise there will be a period where people
who update regularly will have screen corruption, with no easy way of
fixing it. I think this would avoid a few unnecessary bugreports. The
check can be removed in 2.6.39-rc1, if you want I'll remind you about
it too.
Greetings,
Indan
On Fri, Mar 11, 2011 at 02:08:46AM +0100, Indan Zupancic wrote:
> On Thu, March 10, 2011 14:31, Daniel Vetter wrote:
> > Am Do, 10.03.2011, 11:36 schrieb Indan Zupancic:
> >> Which versions fix this, just for reference?
> >
> > git master branch of libdrm and xf86-video-intel newer than 2011-02-22.
>
> Thank you. If there will be no new releases of those packages within
> a couple weeks it might be better to temporarily add those checks back
> for gen 2 only, I think. Otherwise there will be a period where people
> who update regularly will have screen corruption, with no easy way of
> fixing it. I think this would avoid a few unnecessary bugreports. The
> check can be removed in 2.6.39-rc1, if you want I'll remind you about
> it too.
Well, libdrm is already released (2.4.24) and for the ddx there's an rc
(2.4.901) out there. So that should be sufficient.
-Daniel
--
Daniel Vetter
Mail: [email protected]
Mobile: +41 (0)79 365 57 48