Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521497 > unrolled thread

Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-14 11:20 +0100
    Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-18 10:20 +0100
      Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Hans Verkuil <hverkuil@xs4all.nl> - 2016-11-21 16:10 +0100
        Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-21 16:30 +0100
          Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Hans Verkuil <hverkuil@xs4all.nl> - 2016-11-21 16:40 +0100
            Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-21 17:20 +0100
              Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Nicolas Dufresne <nicolas@ndufresne.ca> - 2016-11-23 21:30 +0100
                Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-24 14:20 +0100

#1521497 — Re: [PATCH v3 4/9] media: venus: vdec: add video decoder files

Hi Hans,

Thanks for the comments!

On 11/11/2016 01:39 PM, Hans Verkuil wrote:
> I made some comments about start_streaming in my review of patch 3/9, so
> I am not going to repeat that here.

Sure.

> 
> On 11/07/2016 06:33 PM, Stanimir Varbanov wrote:
>> This consists of video decoder implementation plus decoder
>> controls.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/vdec.c       | 1108 ++++++++++++++++++++++++
>>  drivers/media/platform/qcom/venus/vdec.h       |   32 +
>>  drivers/media/platform/qcom/venus/vdec_ctrls.c |  197 +++++
>>  3 files changed, 1337 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/venus/vdec.c
>>  create mode 100644 drivers/media/platform/qcom/venus/vdec.h
>>  create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>> new file mode 100644
>> index 000000000000..3f0eba7e31dc
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
> 
> <snip>
> 
>> +static int
>> +vdec_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
>> +{
>> +	struct venus_inst *inst = to_inst(file);
>> +
>> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>> +	    s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> +		return -EINVAL;
>> +
>> +	switch (s->target) {
>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>> +	case V4L2_SEL_TGT_CROP_DEFAULT:
>> +	case V4L2_SEL_TGT_CROP:
>> +		return -EINVAL;
>> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
>> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>> +	case V4L2_SEL_TGT_COMPOSE_PADDED:
>> +		return -EINVAL;
>> +	case V4L2_SEL_TGT_COMPOSE:
>> +		if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +			return -EINVAL;
>> +		s->r.width = inst->out_width;
>> +		s->r.height = inst->out_height;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	s->r.top = 0;
>> +	s->r.left = 0;
>> +
>> +	return 0;
>> +}
> 
> This can be simplified to just:
> 
> 	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> 	    s->target != V4L2_SEL_TGT_COMPOSE)
> 		return -EINVAL;
> 
> 	// handle the remaining capture compose case

OK I will rewrite it.

> 
>> +
>> +static int
>> +vdec_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>> +{
>> +	struct vb2_queue *queue = to_vb2q(file, b->type);
>> +
>> +	if (!queue)
>> +		return -EINVAL;
>> +
>> +	return vb2_reqbufs(queue, b);
>> +}
> 
> Is there any reason why the v4l2_m2m_ioctl_reqbufs et al helper functions
> can't be used? I strongly recommend that, unless there is a specific reason
> why that won't work.

So that means I need to completely rewrite the v4l2 part and adopt it
for mem2mem device APIs.

If that is what you meant I can invest some time to make a estimation
what would be the changes and time needed. After that we can decide what
to do - take the driver as is and port it to mem2mem device APIs later
on or wait for the this transition to happen before merging.

-- 
regards,
Stan

[toc] | [next] | [standalone]

#1525115

Hi Hans,

>>> +
>>> +static int
>>> +vdec_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>>> +{
>>> +	struct vb2_queue *queue = to_vb2q(file, b->type);
>>> +
>>> +	if (!queue)
>>> +		return -EINVAL;
>>> +
>>> +	return vb2_reqbufs(queue, b);
>>> +}
>>
>> Is there any reason why the v4l2_m2m_ioctl_reqbufs et al helper functions
>> can't be used? I strongly recommend that, unless there is a specific reason
>> why that won't work.
> 
> So that means I need to completely rewrite the v4l2 part and adopt it
> for mem2mem device APIs.
> 
> If that is what you meant I can invest some time to make a estimation
> what would be the changes and time needed. After that we can decide what
> to do - take the driver as is and port it to mem2mem device APIs later
> on or wait for the this transition to happen before merging.
> 

