Received: by 10.223.176.5 with SMTP id f5csp1027019wra; Tue, 30 Jan 2018 23:51:19 -0800 (PST) X-Google-Smtp-Source: AH8x226aNEXDwNRFmLy1X5LitrRdWX+8NYIzTqmTA4BJvv01k/ema4LeY4nkC6mN8/U9U+3CbxV8 X-Received: by 10.101.75.81 with SMTP id k17mr26508683pgt.335.1517385079266; Tue, 30 Jan 2018 23:51:19 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517385079; cv=none; d=google.com; s=arc-20160816; b=Oz66lnT4dgyJ/vo5Yf7sBOz8whT/JeJkRxcnWt3x5Dv/Yom6WfLPmVAZbYt/3eD19f RSXU0j9+cW59T2XzDMhzU2Nwp2UzN9NNpxwkYvRDIfMaN/HfW1MpTncEUVdKICUSkpMY 21eSMjZTT8ojRlBYPFRJBbgQ/T7B2r9ZX0NFp4qXNSEbHAkYtDVyICbL6RoRZvRjIspv 5Qw9KmjJYit0yu6QxOqO0DqwjTiCnSKjIB3dM8RdhMPzlDhg58BhakevVmgFsgu7GjuH E1EPFgpF6SydeBH6jnhFpCZcs+eSOQz+FN7tKaeng4irPfcos7QLuYrEWLSpIQ5K1BGB KG0g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=vaZ9uiRE9o0aeeiB8AFdIgldZ8/KCX0PZN1gzkodl2I=; b=WWUb0pkhkbY4xjWgouNEMGHUy+I20bLoFvZOlAwg7gL8TpLiAtA2f0Fxp2RFPLeKM9 5zNYwger72SWfepOkxIjWZvt42pYBnYbSV00lnbBC20MJ4Ojx+Z0QH7jfAllzZ/bzsMe 8H3JZpZleZ41Ztx9xjJKFtkvwVrrQ3pgqMO9QI5+d8qurj+QVbQ/Fw7lgHsVIfeWXHyn eZ1g+TBDo2QlEQ6C+W3F1aJ/e1pAdMcPPMlRXYu1G2VVYDSp8ARBl+sYdqjLE0C/dOja j3iHl6SdfvNL7ULe1e49J7dasD7coHfOhj2cnQtoaiFg0JBu4jCvNGb9rj6fTX73wd/F f58A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id x7si1916679pgr.525.2018.01.30.23.51.04; Tue, 30 Jan 2018 23:51:19 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752934AbeAaHuh (ORCPT + 99 others); Wed, 31 Jan 2018 02:50:37 -0500 Received: from lb2-smtp-cloud8.xs4all.net ([194.109.24.25]:41255 "EHLO lb2-smtp-cloud8.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751001AbeAaHuf (ORCPT ); Wed, 31 Jan 2018 02:50:35 -0500 Received: from [192.168.2.10] ([212.251.195.8]) by smtp-cloud8.xs4all.net with ESMTPA id gnA5eIoSLar0wgnA8eGzfz; Wed, 31 Jan 2018 08:50:34 +0100 Subject: Re: [RFC PATCH 0/8] [media] Request API, take three To: Alexandre Courbot Cc: Mauro Carvalho Chehab , Laurent Pinchart , Pawel Osciak , Marek Szyprowski , Tomasz Figa , Sakari Ailus , Gustavo Padovan , Linux Media Mailing List , linux-kernel@vger.kernel.org References: <20180126060216.147918-1-acourbot@chromium.org> From: Hans Verkuil Message-ID: <307f58dd-ce7e-b6f5-092a-f8679349be73@xs4all.nl> Date: Wed, 31 Jan 2018 08:50:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfA7596SLEsvUyt9cNEg9gf2H1FefASE2BmwkTMy1l2wrWrOiXTL7LA7FESrpT8VBd4mlznLGuGixFac7anePKU3R6WEon/laYYcs/lzrUkrWfiFYSuNX 9yirihtpwFgXPzxwB0nMHAF5+QyFu5Evta/BCcrT6oiYtZzuQ3w1AgDKexkH3Cb5gGOBq2WWS7BvfjJFAfe83q6kdZqIQL+lK2F6yjJRT3ZhgOIZtdVuHAbP QtCe6jLPifPdavnHSQ7CocRRZyRSGjbNJzghnfKxNjBN54flVxIA+wap1oKIEzKJEttumDhqEMM8rZN6TyoFICkn2x5VbK4vivS2zMXyJmEAr9smDUQzWhVG jy3nO6aJlffsjjAJXRyfllw3FrinzdEs12l3X2vEbAJVqIQS+goWkMLmrq3PSt73EuPvUz+L8+zq+57jHzeUsxvPuJpW3lEDlfVfSnDkQEYc0/5ARiXw82yE CGIxLqCWEqlld+UC Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/30/2018 07:31 AM, Alexandre Courbot wrote: > Hi Hans, > > On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil wrote: >> On 01/26/2018 07:02 AM, Alexandre Courbot wrote: >>> Howdy. Here is your bi-weekly request API redesign! ;) >>> >>> Again, this is a simple version that only implements the flow of requests, >>> without applying controls. The intent is to get an agreement on a base to work >>> on, since the previous versions went straight back to the redesign board. >>> >>> Highlights of this version: >>> >>> * As requested by Hans, request-bound buffers are now passed earlier to drivers, >>> as early as the request itself is submitted. Doing it earlier is not be useful >>> since the driver would not know the state of the request, and thus cannot do >>> anything with the buffer. Drivers are now responsible for applying request >>> parameters themselves. >>> >>> * As a consequence, there is no such thing as a "request queue" anymore. The >>> flow of buffers decides the order in which requests are processed. Individual >>> devices of the same pipeline can implement synchronization if needed, but this >>> is beyond this first version. >>> >>> * VB2 code is still a bit shady. Some of it will interfere with the fences >>> series, so I am waiting for the latter to land to do it properly. >>> >>> * Requests that are not yet submitted effectively act as fences on the buffers >>> they own, resulting in the buffer queue being blocked until the request is >>> submitted. An alternate design would be to only block the >>> not-submitted-request's buffer and let further buffers pass before it, but since >>> buffer order is becoming increasingly important I have decided to just block the >>> queue. This is open to discussion though. >> >> I don't think we should mess with the order. > > Agreed, let's keep it that way then. > >> >>> >>> * Documentation! Also described usecases for codec and simple (i.e. not part of >>> a complex pipeline) capture device. >> >> I'll concentrate on reviewing that. >> >>> >>> Still remaining to do: >>> >>> * As pointed out by Hans on the previous revision, do not assume that drivers >>> using v4l2_fh have a per-handle state. I have not yet found a good way to >>> differenciate both usages. >> >> I suspect we need to add a flag or something for this. > > I hope we don't need to, let's see if I can find a pattern... > >> >>> * Integrate Hans' patchset for control handling: as said above, this is futile >>> unless we can agree on the basic design, which I hope we can do this time. >>> Chrome OS needs this really badly now and will have to move forward no matter >>> what, so I hope this will be considered good enough for a common base of work. >> >> I am not sure there is any reason to not move forward with the control handling. >> You need this anyway IMHO, regardless of any public API considerations. > > Only reasons are my lazyness and because I wanted to focus on the > request flow first. But you're right. I have a version with your > control framework changes integrated (they worked on the first attempt > btw! :)), let me create a clean patchset from this and send another > RFC. Scary that it worked on the first attempt :-) > >> >>> A few thoughts/questions that emerged when writing this patchset: >>> >>> * Since requests are exposed as file descriptors, we could easily move the >>> MEDIA_REQ_CMD_SUBMIT and MEDIA_REQ_CMD_REININT commands as ioctls on the >>> requests themselves, instead of having to perform them on the media device that >>> provided the request in the first place. That would add a bit more flexibility >>> if/when passing requests around and means the media device only needs to handle >>> MEDIA_REQ_CMD_ALLOC. Conceptually speaking, this seems to make sense to me. >>> Any comment for/against that? >> >> Makes sense IMHO. > > Glad to hear it, that was my preferred design. :) > >> >>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with >>> the request FD that produced them (vim2m acts that way). This seems to work >>> well, however FDs are process-local and could be closed before the CAPTURE >>> buffer is dequeued, rendering that information less meaningful, if not >>> dangerous. >> >> I don't follow this. Once the fd is passed to the kernel its refcount should be >> increased so the data it represents won't be released if userspace closes the fd. > > The refcount of the request is increased. The refcount of the FD is > not, since it is only a userspace reference to the request. I don't think that's right. Looking at how dma-buf does this (dma_buf_get in dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as I can see the struct dma_buf doesn't have a refcount, it is solely refcounted through the fd. That's probably the model you want to follow. > >> >> Obviously if userspace closes the fd it cannot do anything with it anymore, but >> it shouldn't be 'dangerous' AFAICT. > > It userspace later gets that closed FD back from a DQBUF call, and > decides to use it again, then we would have a problem. I agree that it > is userspace responsibility to be careful here, but making things > foolproof never hurts. I think all the issues will go away if you refcount the fd instead of the request. It worked well for dma-buf for years. > >> >>> Wouldn't it be better/safer to use a global request ID for >>> such information instead? That ID would be returned upon MEDIA_REQ_CMD_ALLOC so >>> user-space knows which request ID a FD refers to. >> >> I think it is not a good idea to have both an fd and an ID to refer to the >> same object. That's going to be very confusing I think. > > IDs would not refer to the object, they would just be a way to > identify it. FDs would be the actual reference. > > If we drop the idea of returning request FDs to userspace after the > initial allocation (which is the only time we can be sure that a > returned FD is valid), then this is not a problem anymore, but I think > it may be useful to mark CAPTURE buffers with the request that > generated them. > >> >>> * Using the media node to provide requests makes absolute sense for complex >>> camera devices, but is tedious for codec devices which work on one node and >>> require to protect request/media related code with #ifdef >>> CONFIG_MEDIA_CONTROLLER. >> >> Why? They would now depend on MEDIA_CONTROLLER (i.e. they won't appear in the >> menuconfig unless MEDIA_CONTROLLER is set). No need for an #ifdef. > > Ah, if we make them depend on MEDIA_CONTROLLER, then indeed. But do we > want to do this for e.g. vim2m and vivid? If they support requests, then yes. > >> >> For these devices, the sole role of the media node is >>> to produce the request, and that's it (since request submission could be moved >>> to the request FD as explained above). That's a modest use that hardly justifies >>> bringing in the whole media framework IMHO. With a bit of extra abstraction, it >>> should be possible to decouple the base requests from the media controller >>> altogether, and propose two kinds of requests implementations: one simpler >>> implementation that works directly with a single V4L2 node (useful for codecs), >>> and another one that works on a media node and can control all its entities >>> (good for camera). This would allow codecs to use the request API without >>> forcing the media controller, and would considerably simplify the use-case. Any >>> objection to that? IIRC the earlier design documents mentioned this possibility. >> >> I think this is an interesting idea, but I would postpone making a decision on >> this until later. We need this MC support regardless, so let's start off with that. >> >> Once that's working we can discuss if we should or should not create a shortcut >> for codecs. Trying to decide this now would only confuse the process. > > Sounds good, as long as we make a decision before merging. Of course. Regards, Hans