Re: [RFCv9 PATCH 05/29] media-request: add request ioctls

[Sakari Ailus]

Hi Hans,

I think it'd be easier to review the code if it was a part of the previous
patch. It's so closely related to what's being done there.

On Wed, Mar 28, 2018 at 03:50:06PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Implement the MEDIA_REQUEST_IOC_QUEUE and MEDIA_REQUEST_IOC_REINIT
> ioctls.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/media-request.c | 80 
> +--
>  1 file changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index 8135d9d32af9..3ee3b27fd644 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -105,10 +105,86 @@ static unsigned int media_request_poll(struct file 
> *filp,
>   return 0;
>  }
>  
> +static long media_request_ioctl_queue(struct media_request *req)
> +{
> + struct media_device *mdev = req->mdev;
> + unsigned long flags;
> + int ret = 0;
> +
> + dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> +
> + spin_lock_irqsave(>lock, flags);
> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> + dev_dbg(mdev->dev,
> + "request: unable to queue %s, request in state %s\n",
> + req->debug_str, media_request_state_str(req->state));

You could print the message after releasing the spinlock. Maybe store the
request state locally first?

> + spin_unlock_irqrestore(>lock, flags);
> + return -EINVAL;
> + }
> + req->state = MEDIA_REQUEST_STATE_QUEUEING;
> +
> + spin_unlock_irqrestore(>lock, flags);
> +
> + /*
> +  * Ensure the request that is validated will be the one that gets queued
> +  * next by serialising the queueing process.
> +  */
> + mutex_lock(>req_queue_mutex);
> +
> + ret = mdev->ops->req_queue(req);
> + spin_lock_irqsave(>lock, flags);
> + req->state = ret ? MEDIA_REQUEST_STATE_IDLE : 
> MEDIA_REQUEST_STATE_QUEUED;
> + spin_unlock_irqrestore(>lock, flags);
> + mutex_unlock(>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);
> + }
> +
> + return ret;
> +}
> +
> +static long media_request_ioctl_reinit(struct media_request *req)
> +{
> + struct media_device *mdev = req->mdev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> + if (req->state != MEDIA_REQUEST_STATE_IDLE &&
> + req->state != MEDIA_REQUEST_STATE_COMPLETE) {
> + dev_dbg(mdev->dev,
> + "request: %s not in idle or complete state, cannot 
> reinit\n",
> + req->debug_str);
> + spin_unlock_irqrestore(>lock, flags);
> + return -EINVAL;
> + }
> + req->state = MEDIA_REQUEST_STATE_CLEANING;
> + spin_unlock_irqrestore(>lock, flags);
> +
> + media_request_clean(req);
> +
> + spin_lock_irqsave(>lock, flags);
> + req->state = MEDIA_REQUEST_STATE_IDLE;
> + spin_unlock_irqrestore(>lock, flags);

Newline?

> + return 0;
> +}
> +
>  static long media_request_ioctl(struct file *filp, unsigned int cmd,
> - unsigned long __arg)
> + unsigned long arg)

This belongs to the patch adding media_request_ioctl().

>  {
> - return -ENOIOCTLCMD;
> + struct media_request *req = filp->private_data;
> +
> + switch (cmd) {
> + case MEDIA_REQUEST_IOC_QUEUE:
> + return media_request_ioctl_queue(req);
> + case MEDIA_REQUEST_IOC_REINIT:
> + return media_request_ioctl_reinit(req);
> + default:
> + return -ENOIOCTLCMD;
> + }
>  }
>  
>  static const struct file_operations request_fops = {

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

[RFCv9 PATCH 05/29] media-request: add request ioctls

[Hans Verkuil]

From: Hans Verkuil 

Implement the MEDIA_REQUEST_IOC_QUEUE and MEDIA_REQUEST_IOC_REINIT
ioctls.

Signed-off-by: Hans Verkuil 
---
 drivers/media/media-request.c | 80 +--
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index 8135d9d32af9..3ee3b27fd644 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -105,10 +105,86 @@ static unsigned int media_request_poll(struct file *filp,
return 0;
 }
 
+static long media_request_ioctl_queue(struct media_request *req)
+{
+   struct media_device *mdev = req->mdev;
+   unsigned long flags;
+   int ret = 0;
+
+   dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
+
+   spin_lock_irqsave(>lock, flags);
+   if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+   dev_dbg(mdev->dev,
+   "request: unable to queue %s, request in state %s\n",
+   req->debug_str, media_request_state_str(req->state));
+   spin_unlock_irqrestore(>lock, flags);
+   return -EINVAL;
+   }
+   req->state = MEDIA_REQUEST_STATE_QUEUEING;
+
+   spin_unlock_irqrestore(>lock, flags);
+
+   /*
+* Ensure the request that is validated will be the one that gets queued
+* next by serialising the queueing process.
+*/
+   mutex_lock(>req_queue_mutex);
+
+   ret = mdev->ops->req_queue(req);
+   spin_lock_irqsave(>lock, flags);
+   req->state = ret ? MEDIA_REQUEST_STATE_IDLE : 
MEDIA_REQUEST_STATE_QUEUED;
+   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>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);
+   }
+
+   return ret;
+}
+
+static long media_request_ioctl_reinit(struct media_request *req)
+{
+   struct media_device *mdev = req->mdev;
+   unsigned long flags;
+
+   spin_lock_irqsave(>lock, flags);
+   if (req->state != MEDIA_REQUEST_STATE_IDLE &&
+   req->state != MEDIA_REQUEST_STATE_COMPLETE) {
+   dev_dbg(mdev->dev,
+   "request: %s not in idle or complete state, cannot 
reinit\n",
+   req->debug_str);
+   spin_unlock_irqrestore(>lock, flags);
+   return -EINVAL;
+   }
+   req->state = MEDIA_REQUEST_STATE_CLEANING;
+   spin_unlock_irqrestore(>lock, flags);
+
+   media_request_clean(req);
+
+   spin_lock_irqsave(>lock, flags);
+   req->state = MEDIA_REQUEST_STATE_IDLE;
+   spin_unlock_irqrestore(>lock, flags);
+   return 0;
+}
+
 static long media_request_ioctl(struct file *filp, unsigned int cmd,
-   unsigned long __arg)
+   unsigned long arg)
 {
-   return -ENOIOCTLCMD;
+   struct media_request *req = filp->private_data;
+
+   switch (cmd) {
+   case MEDIA_REQUEST_IOC_QUEUE:
+   return media_request_ioctl_queue(req);
+   case MEDIA_REQUEST_IOC_REINIT:
+   return media_request_ioctl_reinit(req);
+   default:
+   return -ENOIOCTLCMD;
+   }
 }
 
 static const struct file_operations request_fops = {
-- 
2.16.1