On Thu, May 03, 2018 at 10:21:32AM +0200, Hans Verkuil wrote:
> On 03/05/18 00:24, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Tue, May 01, 2018 at 11:00:25AM +0200, Hans Verkuil wrote:
...
> >> +
> >> +static int media_request_close(struct inode *inode, struct file *filp)
> >> +{
> >> + struct media_request *req = filp->private_data;
> >> +
> >> + media_request_put(req);
> >
> > A newline would be nice here.
>
> Why? I do not see it adding anything of value. Some of your other 'newline'
> comments make it indeed more readable, but I don't see any benefit from it
> here.
I'd think it's nice to have an empty line before return if it's detached
from everything else.
>
> >
> >> + return 0;
> >> +}
> >> +
> >> +static unsigned int media_request_poll(struct file *filp,
> >> + struct poll_table_struct *wait)
> >> +{
> >> + struct media_request *req = filp->private_data;
> >> + unsigned long flags;
> >> + unsigned int ret = 0;
> >> + enum media_request_state state;
> >> +
> >> + if (!(poll_requested_events(wait) & POLLPRI))
> >> + return 0;
> >> +
> >> + spin_lock_irqsave(&req->lock, flags);
> >> + state = atomic_read(&req->state);
> >> +
> >> + if (state == MEDIA_REQUEST_STATE_COMPLETE) {
> >> + ret = POLLPRI;
> >> + goto unlock;
> >> + }
> >> + if (state == MEDIA_REQUEST_STATE_IDLE) {
> >
> > Should this be just anything else than QUEUE or VALIDATING? You're missing
> > CLEANING here, for instance.
>
> I changed it to != QUEUED. I don't think it is a good idea to include VALIDATING
> in this. It only makes sense to poll for the result if the request is fully queued.
Sounds good.
...
> >> + /*
> >> + * If the req_validate was successful, then we mark the state as QUEUED
> >> + * and call req_queue. The reason we set the state first is that this
> >> + * allows req_queue to unbind or complete the queued objects in case
> >> + * they are immediately 'consumed'. State changes from QUEUED to another
> >> + * state can only happen if either the driver changes the state or if
> >> + * the user cancels the vb2 queue. The driver can only change the state
> >> + * after each object is queued through the req_queue op (and note that
> >> + * that op cannot fail), so setting the state to QUEUED up front is
> >> + * safe.
> >> + *
> >> + * The other reason for changing the state is if the vb2 queue is
> >> + * canceled, and that uses the req_queue_mutex which is still locked
> >> + * while req_queue is called, so that's safe as well.
> >> + */
> >> + atomic_set(&req->state,
> >> + ret ? MEDIA_REQUEST_STATE_IDLE : MEDIA_REQUEST_STATE_QUEUED);
> >> +
> >> + if (!ret)
> >> + mdev->ops->req_queue(req);
> >> +
> >> + mutex_unlock(&mdev->req_queue_mutex);
> >> +
> >> + if (ret) {
> >> + dev_dbg(mdev->dev, "request: can't queue %s (%d)\n",
> >> + req->debug_str, ret);
> >> + } else {
> >> + media_request_get(req);
> >
> > You'll need to get a reference before you queue the request. Otherwise it's
> > possible that it completes before you get here, and you end up accessing
> > released memory. The request refcount might be also under 0 before that
> > though.
>
> No, that can't happen. This function implements the QUEUE ioctl, which can only
> be called by userspace through the request fd, so there is always at least one
> reference open.
Indeed, you're right.
The curly braces are unnecessary above btw.
...
> >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> >> index bcc6ec434f1f..ae846208be51 100644
> >> --- a/include/media/media-device.h
> >> +++ b/include/media/media-device.h
> >> @@ -27,6 +27,7 @@
> >>
> >> struct ida;
> >> struct device;
> >> +struct media_device;
> >>
> >> /**
> >> * struct media_entity_notify - Media Entity Notify
> >> @@ -50,10 +51,22 @@ struct media_entity_notify {
> >> * struct media_device_ops - Media device operations
> >> * @link_notify: Link state change notification callback. This callback is
> >> * called with the graph_mutex held.
> >> + * @req_alloc: Allocate a request
> >> + * @req_free: Free a request
> >> + * @req_validate: Validate a request, but do not queue yet
> >> + * @req_queue: Queue a validated request, cannot fail. If something goes
> >> + * wrong when queueing this request then it should be marked
> >> + * as such internally in the driver and any related buffers
> >> + * will eventually return to userspace with V4L2_BUF_FLAG_ERROR
> >> + * set.
> >
> > Note that this is MC; I think the reference is fine but I'd make it
> > explicit this is V4L2, and possibly that it's an example.
>
> I've changed the last line to:
>
> "must eventually return to vb2 with state VB2_BUF_STATE_ERROR."
>
> Since this is really a vb2 thing, not V4L2.
>
> It's not an example, based on what I know this should really be done like
> this. If reality proves to be different, then the comment will need to change.
Yes, but this is not a place for VB2 documentation nor a conclusive
document of what a failing queue operation would mean on whichever possible
other non-MC subsystem. So the VB2 related matter here remains an example
only.
We don't seem to have other than V4L2 using MC after more than five years
but I'd still be wary of thinking this is bound to V4L2 only.
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
