linux-kernel - Re: [PATCH v2 0/3] Add Amlogic video decoder driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8b8af340-8bd7-092a-4203-fd01fd0cc5c6@xs4all.nl>
Date:   Fri, 21 Sep 2018 12:51:51 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Maxime Jourdan <mjourdan@...libre.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH v2 0/3] Add Amlogic video decoder driver

On 09/17/18 18:36, Maxime Jourdan wrote:
> 2018-09-17 16:51 GMT+02:00 Hans Verkuil <hverkuil@...all.nl>:
>> On 09/11/2018 05:09 PM, Maxime Jourdan wrote:
>>>  - Moved the single instance check (returning -EBUSY) to start/stop streaming
>>>  The check was previously in queue_setup but there was no great location to
>>>  clear it except for .close().
>>
>> Actually, you can clear it by called VIDIOC_REQBUFS with count set to 0. That
>> freed all buffers and clears this.
>>
>> Now, the difference between queue_setup and start/stop streaming is that if you
>> do this in queue_setup you'll know early on that the device is busy. It is
>> reasonable to assume that you only allocate buffers when you also want to start
>> streaming, so that it a good place to know this quickly.
>>
>> Whereas with start_streaming you won't know until you call STREAMON, or even later
>> if you start streaming with no buffers queued, since start_streaming won't
>> be called until you have at least 'min_buffers_needed' buffers queued (1 for this
>> driver). So in that case EBUSY won't be returned until the first VIDIOC_QBUF.
>>
>> My preference is to check this in queue_setup, but it is up to you to decide.
>> Just be aware of the difference between the two options.
>>
>> Regards,
>>
>>         Hans
> 
> I could for instance keep track of which queue(s) have been called
> with queue_setup, catch calls to VIDIOC_REQBUFS with count set to 0,
> and clear the current session once both queues have been reset ?

I see your point, this is rather awkward. The real problem here is that
we don't have a 'queue_free' callback. If we'd had that this would be
a lot easier.

In any case, I am dropping my objections to doing this in start/stop_streaming.

> You leverage another issue with min_buffers_needed. It's indeed set to
> 1 but this value is wrong for the CAPTURE queue. The problem is that
> this value changes depending on the codec and the amount of CAPTURE
> buffers requested by userspace.
> Ultimately I want it set to the total amount of CAPTURE buffers,
> because the hardware needs the full buffer list before starting a
> decode job.
> Am I free to change this queue parameter later, or is m2m_queue_init
> the only place to do it ?

It has to be set before the VIDIOC_STREAMON. After that you cannot
change it anymore.

But I don't think this is all that relevant, since this is something
that the job_ready() callback should take care of. min_buffers_needed
is really for hardware where the DMA engine cannot start unless that
many buffers are queued. But in that case the DMA runs continuously
capturing video, whereas here these are jobs and the DMA is only
started when you can actually execute a job.

Regards,

	Hans