On 4 May 2017 at 18:16, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
>> +#include <drm/drmP.h>
>
> I wonder if Daniel has already split everything used here into its own
> headers?
not sure, if drm_file is out there yet. I'll find out when I rebase
this onto something newer I expect.
>> +
>> +static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
>> + u32 handle)
>
> I would like this exposed to the driver as well, so I can do handle to
> syncobj conversion once. (drm_syncobj_find() if we go with kref_get style
> naming.)
Where do you expect to need two lookups? At least for the semaphore APIs,
you have a list of wait and list of signals, the lists should be different, I've
no objections to exporting this later, but it would be easier to just do that
with the first user.
>
>> +static struct drm_syncobj *drm_syncobj_fdget(int fd)
>
> It will aslo be useful to export the fd -> syncobj function for drivers. In
> the interface Jason has for execbuf, we can substitute the handle for an
> fd + flag, which may be more convenient for the user.
Happy once we have a use case for it, I'd rather we didn't expose the syncobj
fd to userspace for anything but sending a syncobj between processes, avoiding
using fd's is one of the main points of this, I'd hate for an API to
demand we use
fd's.
>> + mutex_lock(&syncobj->file_lock);
>> + if (!syncobj->file) {
>
> Mutex only being used to ensure syncobj->file is set once? Is the race
> likely? If not, we can spare the mutex and just use a try instead:
It's pretty unlikely, so I've gone ahead and used your code.
>
> We've been steadily converting to the kref_get style of nomenclature for
> drm object reference handling (i.e. drm_syncobj_get, drm_syncobj_put)
Done locally, will resend series soon.
Dave.
