Re: [RFC PATCH 0/9] V4L2 Jobs API WIP

[Laurent Pinchart]

Hi Hans,

On Wednesday, 25 October 2017 19:19:27 EEST Hans Verkuil wrote:
> On 10/25/2017 05:48 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > On Monday, 23 October 2017 11:45:01 EEST Alexandre Courbot wrote:
> >> On Thu, Oct 19, 2017 at 11:43 PM, Sakari Ailus <sakari.ailus@xxxxxx> 
wrote:
> >>> On Thu, Sep 28, 2017 at 06:50:18PM +0900, Alexandre Courbot wrote:
> >>>> Hi everyone,
> >>>> 
> >>>> Here is a new attempt at the "request" (which I propose to rename
> >>>> "jobs")
> >>>> API for V4L2, hopefully in a manner that can converge to something that
> >>>> will be merged. The core ideas should be easy to grasp for those
> >>>> familiar with the previous attemps, yet there are a few important
> >>>> differences.
> >>>> 
> >>>> Most notably, user-space does not need to explicitly allocate and
> >>>> manage
> >>>> requests/jobs (but still can if this makes sense). We noticed that only
> >>>> specific use-cases require such an explicit management, and opted for a
> >>>> jobs queue that controls the flow of work over a set of opened devices.
> >>>> This should simplify user-space code quite a bit, while still retaining
> >>>> the ability to manage states explicitly like the previous request API
> >>>> proposals allowed to do.
> >>>> 
> >>>> The jobs API defines a few new concepts that user-space can use to
> >>>> control the workflow on a set of opened V4L2 devices:
> >>>> 
> >>>> A JOB QUEUE can be created from a set of opened FDs that are part of a
> >>>> pipeline and need to cooperate (be it capture, m2m, or media controller
> >>>> devices).
> >>>> 
> >>>> A JOB can then be set up with regular (if slightly modified) V4L2
> >>>> ioctls,
> >>>> and then submitted to the job queue. Once the job queue schedules the
> >>>> job, its parameters (controls, etc) are applied to the devices of the
> >>>> queue, and itsd buffers are processed. Immediately after a job is
> >>>> submitted, the next job is ready to be set up without further user
> >>>> action.
> >>>> 
> >>>> Once a job completes, it must be dequeued and user-space can then read
> >>>> back its properties (notably controls) at completion time.
> >>>> 
> >>>> Internally, the state of jobs is managed through STATE HANDLERS. Each
> >>>> driver supporting the jobs API needs to specify an implementation of a
> >>>> state handler. Fortunately, most drivers can rely on the generic state
> >>>> handler implementation that simply records and replays a job's
> >>>> parameter
> >>>> using standard V4L2 functions. Thanks to this, adding jobs API support
> >>>> to a driver relying on the control framework and vb2 only requires a
> >>>> dozen lines of codes.
> >>>> 
> >>>> Drivers with specific needs or opportunities for optimization can
> >>>> however
> >>>> provide their own implementation of a state handler. This may in
> >>>> particular be beneficial for hardware that supports configuration or
> >>>> command buffers (thinking about VSP1 here).
> >>>> 
> >>>> This is still very early work, and focus has been on the following
> >>>> points:
> >>>> 
> >>>> * Provide something that anybody can test (currently using vim2m and
> >>>> vivid),
> >>>> * Reuse the current V4L2 APIs as much as possible,
> >>>> * Remain flexible enough to accomodate the inevitable changes that will
> >>>> be requested,
> >>>> * Keep line count low, even if functionality is missing at the moment.
> >>>> 
> >>>> Please keep this in mind while going through the patches. In
> >>>> particular,
> >>>> at the moment the parameters of a job are limited to integer controls.
> >>>> I
> >>>> know that much more is expected, but V4L2 has quite a learning curve
> >>>> and
> >>>> I preferred to focus on the general concepts for now. More is coming
> >>>> though! :)
> >>>> 
> >>>> I have written two small example programs that demonstrate the use of
> >>>> this API:
> >>>> 
> >>>> - With a codec device (vim2m):
> >>>> https://gist.github.com/Gnurou/34c35f1f8e278dad454b51578d239a42
> >>>> 
> >>>> - With a capture device (vivid):
> >>>> https://gist.github.com/Gnurou/5052e6ab41e7c55164b75d2970bc5a04
> >>>> 
> >>>> Considering the history with the request API, I don't expect everything
> >>>> proposed here to be welcome or understood immediately. In particular I
> >>>> apologize for not reusing any of the previous attempts - I was just
> >>>> more
> >>>> comfortable laying down my ideas from scratch.
> >>>> 
> >>>> If this proposal is not dismissed as complete garbage I will also be
> >>>> happy to discuss it in-person at the mini-summit in Prague. :)
> >>> 
> >>> Thank you for the initiative and the patchset.
> >>> 
> >>> While reviewing this patchset, I'm concentrating primarily on the
> >>> approach
> >>> taken and the design, not so much in the actual implementation which I
> >>> don't think matters much at this moment.
> >> 
> >> Thanks, that is exactly how I hoped things would go for the moment.
> >> 
> >>> It's difficult to avoid seeing many similarities with the Request API
> >>> patches posted earlier on. And not only that, rather you have to start
> >>> looking for the differences in what I could call details, while
> >>> important
> >>> design decisions could sometimes be only visible in what appear details
> >>> at
> >>> this point.
> >> 
> >> I was not quite sure whether I should base this work on one of the
> >> existing patchsets (and in this case, which one) or start from
> >> scratch. This being my first contribution to a new area of the kernel
> >> for me, I decided to start from scratch as it would yield more
> >> educative value.
> > 
> > What bothers me here is that we had a full day face to face meeting in
> > Tokyo back in June about this, where you presented this idea of how the
> > userspace API could look like. We went through the proposal point by
> > point to discuss potential issues, and for most points I recall agreeing
> > that the changes proposed compared to the previous request API RFC were
> > introducing problems that couldn't be easily solved. I walked out of the
> > meeting understanding we had an agreement to go back to an API quite
> > similar to the previous RFC, in particular with explicit request object
> > management from userspace, and I now see several months later an RFC that
> > ignores all the conclusions of our meeting.
> 
> Laurent, can you give a link to that RFC? Just so we all refer to the same
> request API RFC. That will help with the discussion tomorrow.

