Date:   Fri, 28 Jul 2023 15:37:42 +0800
From:   Hsia-Jun Li <Randy.Li@...aptics.com>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Nicolas Dufresne <nicolas@...fresne.ca>,
        linux-media@...r.kernel.org, ayaka@...lik.info,
        hans.verkuil@...co.com, mchehab@...nel.org,
        laurent.pinchart@...asonboard.com, hiroh@...omium.org,
        hverkuil@...all.nl, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] media: v4l2-mem2mem: add a list for buf used by hw



On 7/28/23 15:26, Tomasz Figa wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Fri, Jul 28, 2023 at 4:09 PM Hsia-Jun Li <Randy.Li@...aptics.com> wrote:
>>
>>
>>
>> On 7/28/23 12:43, Tomasz Figa wrote:
>>> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>
>>>
>>> On Fri, Jul 28, 2023 at 1:58 AM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
>>>>
>>>> Le jeudi 27 juillet 2023 à 16:43 +0900, Tomasz Figa a écrit :
>>>>> On Mon, Jul 17, 2023 at 11:07 PM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
>>>>>>
>>>>>> Le mercredi 12 juillet 2023 à 09:33 +0000, Tomasz Figa a écrit :
>>>>>>> On Tue, Jul 04, 2023 at 12:00:38PM +0800, Hsia-Jun Li wrote:
>>>>>>>> From: "Hsia-Jun(Randy) Li" <randy.li@...aptics.com>
>>>>>>>>
>>>>>>>> Many drivers have to create its own buf_struct for a
>>>>>>>> vb2_queue to track such a state. Also driver has to
>>>>>>>> iterate over rdy_queue every times to find out a buffer
>>>>>>>> which is not sent to hardware(or firmware), this new
>>>>>>>> list just offers the driver a place to store the buffer
>>>>>>>> that hardware(firmware) has acknowledged.
>>>>>>>>
>>>>>>>> One important advance about this list, it doesn't like
>>>>>>>> rdy_queue which both bottom half of the user calling
>>>>>>>> could operate it, while the v4l2 worker would as well.
>>>>>>>> The v4l2 core could only operate this queue when its
>>>>>>>> v4l2_context is not running, the driver would only
>>>>>>>> access this new hw_queue in its own worker.
>>>>>>>
>>>>>>> Could you describe in what case such a list would be useful for a
>>>>>>> mem2mem driver?
>>>>>>
>>>>>> Today all driver must track buffers that are "owned by the hardware". This is a
>>>>>> concept dictated by the m2m framework and enforced through the ACTIVE flag. All
>>>>>> buffers from this list must be mark as done/error/queued after streamoff of the
>>>>>> respective queue in order to acknowledge that they are no longer in use by the
>>>>>> HW. Not doing so will warn:
>>>>>>
>>>>>>     videobuf2_common: driver bug: stop_streaming operation is leaving buf ...
>>>>>>
>>>>>> Though, there is no queue to easily iterate them. All driver endup having their
>>>>>> own queue, or just leaving the buffers in the rdy_queue (which isn't better).
>>>>>>
>>>>>
>>>>> Thanks for the explanation. I see how it could be useful now.
>>>>>
>>>>> Although I guess this is a problem specifically for hardware (or
>>>>> firmware) which can internally queue more than 1 buffer, right?
>>>>> Otherwise the current buffer could just stay at the top of the
>>>>> rdy_queue until it's removed by the driver's completion handler,
>>>>> timeout/error handler or context destruction.
>>>>
>>>> Correct, its only an issue when you need to process multiple src buffers before
>>>> producing a dst buffer. If affects stateful decoder, stateful encoders and
>>>> deinterlacer as far as I'm aware.
>>>
>>> Is it actually necessary to keep those buffers in a list in that case, though?
>>> I can see that a deinterlacer would indeed need 2 input buffers to
>>> perform the deinterlacing operation, but those would be just known to
>>> the driver, since it's running the task currently.
>>> For a stateful decoder, wouldn't it just consume the bitstream buffer
>>> (producing something partially decoded to its own internal buffers)
>>> and return it shortly?
>> Display re-order. Firmware could do such batch work, taking a few
>> bitstream buffer, then output a list graphics buffer in the display
>> order also discard the usage of the non-display buffer when it is
>> removed from dpb.
>>
>> Even in one input and one output mode, firmware need to do redo, let the
>> driver know when a graphics buffer could be display, so firmware would
>> usually hold the graphics buffer(frame) until its display time.
>>
> 
> Okay, so that hold would be for frame buffers, not bitstream buffers, right?
For the 1:1 model, decoder won't hold the input(OUTPUT queue) buffer 
usually.
While for the VP9, we have a super frame and temporal unit packing for 
AV1 which break the current API requirement for an AU in a buffer. The 
hardware would trigger multiple work for that(that means multiple irqs 
ack for a usual devices).
For the encoder, it is a different story.
> But yeah, I see that then it could hold onto those buffers until it's
> their turn to display and it could be a bigger number of frames,
> depending on the complexity of the codec.
> 
>> Besides, I hate the driver occupied a large of memory without user's
>> order. I would like to drop those internal buffers.
> 
> I think this is one reason to migrate to the stateless decoder design.
> 
I didn't know such plan here. I don't think the current stateless API 
could export the reconstruction buffers for encoder or post-processing 
buffer for decoder to us.
>>> The most realistic scenario would be for stateful encoders which could
>>> keep some input buffers as reference frames for further encoding, but
>>> then would this patch actually work for them? It would make
>>> __v4l2_m2m_try_queue never add the context to the job_queue if there
>>> are some buffers in that hw_queue list.
>> why?
>>>
>>> Maybe what I need here are actual patches modifying some existing
>>> drivers. Randy, would you be able to include that in the next version?
>> May not. The Synaptics VideoSmart is a secure video platform(DRM), I
>> could release a snapshot of the driver when I got the permission, that
>> would be after the official release of the SDK.
>> But you may not be able to compile it because we have our own TEE
>> interface(not optee), also running it because the trusted app would be
>> signed with a per-device key.
> 
> Could you modify another, already existing driver then?
> 
>>> Thanks.
>>>
>>> Best regards,
>>> Tomasz
>>>
>>>>
>>>> Nicolas
>>>>
>>>>>
>>>>> Best regards,
>>>>> Tomasz
>>>>>
>>>>>> Nicolas
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@...aptics.com>
>>>>>>>> ---
>>>>>>>>    drivers/media/v4l2-core/v4l2-mem2mem.c | 25 +++++++++++++++++--------
>>>>>>>>    include/media/v4l2-mem2mem.h           | 10 +++++++++-
>>>>>>>>    2 files changed, 26 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>>>>>> index c771aba42015..b4151147d5bd 100644
>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>>>>>> @@ -321,15 +321,21 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
>>>>>>>>               goto job_unlock;
>>>>>>>>       }
>>>>>>>>
>>>>>>>> -   src = v4l2_m2m_next_src_buf(m2m_ctx);
>>>>>>>> -   dst = v4l2_m2m_next_dst_buf(m2m_ctx);
>>>>>>>> -   if (!src && !m2m_ctx->out_q_ctx.buffered) {
>>>>>>>> -           dprintk("No input buffers available\n");
>>>>>>>> -           goto job_unlock;
>>>>>>>> +   if (list_empty(&m2m_ctx->out_q_ctx.hw_queue)) {
>>>>>>>> +           src = v4l2_m2m_next_src_buf(m2m_ctx);
>>>>>>>> +
>>>>>>>> +           if (!src && !m2m_ctx->out_q_ctx.buffered) {
>>>>>>>> +                   dprintk("No input buffers available\n");
>>>>>>>> +                   goto job_unlock;
>>>>>>>> +           }
>>>>>>>>       }
>>>>>>>> -   if (!dst && !m2m_ctx->cap_q_ctx.buffered) {
>>>>>>>> -           dprintk("No output buffers available\n");
>>>>>>>> -           goto job_unlock;
>>>>>>>> +
>>>>>>>> +   if (list_empty(&m2m_ctx->cap_q_ctx.hw_queue)) {
>>>>>>>> +           dst = v4l2_m2m_next_dst_buf(m2m_ctx);
>>>>>>>> +           if (!dst && !m2m_ctx->cap_q_ctx.buffered) {
>>>>>>>> +                   dprintk("No output buffers available\n");
>>>>>>>> +                   goto job_unlock;
>>>>>>>> +           }
>>>>>>>>       }
>>>>>>>
>>>>>>> src and dst would be referenced unitialized below if neither of the
>>>>>>> above ifs hits...
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Tomasz
>>>>>>>
>>>>>>>>
>>>>>>>>       m2m_ctx->new_frame = true;
>>>>>>>> @@ -896,6 +902,7 @@ int v4l2_m2m_streamoff(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>>>>>>>>       INIT_LIST_HEAD(&q_ctx->rdy_queue);
>>>>>>>>       q_ctx->num_rdy = 0;
>>>>>>>>       spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
>>>>>>>> +   INIT_LIST_HEAD(&q_ctx->hw_queue);
>>>>>>>>
>>>>>>>>       if (m2m_dev->curr_ctx == m2m_ctx) {
>>>>>>>>               m2m_dev->curr_ctx = NULL;
>>>>>>>> @@ -1234,6 +1241,8 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
>>>>>>>>
>>>>>>>>       INIT_LIST_HEAD(&out_q_ctx->rdy_queue);
>>>>>>>>       INIT_LIST_HEAD(&cap_q_ctx->rdy_queue);
>>>>>>>> +   INIT_LIST_HEAD(&out_q_ctx->hw_queue);
>>>>>>>> +   INIT_LIST_HEAD(&cap_q_ctx->hw_queue);
>>>>>>>>       spin_lock_init(&out_q_ctx->rdy_spinlock);
>>>>>>>>       spin_lock_init(&cap_q_ctx->rdy_spinlock);
>>>>>>>>
>>>>>>>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
>>>>>>>> index d6c8eb2b5201..2342656e582d 100644
>>>>>>>> --- a/include/media/v4l2-mem2mem.h
>>>>>>>> +++ b/include/media/v4l2-mem2mem.h
>>>>>>>> @@ -53,9 +53,16 @@ struct v4l2_m2m_dev;
>>>>>>>>     * processed
>>>>>>>>     *
>>>>>>>>     * @q:             pointer to struct &vb2_queue
>>>>>>>> - * @rdy_queue:     List of V4L2 mem-to-mem queues
>>>>>>>> + * @rdy_queue:     List of V4L2 mem-to-mem queues. If v4l2_m2m_buf_queue() is
>>>>>>>> + *         called in struct vb2_ops->buf_queue(), the buffer enqueued
>>>>>>>> + *         by user would be added to this list.
>>>>>>>>     * @rdy_spinlock: spin lock to protect the struct usage
>>>>>>>>     * @num_rdy:       number of buffers ready to be processed
>>>>>>>> + * @hw_queue:      A list for tracking the buffer is occupied by the hardware
>>>>>>>> + *                 (or device's firmware). A buffer could only be in either
>>>>>>>> + *                 this list or @rdy_queue.
>>>>>>>> + *                 Driver may choose not to use this list while uses its own
>>>>>>>> + *                 private data to do this work.
>>>>>>>>     * @buffered:      is the queue buffered?
>>>>>>>>     *
>>>>>>>>     * Queue for buffers ready to be processed as soon as this
>>>>>>>> @@ -68,6 +75,7 @@ struct v4l2_m2m_queue_ctx {
>>>>>>>>       struct list_head        rdy_queue;
>>>>>>>>       spinlock_t              rdy_spinlock;
>>>>>>>>       u8                      num_rdy;
>>>>>>>> +   struct list_head        hw_queue;
>>>>>>>>       bool                    buffered;
>>>>>>>>    };
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.17.1
>>>>>>>>
>>>>>>
>>>>
>>
>> --
>> Hsia-Jun(Randy) Li

-- 
Hsia-Jun(Randy) Li

Powered by blists - more mailing lists