Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759336AbcDEOT3 (ORCPT ); Tue, 5 Apr 2016 10:19:29 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:33630 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759299AbcDEOT0 (ORCPT ); Tue, 5 Apr 2016 10:19:26 -0400 MIME-Version: 1.0 In-Reply-To: References: <1458758847-21170-1-git-send-email-gustavo@padovan.org> <1458758847-21170-2-git-send-email-gustavo@padovan.org> Date: Tue, 5 Apr 2016 15:19:24 +0100 Message-ID: Subject: Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes From: Daniel Stone To: Rob Clark Cc: Gustavo Padovan , Daniel Vetter , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , "dri-devel@lists.freedesktop.org" , Linux Kernel Mailing List , Riley Andrews , Gustavo Padovan , John Harrison Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2986 Lines: 63 Hi, On 5 April 2016 at 15:04, Rob Clark wrote: > On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone wrote: >> I've been assuming that it would have to be write-only; I don't >> believe there would be any meaningful usecases for read. Do you have >> any in mind, or is it just a symmetry/cleanliness thing? > > no, don't see a use-case for it.. but this patch didn't seem to be > preventing it. And it was storing the fence_fd on the kernel side > which is a no-no as well. Yeah, both should be fixed. :) >>> The issue is that 'struct file' / 'int fd' have a fundamentally >>> different lifecycle compared to 'kms obj' / 'u32 obj_id'. For the kms >>> objects (like framebuffers) where we can use them with properties, the >>> id is tied to the kernel side object. This is not true for file >>> descriptors. Resulting in a few problems: >> >> Surely the fence FD tied to the kernel-side struct fence, in exactly >> the same way, no? > > well, what I mean is you can't keep around the int fd on the kernel > side, like this patch does > > A write-only property, which immediately (ie. during the ioctl call) > is converted into a fence object, would work. Although given that we > need to handle fences differently (ie. not a property) for out-fences, > it seems odd to shoehorn them into a property for in-fences. Depends on how you look at it, I guess. From the point of view of all fence-like things being consistent, yes, it falls down. But from the point of view of in-fences being attached to an FB, and out-fences (like events) being separately attached to a request, it's a lot more consistent. >>> I think it is really better to pass in an array of 'struct { u32 >>> plane; int fd }' (which the atomic ioctl code converts into 'struct >>> fence's and attaches to the plane state) and an array of 'struct { u32 >>> crtc; int fd }' filled in on the kernel side for the out-fences. >> >> Mmm, it definitely makes ioctl parsing harder, and still you rely on >> drivers to implement the more-difficult-to-not-screw-up part, which >> (analagous to crtc_state->event) is the driver managing the lifecycle >> of that component of the state. We already enforce this with events >> though, and the difficult part wasn't in the userspace interface, but >> the backend handling. This doesn't change at all regardless of whether >> we use a property or an external array, so ... > > hmm, I'm assuming that the in/out arrays are handled in > drm_mode_atomic_ioctl() and the drivers never see 'em.. True, but it complicates the (already not hugely straightforward) parsing that the ioctl has to do. It also makes extending the ioctl a little harder to do in future, because you're adding in two variable-size elements, and have to do some fairly complicated parsing to even figure out what the size _should_ be. So I'd rather not do it if there was any way out of it; at the very least it'd have to be userptr rather than array. Cheers, Daniel