linux-kernel - Re: [PATCH v6 2/2] media: platform: Add Aspeed Video Engine 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: <d876ac01-238f-7e74-2738-f1c878921c77@xs4all.nl>
Date:   Mon, 3 Dec 2018 16:43:48 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Eddie James <eajames@...ux.ibm.com>, linux-kernel@...r.kernel.org
Cc:     mark.rutland@....com, devicetree@...r.kernel.org,
        linux-aspeed@...ts.ozlabs.org, robh+dt@...nel.org,
        mchehab@...nel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v6 2/2] media: platform: Add Aspeed Video Engine driver

On 12/03/2018 12:04 PM, Hans Verkuil wrote:
> On 11/27/2018 08:37 PM, Eddie James wrote:
>> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
>> can capture and compress video data from digital or analog sources. With
>> the Aspeed chip acting a service processor, the Video Engine can capture
>> the host processor graphics output.
>>
>> Add a V4L2 driver to capture video data and compress it to JPEG images.
>> Make the video frames available through the V4L2 streaming interface.
>>
>> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
>> ---

<snip>

>> +static void aspeed_video_bufs_done(struct aspeed_video *video,
>> +				   enum vb2_buffer_state state)
>> +{
>> +	unsigned long flags;
>> +	struct aspeed_video_buffer *buf;
>> +
>> +	spin_lock_irqsave(&video->lock, flags);
>> +	list_for_each_entry(buf, &video->buffers, link) {
>> +		if (list_is_last(&buf->link, &video->buffers))
>> +			buf->vb.flags |= V4L2_BUF_FLAG_LAST;
> 
> This really makes no sense. This flag is for codecs, not for receivers.
> 
> You say in an earlier reply about this:
> 
> "I mentioned before that dequeue calls hang in an error condition unless
> this flag is specified. For example if resolution change is detected and
> application is in the middle of trying to dequeue..."
> 
> What error condition are you referring to? Isn't your application using
> the select() or poll() calls to wait for events or new buffers to dequeue?
> If you just call VIDIOC_DQBUF to wait in blocking mode for a new buffer,
> then it will indeed block in that call.
> 
> No other video receiver needs this flag, so there is something else that is
> the cause.

Let me give a bit more information on how video receivers behave when the
signal disappears:

They will all send the SOURCE_CHANGE event, but what they do with respect
to streaming buffers is hardware dependent:

1) Some receivers have a freewheeling mode where the hardware generates
   an image when there is no signal (usually this is just a fixed color).
   In that case the application will just keep receiving buffers.

2) VIDIOC_DQBUF blocks until a new signal appears with the same timings,
   then the driver will just keep going as if nothing changed. DQBUF
   remains blocked as long as there is no signal, or the timings are
   different from the currently active timings.

3) The hardware requires a hard stop and cannot continue streaming. In
   that case it can call vb2_queue_error().

That last option should be avoided if possible as it's not very polite.
>From what I can tell from this hardware it seems option 2 is the
appropriate choice.

Regards,

	Hans

> 
>> +		vb2_buffer_done(&buf->vb.vb2_buf, state);
>> +	}
>> +	INIT_LIST_HEAD(&video->buffers);
>> +	spin_unlock_irqrestore(&video->lock, flags);
>> +}