I made an attempt to adopt v4l2 part of the venus driver to m2m API's
and the result was ~300 less lines of code, but with the price of few
extensions in m2m APIs (and I still have issues with running
simultaneously multiple instances).

I have to add few functions/macros to iterate over a list (list_for_each
and friends). This is used to find the returned from decoder buffers by
address and associate them to vb2_buffer, because the decoder can change
the order of the output buffers.

The main problem I have is registering of the capture buffers before
session_start. This is requirement (disadvantage) of the firmware
implementation i.e. I need to announce capture buffers (address and size
of the buffer) to the firmware before start buffer interaction by
session_start.

So having that I think I will need one more week to stabilize the driver
to the state that it was before this m2m transition.

Thoughts?

-- 
regards,
Stan

[toc] | [prev] | [next] | [standalone]

#1526757

On 18/11/16 10:11, Stanimir Varbanov wrote:
> Hi Hans,
>
>>>> +
>>>> +static int
>>>> +vdec_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>>>> +{
>>>> +	struct vb2_queue *queue = to_vb2q(file, b->type);
>>>> +
>>>> +	if (!queue)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return vb2_reqbufs(queue, b);
>>>> +}
>>>
>>> Is there any reason why the v4l2_m2m_ioctl_reqbufs et al helper functions
>>> can't be used? I strongly recommend that, unless there is a specific reason
>>> why that won't work.
>>
>> So that means I need to completely rewrite the v4l2 part and adopt it
>> for mem2mem device APIs.
>>
>> If that is what you meant I can invest some time to make a estimation
>> what would be the changes and time needed. After that we can decide what
>> to do - take the driver as is and port it to mem2mem device APIs later
>> on or wait for the this transition to happen before merging.
>>
>
> I made an attempt to adopt v4l2 part of the venus driver to m2m API's
> and the result was ~300 less lines of code, but with the price of few
> extensions in m2m APIs (and I still have issues with running
> simultaneously multiple instances).
>
> I have to add few functions/macros to iterate over a list (list_for_each
> and friends). This is used to find the returned from decoder buffers by
> address and associate them to vb2_buffer, because the decoder can change
> the order of the output buffers.
>
> The main problem I have is registering of the capture buffers before
> session_start. This is requirement (disadvantage) of the firmware
> implementation i.e. I need to announce capture buffers (address and size
> of the buffer) to the firmware before start buffer interaction by
> session_start.
>
> So having that I think I will need one more week to stabilize the driver
> to the state that it was before this m2m transition.
>
> Thoughts?
>

It sounds like this it worth doing, since if you need these extensions, then
it is likely someone else will need it as well.

Can you mail me a preliminary patch with the core m2m changes? That will be
helpful for me to look at.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1526785

Hi Hans,

On 11/21/2016 05:04 PM, Hans Verkuil wrote:
> On 18/11/16 10:11, Stanimir Varbanov wrote:
>> Hi Hans,
>>
>>>>> +
>>>>> +static int
>>>>> +vdec_reqbufs(struct file *file, void *fh, struct
>>>>> v4l2_requestbuffers *b)
>>>>> +{
>>>>> +    struct vb2_queue *queue = to_vb2q(file, b->type);
>>>>> +
>>>>> +    if (!queue)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    return vb2_reqbufs(queue, b);
>>>>> +}
>>>>
>>>> Is there any reason why the v4l2_m2m_ioctl_reqbufs et al helper
>>>> functions
>>>> can't be used? I strongly recommend that, unless there is a specific
>>>> reason
>>>> why that won't work.
>>>
>>> So that means I need to completely rewrite the v4l2 part and adopt it
>>> for mem2mem device APIs.
>>>
>>> If that is what you meant I can invest some time to make a estimation
>>> what would be the changes and time needed. After that we can decide what
>>> to do - take the driver as is and port it to mem2mem device APIs later
>>> on or wait for the this transition to happen before merging.
>>>
>>
>> I made an attempt to adopt v4l2 part of the venus driver to m2m API's
>> and the result was ~300 less lines of code, but with the price of few
>> extensions in m2m APIs (and I still have issues with running
>> simultaneously multiple instances).
>>
>> I have to add few functions/macros to iterate over a list (list_for_each
>> and friends). This is used to find the returned from decoder buffers by
>> address and associate them to vb2_buffer, because the decoder can change
>> the order of the output buffers.
>>
>> The main problem I have is registering of the capture buffers before
>> session_start. This is requirement (disadvantage) of the firmware
>> implementation i.e. I need to announce capture buffers (address and size
>> of the buffer) to the firmware before start buffer interaction by
>> session_start.
>>
>> So having that I think I will need one more week to stabilize the driver
>> to the state that it was before this m2m transition.
>>
>> Thoughts?
>>
> 
> It sounds like this it worth doing, since if you need these extensions,
> then
> it is likely someone else will need it as well.

