Re: [RFCv4,19/21] media: vim2m: add request support

[Paul Kocialkowski]

Hi,

On Thu, 2018-03-08 at 22:48 +0900, Alexandre Courbot wrote:
> Hi Paul!
> 
> Thanks a lot for taking the time to try this! I am also working on
> getting it to work with an actual driver, but you apparently found
> rough edges that I missed.
> 
> On Thu, Mar 8, 2018 at 1:37 AM, Paul Kocialkowski
> <paul.kocialkowski@xxxxxxxxxxx> 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.
> > 
> > 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.
> 
> I don't think I understand the sequence of events that results in
> v4l2_m2m_try_schedule() not being called. Do you mean something like:
> 
> *
> * QBUF on output queue with request set
> * QBUF on capture queue
> * SUBMIT_REQUEST
> 
> ?
> 
> The call to vb2_request_submit() right after should trigger
> v4l2_m2m_try_schedule(), since the buffers associated to the request
> will enter the vb2 queue and be passed to the m2m framework, which
> will then call v4l2_m2m_try_schedule(). Or maybe you are thinking
> about a different sequence of events?

This is indeed the sequence of events that I'm seeing, but the
scheduling call simply did not happen on vb2_request_submit. I suppose I will need to investigate some more to find out exactly why.

IIRC, the m2m qbuf function is called (and fails to schedule) when the
ioctl happens, not when the task is submitted.

This issue is seen with vim2m as well as the rencently-submitted sunxi-
cedrus driver (with the in-driver calls to v4l2_m2m_try_schedule
removed, obviously). If needs be, I could provide a standalone test
program to reproduce it.

> > 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.
> 
> data_->entity will give you a pointer to the media_request_entity,
> which is part of vim2m_ctx. You can thus get the m2m context by doing
> container_of(data_->entity, struct vim2m_ctx, req_entity). See
> vim2m_entity_data_alloc() for an example.

Excellent, that's exactly what I was looking for! Thanks a lot and see
the related patch I submitted earlier today.

> > > +     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.
> 
> After a request is submitted, the data protected by the mutex can only
> be accessed by the driver when it processes the request. It cannot be
> modified concurrently, so I think we are safe here.

Fait enough, that should be enough then. I've dropped this change.

Cheers,

Paul

> I am also wondering whether the ioctl locking doesn't make the request
> locking redundant. Request information can only be modified and
> accessed through ioctls until it is submitted, and after that there
> are no concurrent accesses. I need to think a bit more about it
> though.
> 
> Cheers,
> Alex.
> 
> > 
> > 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;
> > >  }
> > 
> > --
> > Paul Kocialkowski, Bootlin (formerly Free Electrons)
> > Embedded Linux and kernel engineering
> > https://bootlin.com
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: This is a digitally signed message part