Re: [RFC v4 04/17] WIP: [media] v4l2: add v4l2_event_queue_fh_with_cb()

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 10/20/2017 11:49 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
> 
> For some type of events we may require the event user in the
> kernel to run some operation when DQ_EVENT() is called.
> V4L2_EVENT_OUT_FENCE is the first user of this mechanism as it needs
> to call v4l2 core back to install a file descriptor.
> 
> This is WIP, I believe we are able to come up with better ways to do it,
> but as that is not the main part of the patchset I'll keep it like
> this for now.

I'm OK with this callback. I don't off-hand see a better way of doing this.

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-event.c | 31 +++++++++++++++++++++++++++----
>  include/media/v4l2-event.h           |  7 +++++++
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index 313ee9d1f9ee..6274e3e174e0 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -58,6 +58,9 @@ static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>  
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  
> +	if (kev->prepare)
> +		kev->prepare(kev->data);
> +
>  	return 0;
>  }
>  
> @@ -104,8 +107,11 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(
>  	return NULL;
>  }
>  
> -static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev,
> -		const struct timespec *ts)
> +static void __v4l2_event_queue_fh_with_cb(struct v4l2_fh *fh,

I wouldn't rename this function, just add the two arguments.

> +					  const struct v4l2_event *ev,
> +					  const struct timespec *ts,
> +					  void (*prepare)(void *data),
> +					  void *data)
>  {
>  	struct v4l2_subscribed_event *sev;
>  	struct v4l2_kevent *kev;
> @@ -155,6 +161,8 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
>  	kev->event.id = ev->id;
>  	kev->event.timestamp = *ts;
>  	kev->event.sequence = fh->sequence;
> +	kev->prepare = prepare;
> +	kev->data = data;
>  	sev->in_use++;
>  	list_add_tail(&kev->list, &fh->available);
>  
> @@ -177,7 +185,7 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
>  	spin_lock_irqsave(&vdev->fh_lock, flags);
>  
>  	list_for_each_entry(fh, &vdev->fh_list, list)
> -		__v4l2_event_queue_fh(fh, ev, &timestamp);
> +		__v4l2_event_queue_fh_with_cb(fh, ev, &timestamp, NULL, NULL);
>  
>  	spin_unlock_irqrestore(&vdev->fh_lock, flags);
>  }
> @@ -191,11 +199,26 @@ void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev)
>  	ktime_get_ts(&timestamp);
>  
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> -	__v4l2_event_queue_fh(fh, ev, &timestamp);
> +	__v4l2_event_queue_fh_with_cb(fh, ev, &timestamp, NULL, NULL);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_queue_fh);

I'd drop this function and replace it with a static inline in the v4l2-event.h
header that calls v4l2_event_queue_fh_with_cb.

>  
> +void v4l2_event_queue_fh_with_cb(struct v4l2_fh *fh,
> +				 const struct v4l2_event *ev,
> +				 void (*prepare)(void *data), void *data)
> +{
> +	unsigned long flags;
> +	struct timespec timestamp;
> +
> +	ktime_get_ts(&timestamp);
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	__v4l2_event_queue_fh_with_cb(fh, ev, &timestamp, prepare, data);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue_fh_with_cb);
> +
>  int v4l2_event_pending(struct v4l2_fh *fh)
>  {
>  	return fh->navailable;
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 2b794f2ad824..dc770257811e 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -68,11 +68,14 @@ struct video_device;
>   * @list:	List node for the v4l2_fh->available list.
>   * @sev:	Pointer to parent v4l2_subscribed_event.
>   * @event:	The event itself.
> + * @prepare:	Callback to prepare the event only at DQ_EVENT() time.

@data is not documented.

>   */
>  struct v4l2_kevent {
>  	struct list_head	list;
>  	struct v4l2_subscribed_event *sev;
>  	struct v4l2_event	event;
> +	void (*prepare)(void *data);
> +	void *data;
>  };
>  
>  /**
> @@ -160,6 +163,10 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
>   */
>  void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
>  
> +void v4l2_event_queue_fh_with_cb(struct v4l2_fh *fh,
> +				 const struct v4l2_event *ev,
> +				 void (*prepare)(void *data), void *data);
> +

Missing kerneldoc documentation.

>  /**
>   * v4l2_event_pending - Check if an event is available
>   *
> 

Regards,

	Hans