Re: Re: [PATCH 2/4] drm/radeon: convert fence to uint64_t

[Jerome Glisse <j.glisse@gmail.com>]

On Thu, May 3, 2012 at 5:04 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Thu, May 3, 2012 at 4:46 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> On Thu, May 3, 2012 at 12:34 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>> On Thu, May 3, 2012 at 12:29 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>>> On Thu, May 3, 2012 at 11:56 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>>>> On Thu, May 3, 2012 at 7:39 AM, Christian König <deathsimple@vodafone.de> wrote:
>>>>>> On 03.05.2012 09:21, Michel Dänzer wrote:
>>>>>>>
>>>>>>> On Mit, 2012-05-02 at 16:20 -0400, j.glisse@gmail.com wrote:
>>>>>>>>
>>>>>>>> From: Jerome Glisse<jglisse@redhat.com>
>>>>>>>>
>>>>>>>> This convert fence to use uint64_t sequence number intention is
>>>>>>>> to use the fact that uin64_t is big enough that we don't need to
>>>>>>>> care about wrap around.
>>>>>>>>
>>>>>>>> Tested with and without writeback using 0xFFFFF000 as initial
>>>>>>>> fence sequence and thus allowing to test the wrap around from
>>>>>>>> 32bits to 64bits.
>>>>>>>>
>>>>>>>> Signed-off-by: Jerome Glisse<jglisse@redhat.com>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c
>>>>>>>> b/drivers/gpu/drm/radeon/radeon_fence.c
>>>>>>>> index 7733429..6da1535 100644
>>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_fence.c
>>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
>>>>>>>> @@ -386,9 +388,9 @@ int radeon_fence_driver_start_ring(struct
>>>>>>>> radeon_device *rdev, int ring)
>>>>>>>>                        rdev->fence_drv[ring].scratch_reg -
>>>>>>>>                        rdev->scratch.reg_base;
>>>>>>>>        }
>>>>>>>> -       rdev->fence_drv[ring].cpu_addr =rdev->wb.wb[index/4];
>>>>>>>> +       rdev->fence_drv[ring].cpu_addr =u64*)&rdev->wb.wb[index/4];
>>>>>>>
>>>>>>> Might want to ensure cpu_addr is 64 bit aligned, or there might be
>>>>>>> trouble on some architectures.
>>>>>>>
>>>>>>>
>>>>>>> With this change, Cayman cards will already use six scratch registers
>>>>>>> for the rings. It won't be possible to extend this scheme for even one
>>>>>>> additional ring, will it?
>>>>>>
>>>>>>
>>>>>> That won't work anyway, since not all rings can deal with 64 bit fences, so
>>>>>> we need to still use 32 bit signaling and extend them to 64 bit while
>>>>>> processing the fence value.
>>>>>>
>>>>>> Already working on that.
>>>>>>
>>>>>> Christian.
>>>>>
>>>>> This patch is fine with ring that can't emit directly 64bits, all you
>>>>> have to do is fix the emit_fence callback to properly handle it and
>>>>> then you have to fix the radeon_fence_read which can be move to a ring
>>>>> specific callback. Anyway point is that patchset works and is fine on
>>>>> current set of ring we have and it can work as easily for ring without
>>>>> easy 64bits value emitting. So please explain further why those patch
>>>>> can't work because as i just explained i don't see why.
>>>>>
>>>>> I have updated some v2 version of those patchset to handle the cayman
>>>>> and newer possibly running out of scratch reg and i also fix the
>>>>> alignment issue to be 64bits
>>>>
>>>> FWIW, we don't actually use scratch regs any more on r6xx+ (non-AGP at
>>>> least), it's just memory writes so we could make the scratch pool
>>>> bigger.
>>>>
>>>> Alex
>>>>
>>>
>>> That's what my v2 does, just drop scratch reg for cayman and newer.
>>>
>>> Cheers,
>>> Jerome
>>
>> Btw uploaded a v3 with some more thing like warnononce for extra
>> safety, better comment and trying to mitigate race btw cpu reading and
>> gpu writing fence.
>> http://people.freedesktop.org/~glisse/reset3/
>
> In patch:
> http://people.freedesktop.org/~glisse/reset3/0003-drm-radeon-rework-fence-handling-drop-fence-list-v3.patch
>
> You can drop this hunk:
> +       if (rdev->family >= CHIP_CAYMAN) {
> +               /* because there is so many ring on newer GPU we can't use
> +                * scratch reg thus we need to use event, on those GPU there
> +                * is no AGP version and writting to system ram have always
> +                * been reliable so far
> +                */
> +               rdev->wb.enabled = true;
> +               rdev->wb.use_event = true;
> +       }
>
> As the code right below it does the exact same thing.  It could
> probably be extended to APUs as well since they will never have AGP
> support.  I'll send out a patch.
>
> Alex
>

Good catch updated patch
http://people.freedesktop.org/~glisse/reset3/

Cheers,
Jerome