Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933367AbcDEOEz (ORCPT ); Tue, 5 Apr 2016 10:04:55 -0400 Received: from mail-yw0-f174.google.com ([209.85.161.174]:33168 "EHLO mail-yw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751976AbcDEOEy (ORCPT ); Tue, 5 Apr 2016 10:04:54 -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 10:04:53 -0400 Message-ID: Subject: Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes From: Rob Clark To: Daniel Stone 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: 4060 Lines: 85 On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone wrote: > Hi, > > On 5 April 2016 at 13:36, Rob Clark wrote: >> ok, so I've been slacking on writing up the reasons that I don't like >> the idea of using a property for fd's (including fence fd's).. I did >> at one point assume we'd use properties for fence fd's, but that idea >> falls apart pretty quickly when you think about the 'struct file' vs >> fd lifecycle. It could *possibly* work if it was a write-only >> property, but I don't think that is a good solution. > > 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. >> 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. >> 1) if it was a readable property, reading it would (should) take a >> reference.. we can't just return fence_fd that was previously set >> (since in the mean time userspace might have close()d the fd). So we >> have to return a fresh fd. And if userspace (like modetest) doesn't >> realize it is responsible to close() that fd we have a leak > > Again, assuming that read would always return -1. > >> 2) basically we shouldn't be tracking fd's on the kernel side, since >> we can only count on them being valid during the duration of the ioctl >> call. Once the call returns, you must assume userspace has close()d >> the fd. So in the ioctl we need to convert fd -> file, and from then >> on out track the file object (or in this case the underlying fence >> object). > > Right, it would have to be the same as KMS objects, where userspace > passes in an ID (in this case an entry in a non-IDR table, but still), > and the kernel maps that to struct fence and handles the lifetime. So, > almost exactly the same as what we do with KMS objects ... > >> I guess we *could* have something analogous to dmabuf, where we import >> the fence fd and get a kms drm_fence object (with an id tied to the >> kernel side object), and then use a property to attach the drm_fence >> object.. but that seems kind of pointless and just trying to force the >> 'everything is a property' mantra. > > I think that would be pointless indirection as well. > >> 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.. BR, -R > Cheers, > Daniel