Sure, it's available at https://www.spinics.net/lists/linux-sh/msg48289.html. 
Sakari has posted two newer versions based on that original series.

> Two notes: my hope is that by the end of Friday we have a public API that is
> good enough for the codec use case and can be extended for the more generic
> use case. We have codec drivers waiting to be mainlined for years now that
> are blocked because of this. It's getting ridiculous. That also means that
> I don't care all that much about the kernel API: ideally it will be
> suitable or extensible for the generic use case, but if it isn't and needs
> to be reworked substantially for the generic use case, then so be it.
> Obviously this is something I hope we can avoid, but it would be much worse
> if the codec vendors would move away from V4L2 and start using other
> suboptimal solutions just because we can't reach an agreement.
> 
> As I said in the beginning: we don't have that option for the public API:
> that does need some careful thought as we cannot change that later, we can
> only extend it.

I agree with your overall, we need to move forward on this. Focussing on the 
userspace API first is the way to go according to me as well. I wouldn't say 
that I don't care about the kernel API, but that's indeed easier to change. 
I'd like to share your optimism about what we will achieve by the end of 
Friday, and I would certainly love to be proven wrong when I'm pessimistic :) 
Let's try our best.

> Second note (just to throw it in the discussion): I've always thought that
> controls are a good way to store state, including things like formats.
> 
> Having a single framework taking care of that would simplify things
> substantially. With VIDIOC_S_EXT_CTRLS you can already set a large number
> of controls in one ioctl.

To some extent I agree with you, but I don't think we should base the API on 
VIDIOC_S_EXT_CTRLS, nor the implementation on the control framework as it 
exists today. That's a topic to be discussed tomorrow :-)

> This does require some (or a lot?) refactoring of the control framework as
> you rightly mentioned in your email, but I'm willing to do that work.

Thank you.

-- 
Regards,

Laurent Pinchart