linux-kernel - Re: [RFC PATCH] drm/radeon: rework to new fence interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52127411.2010106@canonical.com>
Date:	Mon, 19 Aug 2013 21:37:53 +0200
From:	Maarten Lankhorst <maarten.lankhorst@...onical.com>
To:	Christian König <deathsimple@...afone.de>
CC:	dri-devel@...ts.freedesktop.org, linux-arch@...r.kernel.org,
	linaro-mm-sig@...ts.linaro.org,
	Alex Deucher <alexander.deucher@....com>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [RFC PATCH] drm/radeon: rework to new fence interface

Op 19-08-13 14:35, Christian König schreef:
> Am 19.08.2013 12:17, schrieb Maarten Lankhorst:
>> [SNIP]
>> @@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
>>           }
>>       } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
>>   -    if (wake) {
>> +    if (wake)
>>           rdev->fence_drv[ring].last_activity = jiffies;
>> -        wake_up_all(&rdev->fence_queue);
>> -    }
>> +    return wake;
>>   }
>
> Very bad idea, when sequence numbers change, you always want to wake up the whole fence queue here.
Yes, and the callers of this function call wake_up_all or wake_up_all_locked themselves, based on the return value..

>> [SNIP]
>> +/**
>> + * radeon_fence_enable_signaling - enable signalling on fence
>> + * @fence: fence
>> + *
>> + * This function is called with fence_queue lock held, and adds a callback
>> + * to fence_queue that checks if this fence is signaled, and if so it
>> + * signals the fence and removes itself.
>> + */
>> +static bool radeon_fence_enable_signaling(struct fence *f)
>> +{
>> +    struct radeon_fence *fence = to_radeon_fence(f);
>> +
>> +    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
>> +        !fence->rdev->ddev->irq_enabled)
>> +        return false;
>> +
>
> Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.
For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster,
and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.
>> +    radeon_irq_kms_sw_irq_get(fence->rdev, fence->ring);
>> +
>> +    if (__radeon_fence_process(fence->rdev, fence->ring))
>> +        wake_up_all_locked(&fence->rdev->fence_queue);
>> +
>> +    /* did fence get signaled after we enabled the sw irq? */
>> +    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq) {
>> +        radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring);
>> +        return false;
>> +    }
>> +
>> +    fence->fence_wake.flags = 0;
>> +    fence->fence_wake.private = NULL;
>> +    fence->fence_wake.func = radeon_fence_check_signaled;
>> +    __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake);
>> +    fence_get(f);
>> +
>> +    return true;
>> +}
>> +
>>   /**
>>    * radeon_fence_signaled - check if a fence has signaled
>>    *
>>
>
> Christian.
>
~Maarten

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/