Meanwhile I have found bigger obstacle - I cannot run multiple instances
simultaneously. By m2m design it can execute only one job (m2m context)
at a time per m2m device. Can you confirm that my observation is correct?

If so one solution could be on every fops::open I can create m2m_dev and
init m2m_cxt.

> 
> Can you mail me a preliminary patch with the core m2m changes? That will be
> helpful for me to look at.

Something like below diffs:

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 61d56c940f80..52e22ec0f67b 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -136,6 +136,28 @@ void *v4l2_m2m_buf_remove(struct v4l2_m2m_queue_ctx
*q_ctx)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove);

+struct vb2_v4l2_buffer *
+v4l2_m2m_buf_remove_match(struct v4l2_m2m_queue_ctx *q_ctx, void *priv,
+                          int (*match)(void *priv, struct
vb2_v4l2_buffer *vb))
+{
+       struct v4l2_m2m_buffer *b, *tmp;
+       struct vb2_v4l2_buffer *ret = NULL;
+       unsigned long flags;
+
+       spin_lock_irqsave(&q_ctx->rdy_spinlock, flags);
+       list_for_each_entry_safe(b, tmp, &q_ctx->rdy_queue, list) {
+               if (match(priv, &b->vb)) {
+                       list_del(&b->list);
+                       ret = &b->vb;
+                       break;
+               }
+       }
+       spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_match);
+
 /*
  * Scheduling handlers
  */
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 64e1819ea66d..e943609209ba 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -263,6 +263,24 @@ static inline void *v4l2_m2m_dst_buf_remove(struct
v4l2_m2m_ctx *m2m_ctx)
        return v4l2_m2m_buf_remove(&m2m_ctx->cap_q_ctx);
 }

+struct vb2_v4l2_buffer *
+v4l2_m2m_buf_remove_match(struct v4l2_m2m_queue_ctx *q_ctx, void *priv,
+                       int (*match)(void *priv, struct vb2_v4l2_buffer
*vb));
+
+static inline struct vb2_v4l2_buffer *
+v4l2_m2m_src_buf_remove_match(struct v4l2_m2m_ctx *m2m_ctx, void *priv,
+                       int (*match)(void *priv, struct vb2_v4l2_buffer
*vb))
+{
+       return v4l2_m2m_buf_remove_match(&m2m_ctx->out_q_ctx, priv, match);
+}
+
+static inline struct vb2_v4l2_buffer *
+v4l2_m2m_dst_buf_remove_match(struct v4l2_m2m_ctx *m2m_ctx, void *priv,
+                       int (*match)(void *priv, struct vb2_v4l2_buffer
*vb))
+{
+       return v4l2_m2m_buf_remove_match(&m2m_ctx->cap_q_ctx, priv, match);
+}

diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 5a9597dd1ee0..64e1819ea66d 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -211,6 +211,12 @@ static inline void *v4l2_m2m_next_dst_buf(struct
v4l2_m2m_ctx *m2m_ctx)
        return v4l2_m2m_next_buf(&m2m_ctx->cap_q_ctx);
 }

