Hello,
On 07.03.2018 19:37, Paul Kocialkowski wrote:
> Hi,
>
> First off, I'd like to take the occasion to say thank-you for your work.
> This is a major piece of plumbing that is required for me to add support
> for the Allwinner CedarX VPU hardware in upstream Linux. Other drivers,
> such as tegra-vde (that was recently merged in staging) are also badly
> in need of this API.
Certainly it would be good to have a common UAPI. Yet I haven't got my hands on
trying to implement the V4L interface for the tegra-vde driver, but I've taken a
look at Cedrus driver and for now I've one question:
Would it be possible (or maybe already is) to have a single IOCTL that takes
input/output buffers with codec parameters, processes the request(s) and returns
to userspace when everything is done? Having 5 context switches for a single
frame decode (like Cedrus VAAPI driver does) looks like a bit of overhead.
> I have a few comments based on my experience integrating this request
> API with the Cedrus VPU driver (and the associated libva backend), that
> also concern the vim2m driver.
>
> On Tue, 2018-02-20 at 13:44 +0900, Alexandre Courbot wrote:
>> Set the necessary ops for supporting requests in vim2m.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
>> ---
>> drivers/media/platform/Kconfig | 1 +
>> drivers/media/platform/vim2m.c | 75
>> ++++++++++++++++++++++++++++++++++
>> 2 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/media/platform/Kconfig
>> b/drivers/media/platform/Kconfig
>> index 614fbef08ddc..09be0b5f9afe 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>
> [...]
>
>> +static int vim2m_request_submit(struct media_request *req,
>> + struct media_request_entity_data
>> *_data)
>> +{
>> + struct v4l2_request_entity_data *data;
>> +
>> + data = to_v4l2_entity_data(_data);
>
> We need to call v4l2_m2m_try_schedule here so that m2m scheduling can
> happen when only 2 buffers were queued and no other action was taken
> from usespace. In that scenario, m2m scheduling currently doesn't
> happen.
>
> However, this requires access to the m2m context, which is not easy to
> get from req or _data. I'm not sure that some container_of magic would
> even do the trick here.
>
>> + return vb2_request_submit(data);
>
> vb2_request_submit does not lock the associated request mutex although
> it accesses the associated queued buffers list, which I believe this
> mutex is supposed to protect.
>
> We could either wrap this call with media_request_lock(req) and
> media_request_unlock(req) or have the lock in the function itself, which
> would require passing it the req pointer.
>
> The latter would probably be safer for future use of the function.
>
>> +}
>> +
>> +static const struct media_request_entity_ops vim2m_request_entity_ops
>> = {
>> + .data_alloc = vim2m_entity_data_alloc,
>> + .data_free = v4l2_request_entity_data_free,
>> + .submit = vim2m_request_submit,
>> +};
>> +
>> /*
>> * File operations
>> */
>> @@ -900,6 +967,9 @@ static int vim2m_open(struct file *file)
>> ctx->dev = dev;
>> hdl = &ctx->hdl;
>> v4l2_ctrl_handler_init(hdl, 4);
>> + v4l2_request_entity_init(&ctx->req_entity,
>> &vim2m_request_entity_ops,
>> + &ctx->dev->vfd);
>> + ctx->fh.entity = &ctx->req_entity.base;
>> v4l2_ctrl_new_std(hdl, &vim2m_ctrl_ops, V4L2_CID_HFLIP, 0, 1,
>> 1, 0);
>> v4l2_ctrl_new_std(hdl, &vim2m_ctrl_ops, V4L2_CID_VFLIP, 0, 1,
>> 1, 0);
>> v4l2_ctrl_new_custom(hdl, &vim2m_ctrl_trans_time_msec, NULL);
>> @@ -999,6 +1069,9 @@ static int vim2m_probe(struct platform_device
>> *pdev)
>> if (!dev)
>> return -ENOMEM;
>>
>> + v4l2_request_mgr_init(&dev->req_mgr, &dev->vfd,
>> + &v4l2_request_ops);
>> +
>> spin_lock_init(&dev->irqlock);
>>
>> ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>> @@ -1012,6 +1085,7 @@ static int vim2m_probe(struct platform_device
>> *pdev)
>> vfd = &dev->vfd;
>> vfd->lock = &dev->dev_mutex;
>> vfd->v4l2_dev = &dev->v4l2_dev;
>> + vfd->req_mgr = &dev->req_mgr.base;
>>
>> ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
>> if (ret) {
>> @@ -1054,6 +1128,7 @@ static int vim2m_remove(struct platform_device
>> *pdev)
>> del_timer_sync(&dev->timer);
>> video_unregister_device(&dev->vfd);
>> v4l2_device_unregister(&dev->v4l2_dev);
>> + v4l2_request_mgr_free(&dev->req_mgr);
>>
>> return 0;
>> }
>
--
Dmitry
