On Thu, May 3, 2012 at 5:04 PM, Alex Deucher <alexdeucher at gmail.com> wrote: > On Thu, May 3, 2012 at 4:46 PM, Jerome Glisse <j.glisse at gmail.com> wrote: >> On Thu, May 3, 2012 at 12:34 PM, Jerome Glisse <j.glisse at gmail.com> wrote: >>> On Thu, May 3, 2012 at 12:29 PM, Alex Deucher <alexdeucher at gmail.com> >>> wrote: >>>> On Thu, May 3, 2012 at 11:56 AM, Jerome Glisse <j.glisse at gmail.com> >>>> wrote: >>>>> On Thu, May 3, 2012 at 7:39 AM, Christian K?nig <deathsimple at >>>>> vodafone.de> wrote: >>>>>> On 03.05.2012 09:21, Michel D?nzer wrote: >>>>>>> >>>>>>> On Mit, 2012-05-02 at 16:20 -0400, j.glisse at gmail.com wrote: >>>>>>>> >>>>>>>> From: Jerome Glisse<jglisse at 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 at 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