+#define v4l2_m2m_for_each_dst_buf(q_ctx, b)    \
+       list_for_each_entry(b, &q_ctx->cap_q_ctx.rdy_queue, list)
+
+#define v4l2_m2m_for_each_src_buf(q_ctx, b)    \
+       list_for_each_entry(b, &q_ctx->out_q_ctx.rdy_queue, list)
+
 /**
  * v4l2_m2m_get_src_vq() - return vb2_queue for source buffers
  *

-- 
regards,
Stan

[toc] | [prev] | [next] | [standalone]

#1526801

On 21/11/16 16:29, Stanimir Varbanov wrote:
> Hi Hans,
>
> On 11/21/2016 05:04 PM, Hans Verkuil wrote:
>> On 18/11/16 10:11, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>>>>> +
>>>>>> +static int
>>>>>> +vdec_reqbufs(struct file *file, void *fh, struct
>>>>>> v4l2_requestbuffers *b)
>>>>>> +{
>>>>>> +    struct vb2_queue *queue = to_vb2q(file, b->type);
>>>>>> +
>>>>>> +    if (!queue)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    return vb2_reqbufs(queue, b);
>>>>>> +}
>>>>>
>>>>> Is there any reason why the v4l2_m2m_ioctl_reqbufs et al helper
>>>>> functions
>>>>> can't be used? I strongly recommend that, unless there is a specific
>>>>> reason
>>>>> why that won't work.
>>>>
>>>> So that means I need to completely rewrite the v4l2 part and adopt it
>>>> for mem2mem device APIs.
>>>>
>>>> If that is what you meant I can invest some time to make a estimation
>>>> what would be the changes and time needed. After that we can decide what
>>>> to do - take the driver as is and port it to mem2mem device APIs later
>>>> on or wait for the this transition to happen before merging.
>>>>
>>>
>>> I made an attempt to adopt v4l2 part of the venus driver to m2m API's
>>> and the result was ~300 less lines of code, but with the price of few
>>> extensions in m2m APIs (and I still have issues with running
>>> simultaneously multiple instances).
>>>
>>> I have to add few functions/macros to iterate over a list (list_for_each
>>> and friends). This is used to find the returned from decoder buffers by
>>> address and associate them to vb2_buffer, because the decoder can change
>>> the order of the output buffers.
>>>
>>> The main problem I have is registering of the capture buffers before
>>> session_start. This is requirement (disadvantage) of the firmware
>>> implementation i.e. I need to announce capture buffers (address and size
>>> of the buffer) to the firmware before start buffer interaction by
>>> session_start.
>>>
>>> So having that I think I will need one more week to stabilize the driver
>>> to the state that it was before this m2m transition.
>>>
>>> Thoughts?
>>>
>>
>> It sounds like this it worth doing, since if you need these extensions,
>> then
>> it is likely someone else will need it as well.
>
> Meanwhile I have found bigger obstacle - I cannot run multiple instances
> simultaneously. By m2m design it can execute only one job (m2m context)
> at a time per m2m device. Can you confirm that my observation is correct?

The m2m framework assumes a single HW instance, yes. Do you have multiple
HW decoders? I might not understand what you mean...

Regards,

	Hans

>
> If so one solution could be on every fops::open I can create m2m_dev and
> init m2m_cxt.
>
>>
>> Can you mail me a preliminary patch with the core m2m changes? That will be
>> helpful for me to look at.
>
> Something like below diffs:
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 61d56c940f80..52e22ec0f67b 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -136,6 +136,28 @@ void *v4l2_m2m_buf_remove(struct v4l2_m2m_queue_ctx
> *q_ctx)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove);
>
> +struct vb2_v4l2_buffer *
> +v4l2_m2m_buf_remove_match(struct v4l2_m2m_queue_ctx *q_ctx, void *priv,
> +                          int (*match)(void *priv, struct
> vb2_v4l2_buffer *vb))
> +{
> +       struct v4l2_m2m_buffer *b, *tmp;
> +       struct vb2_v4l2_buffer *ret = NULL;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&q_ctx->rdy_spinlock, flags);
> +       list_for_each_entry_safe(b, tmp, &q_ctx->rdy_queue, list) {
> +               if (match(priv, &b->vb)) {
> +                       list_del(&b->list);
> +                       ret = &b->vb;
> +                       break;
> +               }
> +       }
> +       spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_match);
> +
>  /*
>   * Scheduling handlers
>   */
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 64e1819ea66d..e943609209ba 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -263,6 +263,24 @@ static inline void *v4l2_m2m_dst_buf_remove(struct
> v4l2_m2m_ctx *m2m_ctx)
>         return v4l2_m2m_buf_remove(&m2m_ctx->cap_q_ctx);
>  }
>
> +struct vb2_v4l2_buffer *
> +v4l2_m2m_buf_remove_match(struct v4l2_m2m_queue_ctx *q_ctx, void *priv,
> +                       int (*match)(void *priv, struct vb2_v4l2_buffer
> *vb));
> +
> +static inline struct vb2_v4l2_buffer *
> +v4l2_m2m_src_buf_remove_match(struct v4l2_m2m_ctx *m2m_ctx, void *priv,
> +                       int (*match)(void *priv, struct vb2_v4l2_buffer
> *vb))
> +{
> +       return v4l2_m2m_buf_remove_match(&m2m_ctx->out_q_ctx, priv, match);
> +}
> +
> +static inline struct vb2_v4l2_buffer *
> +v4l2_m2m_dst_buf_remove_match(struct v4l2_m2m_ctx *m2m_ctx, void *priv,
> +                       int (*match)(void *priv, struct vb2_v4l2_buffer
> *vb))
> +{
> +       return v4l2_m2m_buf_remove_match(&m2m_ctx->cap_q_ctx, priv, match);
> +}
>
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 5a9597dd1ee0..64e1819ea66d 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -211,6 +211,12 @@ static inline void *v4l2_m2m_next_dst_buf(struct
> v4l2_m2m_ctx *m2m_ctx)
>         return v4l2_m2m_next_buf(&m2m_ctx->cap_q_ctx);
>  }
>
> +#define v4l2_m2m_for_each_dst_buf(q_ctx, b)    \
> +       list_for_each_entry(b, &q_ctx->cap_q_ctx.rdy_queue, list)
> +
> +#define v4l2_m2m_for_each_src_buf(q_ctx, b)    \
> +       list_for_each_entry(b, &q_ctx->out_q_ctx.rdy_queue, list)
> +
>  /**
>   * v4l2_m2m_get_src_vq() - return vb2_queue for source buffers
>   *
>

[toc] | [prev] | [next] | [standalone]

#1526858

On 11/21/2016 05:33 PM, Hans Verkuil wrote:
> On 21/11/16 16:29, Stanimir Varbanov wrote:
>> Hi Hans,
>>
>> On 11/21/2016 05:04 PM, Hans Verkuil wrote:
>>> On 18/11/16 10:11, Stanimir Varbanov wrote:
>>>> Hi Hans,
>>>>
>>>>>>> +
>>>>>>> +static int
>>>>>>> +vdec_reqbufs(struct file *file, void *fh, struct
>>>>>>> v4l2_requestbuffers *b)
>>>>>>> +{
>>>>>>> +    struct vb2_queue *queue = to_vb2q(file, b->type);
>>>>>>> +
>>>>>>> +    if (!queue)
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    return vb2_reqbufs(queue, b);
>>>>>>> +}
>>>>>>
>>>>>> Is there any reason why the v4l2_m2m_ioctl_reqbufs et al helper
>>>>>> functions
>>>>>> can't be used? I strongly recommend that, unless there is a specific
>>>>>> reason
>>>>>> why that won't work.
>>>>>
>>>>> So that means I need to completely rewrite the v4l2 part and adopt it
>>>>> for mem2mem device APIs.
>>>>>
>>>>> If that is what you meant I can invest some time to make a estimation
>>>>> what would be the changes and time needed. After that we can decide
>>>>> what
>>>>> to do - take the driver as is and port it to mem2mem device APIs later
>>>>> on or wait for the this transition to happen before merging.
>>>>>
>>>>
>>>> I made an attempt to adopt v4l2 part of the venus driver to m2m API's
>>>> and the result was ~300 less lines of code, but with the price of few
>>>> extensions in m2m APIs (and I still have issues with running
>>>> simultaneously multiple instances).
>>>>
>>>> I have to add few functions/macros to iterate over a list
>>>> (list_for_each
>>>> and friends). This is used to find the returned from decoder buffers by
>>>> address and associate them to vb2_buffer, because the decoder can
>>>> change
>>>> the order of the output buffers.
>>>>
>>>> The main problem I have is registering of the capture buffers before
>>>> session_start. This is requirement (disadvantage) of the firmware
>>>> implementation i.e. I need to announce capture buffers (address and
>>>> size
>>>> of the buffer) to the firmware before start buffer interaction by
>>>> session_start.
>>>>
>>>> So having that I think I will need one more week to stabilize the
>>>> driver
>>>> to the state that it was before this m2m transition.
>>>>
>>>> Thoughts?
>>>>
>>>
>>> It sounds like this it worth doing, since if you need these extensions,
>>> then
>>> it is likely someone else will need it as well.
>>
>> Meanwhile I have found bigger obstacle - I cannot run multiple instances
>> simultaneously. By m2m design it can execute only one job (m2m context)
>> at a time per m2m device. Can you confirm that my observation is correct?
> 
> The m2m framework assumes a single HW instance, yes. Do you have multiple
> HW decoders? I might not understand what you mean...
> 

I mean that I can start and execute up to 16 decoder sessions
simultaneously. Its a firmware responsibility how those sessions are
scheduled and how the hardware is shared between them. Of course
depending on the resolution the firmware can refuse to start the session
because the hardware will be overloaded and will not be able to satisfy
the bitrate requirements.

-- 
regards,
Stan

[toc] | [prev] | [next] | [standalone]

#1528759

[Multipart message — attachments visible in raw view] — view raw

Le lundi 21 novembre 2016 à 18:09 +0200, Stanimir Varbanov a écrit :
> >> Meanwhile I have found bigger obstacle - I cannot run multiple
> instances
> >> simultaneously. By m2m design it can execute only one job (m2m
> context)
> >> at a time per m2m device. Can you confirm that my observation is
> correct?
> > 
> > The m2m framework assumes a single HW instance, yes. Do you have
> multiple
> > HW decoders? I might not understand what you mean...
> > 
> 
> I mean that I can start and execute up to 16 decoder sessions
> simultaneously. Its a firmware responsibility how those sessions are
> scheduled and how the hardware is shared between them. Of course
> depending on the resolution the firmware can refuse to start the
> session
> because the hardware will be overloaded and will not be able to
> satisfy
> the bitrate requirements.

This is similar to S5P-MFC driver, which you may have notice not use
m2m framework.

Nicolas

[toc] | [prev] | [next] | [standalone]

#1529265

Hi Nicolas,

On 11/23/2016 10:24 PM, Nicolas Dufresne wrote:
> Le lundi 21 novembre 2016 à 18:09 +0200, Stanimir Varbanov a écrit :
>>>> Meanwhile I have found bigger obstacle - I cannot run multiple
>> instances
>>>> simultaneously. By m2m design it can execute only one job (m2m
>> context)
>>>> at a time per m2m device. Can you confirm that my observation is
>> correct?
>>>  
>>> The m2m framework assumes a single HW instance, yes. Do you have
>> multiple
>>> HW decoders? I might not understand what you mean...
>>>  
>>
>> I mean that I can start and execute up to 16 decoder sessions
>> simultaneously. Its a firmware responsibility how those sessions are
>> scheduled and how the hardware is shared between them. Of course
>> depending on the resolution the firmware can refuse to start the
>> session
>> because the hardware will be overloaded and will not be able to
>> satisfy
>> the bitrate requirements.
> 
> This is similar to S5P-MFC driver, which you may have notice not use
> m2m framework.

Thanks for the note.

I have started to look into m2m because Hans asked me to reuse the ioctl
helpers that it provides.

I have no problem with usage of the m2m API if they help me to reduce
code size and doesn't impact performance.


-- 
regards,
Stan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web