Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
Hi,
Le jeudi 06 septembre 2018 à 09:22 +0200, Hans Verkuil a écrit :
> On 09/06/2018 09:01 AM, Hans Verkuil wrote:
> > On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> > > Hi and thanks for the review!
> > >
> > > Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> > > > On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> > > > > +static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int
> > > > > *nbufs,
> > > > > + unsigned int *nplanes, unsigned int
> > > > > sizes[],
> > > > > + struct device *alloc_devs[])
> > > > > +{
> > > > > + struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > + struct cedrus_dev *dev = ctx->dev;
> > > > > + struct v4l2_pix_format_mplane *mplane_fmt;
> > > > > + struct cedrus_format *fmt;
> > > > > + unsigned int i;
> > > > > +
> > > > > + switch (vq->type) {
> > > > > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > > > + mplane_fmt = >src_fmt;
> > > > > + fmt = cedrus_find_format(mplane_fmt->pixelformat,
> > > > > + CEDRUS_DECODE_SRC,
> > > > > + dev->capabilities);
> > > > > + break;
> > > > > +
> > > > > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > > > + mplane_fmt = >dst_fmt;
> > > > > + fmt = cedrus_find_format(mplane_fmt->pixelformat,
> > > > > + CEDRUS_DECODE_DST,
> > > > > + dev->capabilities);
> > > > > + break;
> > > > > +
> > > > > + default:
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + if (!fmt)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (fmt->num_buffers == 1) {
> > > > > + sizes[0] = 0;
> > > > > +
> > > > > + for (i = 0; i < fmt->num_planes; i++)
> > > > > + sizes[0] += mplane_fmt->plane_fmt[i].sizeimage;
> > > > > + } else if (fmt->num_buffers == fmt->num_planes) {
> > > > > + for (i = 0; i < fmt->num_planes; i++)
> > > > > + sizes[i] = mplane_fmt->plane_fmt[i].sizeimage;
> > > > > + } else {
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + *nplanes = fmt->num_buffers;
> > > >
> > > > This code does not take VIDIOC_CREATE_BUFFERS into account.
> > > >
> > > > If it is called from that ioctl, then *nplanes is non-zero and you need
> > > > to check if *nplanes equals fmt->num_buffers and that sizes[n] is >=
> > > > the required size of the format. If so, then return 0, otherwise return
> > > > -EINVAL.
> > >
> > > Thanks for spotting this, I'll fix it as you suggested in the next
> > > revision.
> > >
> > > > Doesn't v4l2-compliance fail on that? Or is that test skipped because
> > > > this
> > > > is a decoder for which streaming is not supported (yet)?
> > >
> > > Apparently, v4l2-compliance doesn't fail since I'm getting:
> > > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >
> > It is tested, but only with the -s option. I'll see if I can improve the
> > tests.
>
> I've improved the tests. v4l2-compliance should now fail when run (without the
> -s option) against this driver. Can you check that that is indeed the case?
I think this wasn't being tested with v8 of the driver as no default
format was provided (for the G_FMT test), which probably led to MMAP
not being picked up in valid_memorytype. Thus the subsequent
CREATE_BUFS test was reported as not failing, but it really didn't test
much of anything.
Cheers,
Paul
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On Thu, Sep 6, 2018 at 4:39 PM Hans Verkuil wrote:
>
> On 09/06/2018 09:25 AM, Tomasz Figa wrote:
> > On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil wrote:
> >>
> >> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> >>> Hi and thanks for the review!
> >>>
> >>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> > +static int cedrus_request_validate(struct media_request *req)
> > +{
> > + struct media_request_object *obj, *obj_safe;
> > + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> > + struct cedrus_ctx *ctx = NULL;
> > + struct v4l2_ctrl *ctrl_test;
> > + unsigned int i;
> > +
> > + list_for_each_entry_safe(obj, obj_safe, >objects, list) {
>
> You don't need to use the _safe variant during validation.
> >>>
> >>> Okay, I'll use the regular one then.
> >>>
> > + struct vb2_buffer *vb;
> > +
> > + if (vb2_request_object_is_buffer(obj)) {
> > + vb = container_of(obj, struct vb2_buffer, req_obj);
> > + ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > + break;
> > + }
> > + }
>
> Interesting question: what happens if more than one buffer is queued in
> the
> request? This is allowed by the request API and in that case the
> associated
> controls in the request apply to all queued buffers.
>
> Would this make sense at all for this driver? If not, then you need to
> check here if there is more than one buffer in the request and document
> in
> the spec that this is not allowed.
> >>>
> >>> Well, our driver was written with the (unformal) assumption that we
> >>> only deal with a pair of one output and one capture buffer. So I will
> >>> add a check for this at request validation time and document it in the
> >>> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
> >>> duplicated for future formats we add support for)?
> >>
> >> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
> >> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
> >> in the request?
> >>
> >> Then you can call it here to check that you have only one buffer.
> >>
> >> And this has to be documented with the PIXFMT.
> >>
> >> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
> >> vivid happily accept that), so this is an exception that should be
> >> documented and checked in the codec driver.
> >
> > Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
> > included in the request?
>
> No. The request API allows multiple buffers for the same vb2_queue to be
> queued for the same request (obviously when the request is committed, the
> buffers are queued to the driver in the same order).
>
> >
> > If we indeed allow multiple buffers for the same queue in a request,
> > we shouldn't restrict this on a per-driver basis. It's definitely not
> > a hardware limitation, since the driver could just do the same as if 2
> > requests with the same controls were given.
>
> That's how it operates: for all buffers in the request the same controls
> apply. But does this make sense for codecs? If the control(s) with the
> codec metadata always change for every buffer, then having more than one
> buffer in the request is senseless and the driver should check for this
> in the validation step.
>
> If it *does* make sense in some circumstances to have the same metadata
> for multiple buffers, then it should be checked if the cedrus driver
> handles this correctly.
Just FYI, we may want to move this discussion to Alex's RFC with
documentation of stateless interface:
https://patchwork.kernel.org/patch/10583233/
Best regards,
Tomasz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On 09/06/2018 09:25 AM, Tomasz Figa wrote:
> On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil wrote:
>>
>> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
>>> Hi and thanks for the review!
>>>
>>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> +static int cedrus_request_validate(struct media_request *req)
> +{
> + struct media_request_object *obj, *obj_safe;
> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> + struct cedrus_ctx *ctx = NULL;
> + struct v4l2_ctrl *ctrl_test;
> + unsigned int i;
> +
> + list_for_each_entry_safe(obj, obj_safe, >objects, list) {
You don't need to use the _safe variant during validation.
>>>
>>> Okay, I'll use the regular one then.
>>>
> + struct vb2_buffer *vb;
> +
> + if (vb2_request_object_is_buffer(obj)) {
> + vb = container_of(obj, struct vb2_buffer, req_obj);
> + ctx = vb2_get_drv_priv(vb->vb2_queue);
> +
> + break;
> + }
> + }
Interesting question: what happens if more than one buffer is queued in the
request? This is allowed by the request API and in that case the associated
controls in the request apply to all queued buffers.
Would this make sense at all for this driver? If not, then you need to
check here if there is more than one buffer in the request and document in
the spec that this is not allowed.
>>>
>>> Well, our driver was written with the (unformal) assumption that we
>>> only deal with a pair of one output and one capture buffer. So I will
>>> add a check for this at request validation time and document it in the
>>> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
>>> duplicated for future formats we add support for)?
>>
>> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
>> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
>> in the request?
>>
>> Then you can call it here to check that you have only one buffer.
>>
>> And this has to be documented with the PIXFMT.
>>
>> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
>> vivid happily accept that), so this is an exception that should be
>> documented and checked in the codec driver.
>
> Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
> included in the request?
No. The request API allows multiple buffers for the same vb2_queue to be
queued for the same request (obviously when the request is committed, the
buffers are queued to the driver in the same order).
>
> If we indeed allow multiple buffers for the same queue in a request,
> we shouldn't restrict this on a per-driver basis. It's definitely not
> a hardware limitation, since the driver could just do the same as if 2
> requests with the same controls were given.
That's how it operates: for all buffers in the request the same controls
apply. But does this make sense for codecs? If the control(s) with the
codec metadata always change for every buffer, then having more than one
buffer in the request is senseless and the driver should check for this
in the validation step.
If it *does* make sense in some circumstances to have the same metadata
for multiple buffers, then it should be checked if the cedrus driver
handles this correctly.
Regards,
Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil wrote:
>
> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> > Hi and thanks for the review!
> >
> > Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> >> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> >>> +static int cedrus_request_validate(struct media_request *req)
> >>> +{
> >>> + struct media_request_object *obj, *obj_safe;
> >>> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> >>> + struct cedrus_ctx *ctx = NULL;
> >>> + struct v4l2_ctrl *ctrl_test;
> >>> + unsigned int i;
> >>> +
> >>> + list_for_each_entry_safe(obj, obj_safe, >objects, list) {
> >>
> >> You don't need to use the _safe variant during validation.
> >
> > Okay, I'll use the regular one then.
> >
> >>> + struct vb2_buffer *vb;
> >>> +
> >>> + if (vb2_request_object_is_buffer(obj)) {
> >>> + vb = container_of(obj, struct vb2_buffer, req_obj);
> >>> + ctx = vb2_get_drv_priv(vb->vb2_queue);
> >>> +
> >>> + break;
> >>> + }
> >>> + }
> >>
> >> Interesting question: what happens if more than one buffer is queued in the
> >> request? This is allowed by the request API and in that case the associated
> >> controls in the request apply to all queued buffers.
> >>
> >> Would this make sense at all for this driver? If not, then you need to
> >> check here if there is more than one buffer in the request and document in
> >> the spec that this is not allowed.
> >
> > Well, our driver was written with the (unformal) assumption that we
> > only deal with a pair of one output and one capture buffer. So I will
> > add a check for this at request validation time and document it in the
> > spec. Should that be part of the MPEG-2 PIXFMT documentation (and
> > duplicated for future formats we add support for)?
>
> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
> in the request?
>
> Then you can call it here to check that you have only one buffer.
>
> And this has to be documented with the PIXFMT.
>
> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
> vivid happily accept that), so this is an exception that should be
> documented and checked in the codec driver.
Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
included in the request?
If we indeed allow multiple buffers for the same queue in a request,
we shouldn't restrict this on a per-driver basis. It's definitely not
a hardware limitation, since the driver could just do the same as if 2
requests with the same controls were given.
Best regards,
Tomasz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On 09/06/2018 09:01 AM, Hans Verkuil wrote:
> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
>> Hi and thanks for the review!
>>
>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
>>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
+static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
+unsigned int *nplanes, unsigned int sizes[],
+struct device *alloc_devs[])
+{
+ struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
+ struct cedrus_dev *dev = ctx->dev;
+ struct v4l2_pix_format_mplane *mplane_fmt;
+ struct cedrus_format *fmt;
+ unsigned int i;
+
+ switch (vq->type) {
+ case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+ mplane_fmt = >src_fmt;
+ fmt = cedrus_find_format(mplane_fmt->pixelformat,
+ CEDRUS_DECODE_SRC,
+ dev->capabilities);
+ break;
+
+ case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+ mplane_fmt = >dst_fmt;
+ fmt = cedrus_find_format(mplane_fmt->pixelformat,
+ CEDRUS_DECODE_DST,
+ dev->capabilities);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ if (!fmt)
+ return -EINVAL;
+
+ if (fmt->num_buffers == 1) {
+ sizes[0] = 0;
+
+ for (i = 0; i < fmt->num_planes; i++)
+ sizes[0] += mplane_fmt->plane_fmt[i].sizeimage;
+ } else if (fmt->num_buffers == fmt->num_planes) {
+ for (i = 0; i < fmt->num_planes; i++)
+ sizes[i] = mplane_fmt->plane_fmt[i].sizeimage;
+ } else {
+ return -EINVAL;
+ }
+
+ *nplanes = fmt->num_buffers;
>>>
>>> This code does not take VIDIOC_CREATE_BUFFERS into account.
>>>
>>> If it is called from that ioctl, then *nplanes is non-zero and you need
>>> to check if *nplanes equals fmt->num_buffers and that sizes[n] is >=
>>> the required size of the format. If so, then return 0, otherwise return
>>> -EINVAL.
>>
>> Thanks for spotting this, I'll fix it as you suggested in the next
>> revision.
>>
>>> Doesn't v4l2-compliance fail on that? Or is that test skipped because this
>>> is a decoder for which streaming is not supported (yet)?
>>
>> Apparently, v4l2-compliance doesn't fail since I'm getting:
>> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>
> It is tested, but only with the -s option. I'll see if I can improve the
> tests.
I've improved the tests. v4l2-compliance should now fail when run (without the
-s option) against this driver. Can you check that that is indeed the case?
Thanks!
Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> Hi and thanks for the review!
>
> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
>>> +static int cedrus_request_validate(struct media_request *req)
>>> +{
>>> + struct media_request_object *obj, *obj_safe;
>>> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
>>> + struct cedrus_ctx *ctx = NULL;
>>> + struct v4l2_ctrl *ctrl_test;
>>> + unsigned int i;
>>> +
>>> + list_for_each_entry_safe(obj, obj_safe, >objects, list) {
>>
>> You don't need to use the _safe variant during validation.
>
> Okay, I'll use the regular one then.
>
>>> + struct vb2_buffer *vb;
>>> +
>>> + if (vb2_request_object_is_buffer(obj)) {
>>> + vb = container_of(obj, struct vb2_buffer, req_obj);
>>> + ctx = vb2_get_drv_priv(vb->vb2_queue);
>>> +
>>> + break;
>>> + }
>>> + }
>>
>> Interesting question: what happens if more than one buffer is queued in the
>> request? This is allowed by the request API and in that case the associated
>> controls in the request apply to all queued buffers.
>>
>> Would this make sense at all for this driver? If not, then you need to
>> check here if there is more than one buffer in the request and document in
>> the spec that this is not allowed.
>
> Well, our driver was written with the (unformal) assumption that we
> only deal with a pair of one output and one capture buffer. So I will
> add a check for this at request validation time and document it in the
> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
> duplicated for future formats we add support for)?
Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
renaming it to vb2_request_buffer_cnt() and returning the number of buffers
in the request?
Then you can call it here to check that you have only one buffer.
And this has to be documented with the PIXFMT.
Multiple buffers are certainly possible in non-codec scenarios (vim2m and
vivid happily accept that), so this is an exception that should be
documented and checked in the codec driver.
>
>> If it does make sense, then you need to test this.
>>
>> Again, this can be corrected in a follow-up patch, unless there will be a
>> v9 anyway.
>
> [...]
>>> +static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
>>> + unsigned int *nplanes, unsigned int sizes[],
>>> + struct device *alloc_devs[])
>>> +{
>>> + struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
>>> + struct cedrus_dev *dev = ctx->dev;
>>> + struct v4l2_pix_format_mplane *mplane_fmt;
>>> + struct cedrus_format *fmt;
>>> + unsigned int i;
>>> +
>>> + switch (vq->type) {
>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> + mplane_fmt = >src_fmt;
>>> + fmt = cedrus_find_format(mplane_fmt->pixelformat,
>>> +CEDRUS_DECODE_SRC,
>>> +dev->capabilities);
>>> + break;
>>> +
>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> + mplane_fmt = >dst_fmt;
>>> + fmt = cedrus_find_format(mplane_fmt->pixelformat,
>>> +CEDRUS_DECODE_DST,
>>> +dev->capabilities);
>>> + break;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!fmt)
>>> + return -EINVAL;
>>> +
>>> + if (fmt->num_buffers == 1) {
>>> + sizes[0] = 0;
>>> +
>>> + for (i = 0; i < fmt->num_planes; i++)
>>> + sizes[0] += mplane_fmt->plane_fmt[i].sizeimage;
>>> + } else if (fmt->num_buffers == fmt->num_planes) {
>>> + for (i = 0; i < fmt->num_planes; i++)
>>> + sizes[i] = mplane_fmt->plane_fmt[i].sizeimage;
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + *nplanes = fmt->num_buffers;
>>
>> This code does not take VIDIOC_CREATE_BUFFERS into account.
>>
>> If it is called from that ioctl, then *nplanes is non-zero and you need
>> to check if *nplanes equals fmt->num_buffers and that sizes[n] is >=
>> the required size of the format. If so, then return 0, otherwise return
>> -EINVAL.
>
> Thanks for spotting this, I'll fix it as you suggested in the next
> revision.
>
>> Doesn't v4l2-compliance fail on that? Or is that test skipped because this
>> is a decoder for which streaming is not supported (yet)?
>
> Apparently, v4l2-compliance doesn't fail since I'm getting:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
It is tested, but only with the -s option. I'll see if I can improve the
tests.
Regards,
Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
Hi and thanks for the review!
Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> > +static int cedrus_request_validate(struct media_request *req)
> > +{
> > + struct media_request_object *obj, *obj_safe;
> > + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> > + struct cedrus_ctx *ctx = NULL;
> > + struct v4l2_ctrl *ctrl_test;
> > + unsigned int i;
> > +
> > + list_for_each_entry_safe(obj, obj_safe, >objects, list) {
>
> You don't need to use the _safe variant during validation.
Okay, I'll use the regular one then.
> > + struct vb2_buffer *vb;
> > +
> > + if (vb2_request_object_is_buffer(obj)) {
> > + vb = container_of(obj, struct vb2_buffer, req_obj);
> > + ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > + break;
> > + }
> > + }
>
> Interesting question: what happens if more than one buffer is queued in the
> request? This is allowed by the request API and in that case the associated
> controls in the request apply to all queued buffers.
>
> Would this make sense at all for this driver? If not, then you need to
> check here if there is more than one buffer in the request and document in
> the spec that this is not allowed.
Well, our driver was written with the (unformal) assumption that we
only deal with a pair of one output and one capture buffer. So I will
add a check for this at request validation time and document it in the
spec. Should that be part of the MPEG-2 PIXFMT documentation (and
duplicated for future formats we add support for)?
> If it does make sense, then you need to test this.
>
> Again, this can be corrected in a follow-up patch, unless there will be a
> v9 anyway.
[...]
> > +static int cedrus_probe(struct platform_device *pdev)
> > +{
> > + struct cedrus_dev *dev;
> > + struct video_device *vfd;
> > + int ret;
> > +
> > + dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
> > + if (!dev)
> > + return -ENOMEM;
> > +
> > + dev->dev = >dev;
> > + dev->pdev = pdev;
> > +
> > + ret = cedrus_hw_probe(dev);
> > + if (ret) {
> > + dev_err(>dev, "Failed to probe hardware\n");
> > + return ret;
> > + }
> > +
> > + dev->dec_ops[CEDRUS_CODEC_MPEG2] = _dec_ops_mpeg2;
> > +
> > + mutex_init(>dev_mutex);
> > + spin_lock_init(>irq_lock);
> > +
> > + ret = v4l2_device_register(>dev, >v4l2_dev);
> > + if (ret) {
> > + dev_err(>dev, "Failed to register V4L2 device\n");
> > + return ret;
> > + }
> > +
> > + dev->vfd = cedrus_video_device;
> > + vfd = >vfd;
> > + vfd->lock = >dev_mutex;
> > + vfd->v4l2_dev = >v4l2_dev;
> > +
> > + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > + if (ret) {
> > + v4l2_err(>v4l2_dev, "Failed to register video device\n");
> > + goto err_v4l2;
> > + }
> > +
> > + snprintf(vfd->name, sizeof(vfd->name), "%s", cedrus_video_device.name);
> > + video_set_drvdata(vfd, dev);
> > +
> > + v4l2_info(>v4l2_dev,
> > + "Device registered as /dev/video%d\n", vfd->num);
> > +
> > + dev->m2m_dev = v4l2_m2m_init(_m2m_ops);
>
> This ^^^ initialization...
>
> > + if (IS_ERR(dev->m2m_dev)) {
> > + v4l2_err(>v4l2_dev,
> > +"Failed to initialize V4L2 M2M device\n");
> > + ret = PTR_ERR(dev->m2m_dev);
> > +
> > + goto err_video;
> > + }
> > +
> > + dev->mdev.dev = >dev;
> > + strlcpy(dev->mdev.model, CEDRUS_NAME, sizeof(dev->mdev.model));
> > +
> > + media_device_init(>mdev);
> > + dev->mdev.ops = _m2m_media_ops;
> > + dev->v4l2_dev.mdev = >mdev;
>
> ...and this ^^^ initialization should be done before you start registering
> devices.
>
> > +
> > + ret = v4l2_m2m_register_media_controller(dev->m2m_dev,
> > + vfd, MEDIA_ENT_F_PROC_VIDEO_DECODER);
> > + if (ret) {
> > + v4l2_err(>v4l2_dev,
> > +"Failed to initialize V4L2 M2M media controller\n");
> > + goto err_m2m;
> > + }
>
> ^^^ this one too.
>
> If you don't do that, then you are registering partially initialized devices,
> which is never a good idea.
>
> Just move the video_register_device() call to here, just before the
> media_device_register().
>
> This is worthy of a v9, since this can cause real problems.
Thanks for pointing this out, will fix in the next revision.
[...]
> > +static const u8 intra_quantization_matrix_default[64] = {
> > + 8, 16, 16, 19, 16, 19, 22, 22,
> > + 22, 22, 22, 22, 26, 24, 26, 27,
> > + 27, 27, 26, 26, 26, 26, 27, 27,
> > + 27, 29, 29, 29, 34, 34, 34, 29,
> > + 29, 29, 27, 27, 29, 29, 32, 32,
> > + 34, 34, 37, 38, 37, 35, 35, 34,
> > + 35, 38, 38, 40, 40, 40, 48, 48,
> > + 46, 46, 56, 56, 58, 69, 69, 83
> > +};
> > +
> > +static const u8 non_intra_quantization_matrix_default[64] = {
> > + 16, 16, 16, 16, 16, 16, 16, 16,
> > + 16, 16, 16,
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
Hi,
Le mardi 28 août 2018 à 22:08 -0300, Ezequiel Garcia a écrit :
> On Tue, 2018-08-28 at 09:34 +0200, Paul Kocialkowski wrote:
> > +static const struct v4l2_m2m_ops cedrus_m2m_ops = {
> > + .device_run = cedrus_device_run,
> > + .job_abort = cedrus_job_abort,
> > +};
> > +
>
> I think you can get rid of this .job_abort. It should
> simplify your .device_run quite a bit.
>
> .job_abort is optional now since
> 5525b8314389a0c558d15464e86f438974b94e32.
Alright, I will probably do that for the next revision, since it seems
that there is no particular downside to removing it.
Thanks,
Paul
> Regards,
> Ezequiel
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On 08/28/2018 09:34 AM, Paul Kocialkowski wrote: > This introduces the Cedrus VPU driver that supports the VPU found in > Allwinner SoCs, also known as Video Engine. It is implemented through > a v4l2 m2m decoder device and a media device (used for media requests). > So far, it only supports MPEG2 decoding. > > Since this VPU is stateless, synchronization with media requests is > required in order to ensure consistency between frame headers that > contain metadata about the frame to process and the raw slice data that > is used to generate the frame. > > This driver was made possible thanks to the long-standing effort > carried out by the linux-sunxi community in the interest of reverse > engineering, documenting and implementing support for Allwinner VPU. > > Signed-off-by: Paul Kocialkowski > --- > MAINTAINERS | 7 + > drivers/staging/media/Kconfig | 2 + > drivers/staging/media/Makefile| 1 + > drivers/staging/media/sunxi/Kconfig | 15 + > drivers/staging/media/sunxi/Makefile | 1 + > drivers/staging/media/sunxi/cedrus/Kconfig| 14 + > drivers/staging/media/sunxi/cedrus/Makefile | 3 + > drivers/staging/media/sunxi/cedrus/cedrus.c | 420 + > drivers/staging/media/sunxi/cedrus/cedrus.h | 167 + > .../staging/media/sunxi/cedrus/cedrus_dec.c | 116 > .../staging/media/sunxi/cedrus/cedrus_dec.h | 28 + > .../staging/media/sunxi/cedrus/cedrus_hw.c| 322 ++ > .../staging/media/sunxi/cedrus/cedrus_hw.h| 30 + > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 235 +++ > .../staging/media/sunxi/cedrus/cedrus_regs.h | 233 +++ > .../staging/media/sunxi/cedrus/cedrus_video.c | 574 ++ > .../staging/media/sunxi/cedrus/cedrus_video.h | 32 + > 17 files changed, 2200 insertions(+) > create mode 100644 drivers/staging/media/sunxi/Kconfig > create mode 100644 drivers/staging/media/sunxi/Makefile > create mode 100644 drivers/staging/media/sunxi/cedrus/Kconfig > create mode 100644 drivers/staging/media/sunxi/cedrus/Makefile > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_regs.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 435e6c08c694..08065d53c69d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -656,6 +656,13 @@ L: linux-cry...@vger.kernel.org > S: Maintained > F: drivers/crypto/sunxi-ss/ > > +ALLWINNER VPU DRIVER > +M: Maxime Ripard > +M: Paul Kocialkowski > +L: linux-me...@vger.kernel.org > +S: Maintained > +F: drivers/staging/media/sunxi/cedrus/ > + > ALPHA PORT > M: Richard Henderson > M: Ivan Kokshaysky > diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig > index db5cf67047ad..b3620a8f2d9f 100644 > --- a/drivers/staging/media/Kconfig > +++ b/drivers/staging/media/Kconfig > @@ -31,6 +31,8 @@ source "drivers/staging/media/mt9t031/Kconfig" > > source "drivers/staging/media/omap4iss/Kconfig" > > +source "drivers/staging/media/sunxi/Kconfig" > + > source "drivers/staging/media/tegra-vde/Kconfig" > > source "drivers/staging/media/zoran/Kconfig" > diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile > index 503fbe47fa58..42948f805548 100644 > --- a/drivers/staging/media/Makefile > +++ b/drivers/staging/media/Makefile > @@ -5,5 +5,6 @@ obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074/ > obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031/ > obj-$(CONFIG_VIDEO_DM365_VPFE) += davinci_vpfe/ > obj-$(CONFIG_VIDEO_OMAP4)+= omap4iss/ > +obj-$(CONFIG_VIDEO_SUNXI)+= sunxi/ > obj-$(CONFIG_TEGRA_VDE) += tegra-vde/ > obj-$(CONFIG_VIDEO_ZORAN)+= zoran/ > diff --git a/drivers/staging/media/sunxi/Kconfig > b/drivers/staging/media/sunxi/Kconfig > new file mode 100644 > index ..c78d92240ceb > --- /dev/null > +++ b/drivers/staging/media/sunxi/Kconfig > @@ -0,0 +1,15 @@ > +config VIDEO_SUNXI > + bool "Allwinner sunXi family Video Devices" > + depends on ARCH_SUNXI || COMPILE_TEST > + help > + If you have an Allwinner SoC based on the sunXi family, say Y. > + > + Note that this option doesn't include new drivers in the > + kernel: saying N will just cause Kconfig to skip all the > + questions about Allwinner media devices. > + > +if VIDEO_SUNXI > + >
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On Tue, 2018-08-28 at 09:34 +0200, Paul Kocialkowski wrote: > This introduces the Cedrus VPU driver that supports the VPU found in > Allwinner SoCs, also known as Video Engine. It is implemented through > a v4l2 m2m decoder device and a media device (used for media requests). > So far, it only supports MPEG2 decoding. > > Since this VPU is stateless, synchronization with media requests is > required in order to ensure consistency between frame headers that > contain metadata about the frame to process and the raw slice data that > is used to generate the frame. > > This driver was made possible thanks to the long-standing effort > carried out by the linux-sunxi community in the interest of reverse > engineering, documenting and implementing support for Allwinner VPU. > > Signed-off-by: Paul Kocialkowski > --- > MAINTAINERS | 7 + > drivers/staging/media/Kconfig | 2 + > drivers/staging/media/Makefile| 1 + > drivers/staging/media/sunxi/Kconfig | 15 + > drivers/staging/media/sunxi/Makefile | 1 + > drivers/staging/media/sunxi/cedrus/Kconfig| 14 + > drivers/staging/media/sunxi/cedrus/Makefile | 3 + > drivers/staging/media/sunxi/cedrus/cedrus.c | 420 + > drivers/staging/media/sunxi/cedrus/cedrus.h | 167 + > .../staging/media/sunxi/cedrus/cedrus_dec.c | 116 > .../staging/media/sunxi/cedrus/cedrus_dec.h | 28 + > .../staging/media/sunxi/cedrus/cedrus_hw.c| 322 ++ > .../staging/media/sunxi/cedrus/cedrus_hw.h| 30 + > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 235 +++ > .../staging/media/sunxi/cedrus/cedrus_regs.h | 233 +++ > .../staging/media/sunxi/cedrus/cedrus_video.c | 574 ++ > .../staging/media/sunxi/cedrus/cedrus_video.h | 32 + > 17 files changed, 2200 insertions(+) > create mode 100644 drivers/staging/media/sunxi/Kconfig > create mode 100644 drivers/staging/media/sunxi/Makefile > create mode 100644 drivers/staging/media/sunxi/cedrus/Kconfig > create mode 100644 drivers/staging/media/sunxi/cedrus/Makefile > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_regs.h > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.c > create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 435e6c08c694..08065d53c69d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -656,6 +656,13 @@ L: linux-cry...@vger.kernel.org > S: Maintained > F: drivers/crypto/sunxi-ss/ > > +ALLWINNER VPU DRIVER > +M: Maxime Ripard > +M: Paul Kocialkowski > +L: linux-me...@vger.kernel.org > +S: Maintained > +F: drivers/staging/media/sunxi/cedrus/ > + > ALPHA PORT > M: Richard Henderson > M: Ivan Kokshaysky > diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig > index db5cf67047ad..b3620a8f2d9f 100644 > --- a/drivers/staging/media/Kconfig > +++ b/drivers/staging/media/Kconfig > @@ -31,6 +31,8 @@ source "drivers/staging/media/mt9t031/Kconfig" > > source "drivers/staging/media/omap4iss/Kconfig" > > +source "drivers/staging/media/sunxi/Kconfig" > + > source "drivers/staging/media/tegra-vde/Kconfig" > > source "drivers/staging/media/zoran/Kconfig" > diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile > index 503fbe47fa58..42948f805548 100644 > --- a/drivers/staging/media/Makefile > +++ b/drivers/staging/media/Makefile > @@ -5,5 +5,6 @@ obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074/ > obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031/ > obj-$(CONFIG_VIDEO_DM365_VPFE) += davinci_vpfe/ > obj-$(CONFIG_VIDEO_OMAP4)+= omap4iss/ > +obj-$(CONFIG_VIDEO_SUNXI)+= sunxi/ > obj-$(CONFIG_TEGRA_VDE) += tegra-vde/ > obj-$(CONFIG_VIDEO_ZORAN)+= zoran/ > diff --git a/drivers/staging/media/sunxi/Kconfig > b/drivers/staging/media/sunxi/Kconfig > new file mode 100644 > index ..c78d92240ceb > --- /dev/null > +++ b/drivers/staging/media/sunxi/Kconfig > @@ -0,0 +1,15 @@ > +config VIDEO_SUNXI > + bool "Allwinner sunXi family Video Devices" > + depends on ARCH_SUNXI || COMPILE_TEST > + help > + If you have an Allwinner SoC based on the sunXi family, say Y. > + > + Note that this option doesn't include new drivers in the > + kernel: saying N will just cause Kconfig to skip all the > + questions about Allwinner media devices. > + > +if
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On Tue, Aug 28, 2018 at 09:34:20AM +0200, Paul Kocialkowski wrote: > This introduces the Cedrus VPU driver that supports the VPU found in > Allwinner SoCs, also known as Video Engine. It is implemented through > a v4l2 m2m decoder device and a media device (used for media requests). > So far, it only supports MPEG2 decoding. > > Since this VPU is stateless, synchronization with media requests is > required in order to ensure consistency between frame headers that > contain metadata about the frame to process and the raw slice data that > is used to generate the frame. > > This driver was made possible thanks to the long-standing effort > carried out by the linux-sunxi community in the interest of reverse > engineering, documenting and implementing support for Allwinner VPU. > > Signed-off-by: Paul Kocialkowski Acked-by: Maxime Ripard Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
