[PATCH v2 0/4] media: uvcvideo: Prepare deprecation of nodrop

[PATCH v2 0/4] media: uvcvideo: Prepare deprecation of nodrop

[Ricardo Ribalda]

We intend to deprecate the nodrop parameter in the future and adopt the
default behaviour of the other media drivers: return buffers with an error
to userspace with V4L2_BUF_FLAG_ERROR set in v4l2_buffer.flags.

Make the first step in the deprecation by changing the default value of
the parameter and printing an error message when the value is changed.

While implementing this change, Hans found an error in
uvc_queue_buffer_complete(). This series also fix it.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v2:
- Improve cover letter wording.
- Add new patch to fix vb2_buffer_done()
- Link to v1: https://lore.kernel.org/r/20241217-uvc-deprecate-v1-0-57d353f68f8f@chromium.org

---
Ricardo Ribalda (4):
      media: uvcvideo: Propagate buf->error to userspace
      media: uvcvideo: Invert default value for nodrop module param
      media: uvcvideo: Allow changing noparam on the fly
      media: uvcvideo: Announce the user our deprecation intentions

 drivers/media/usb/uvc/uvc_driver.c | 23 ++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_queue.c  |  9 ++++-----
 drivers/media/usb/uvc/uvcvideo.h   |  4 +---
 3 files changed, 25 insertions(+), 11 deletions(-)
---
base-commit: d216d9cb4dd854ef0a2ec1701f403facb298af51
change-id: 20241217-uvc-deprecate-fbd6228fa1e2

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 1/4] media: uvcvideo: Propagate buf->error to userspace

[Ricardo Ribalda]

Now we return VB2_BUF_STATE_DONE for valid and invalid frames. Propagate
the correct value, so the user can know if the frame is valid or not via
struct v4l2_buffer->flags.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Closes: https://lore.kernel.org/linux-media/84b0f212-cd88-46bb-8e6f-b94ec3eccba6@redhat.com
Fixes: 6998b6fb4b1c ("[media] uvcvideo: Use videobuf2-vmalloc")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_queue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 26ee85657fc8..f8464f0aae1b 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -479,7 +479,8 @@ static void uvc_queue_buffer_complete(struct kref *ref)
 
 	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
 	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
-	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+	vb2_buffer_done(&buf->buf.vb2_buf, buf->error ? VB2_BUF_STATE_ERROR :
+							VB2_BUF_STATE_DONE);
 }
 
 /*

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 1/4] media: uvcvideo: Propagate buf->error to userspace

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Dec 18, 2024 at 09:39:08PM +0000, Ricardo Ribalda wrote:
> Now we return VB2_BUF_STATE_DONE for valid and invalid frames. Propagate
> the correct value, so the user can know if the frame is valid or not via
> struct v4l2_buffer->flags.
> 
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Closes: https://lore.kernel.org/linux-media/84b0f212-cd88-46bb-8e6f-b94ec3eccba6@redhat.com
> Fixes: 6998b6fb4b1c ("[media] uvcvideo: Use videobuf2-vmalloc")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_queue.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 26ee85657fc8..f8464f0aae1b 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -479,7 +479,8 @@ static void uvc_queue_buffer_complete(struct kref *ref)
>  
>  	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
>  	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
> -	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +	vb2_buffer_done(&buf->buf.vb2_buf, buf->error ? VB2_BUF_STATE_ERROR :
> +							VB2_BUF_STATE_DONE);
>  }
>  
>  /*

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/4] media: uvcvideo: Invert default value for nodrop module param

[Ricardo Ribalda]

The module param `nodrop` defines what to do with frames that contain an
error: drop them or sending them to userspace.

The default in the rest of the media subsystem is to return buffers with
an error to userspace with V4L2_BUF_FLAG_ERROR set in v4l2_buffer.flags.
In UVC we drop buffers with errors by default.

Change the default behaviour of uvcvideo to match the rest of the
drivers and maybe get rid of the module parameter in the future.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index b3c8411dc05c..091145743872 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -32,7 +32,7 @@
 
 unsigned int uvc_clock_param = CLOCK_MONOTONIC;
 unsigned int uvc_hw_timestamps_param;
-unsigned int uvc_no_drop_param;
+unsigned int uvc_no_drop_param = 1;
 static unsigned int uvc_quirks_param = -1;
 unsigned int uvc_dbg_param;
 unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 2/4] media: uvcvideo: Invert default value for nodrop module param

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Dec 18, 2024 at 09:39:09PM +0000, Ricardo Ribalda wrote:
> The module param `nodrop` defines what to do with frames that contain an
> error: drop them or sending them to userspace.
> 
> The default in the rest of the media subsystem is to return buffers with
> an error to userspace with V4L2_BUF_FLAG_ERROR set in v4l2_buffer.flags.
> In UVC we drop buffers with errors by default.
> 
> Change the default behaviour of uvcvideo to match the rest of the
> drivers and maybe get rid of the module parameter in the future.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index b3c8411dc05c..091145743872 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -32,7 +32,7 @@
>  
>  unsigned int uvc_clock_param = CLOCK_MONOTONIC;
>  unsigned int uvc_hw_timestamps_param;
> -unsigned int uvc_no_drop_param;
> +unsigned int uvc_no_drop_param = 1;
>  static unsigned int uvc_quirks_param = -1;
>  unsigned int uvc_dbg_param;
>  unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/4] media: uvcvideo: Allow changing noparam on the fly

[Ricardo Ribalda]

Right now the parameter value is read during video_registration and
cannot be changed afterwards, despite its permissions 0644, that makes
the user believe that the value can be written.

The parameter only affects the beviour of uvc_queue_buffer_complete(),
with only one check per buffer.

We can read the value directly from uvc_queue_buffer_complete() and
therefore allowing changing it with sysfs on the fly.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 2 +-
 drivers/media/usb/uvc/uvc_queue.c  | 6 ++----
 drivers/media/usb/uvc/uvcvideo.h   | 4 +---
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 091145743872..10812a841587 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1995,7 +1995,7 @@ int uvc_register_video_device(struct uvc_device *dev,
 	int ret;
 
 	/* Initialize the video buffers queue. */
-	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
+	ret = uvc_queue_init(queue, type);
 	if (ret)
 		return ret;
 
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index f8464f0aae1b..2ee142621042 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -208,8 +208,7 @@ static const struct vb2_ops uvc_meta_queue_qops = {
 	.stop_streaming = uvc_stop_streaming,
 };
 
-int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
-		    int drop_corrupted)
+int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
 {
 	int ret;
 
@@ -239,7 +238,6 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
 	mutex_init(&queue->mutex);
 	spin_lock_init(&queue->irqlock);
 	INIT_LIST_HEAD(&queue->irqqueue);
-	queue->flags = drop_corrupted ? UVC_QUEUE_DROP_CORRUPTED : 0;
 
 	return 0;
 }
@@ -472,7 +470,7 @@ static void uvc_queue_buffer_complete(struct kref *ref)
 	struct vb2_buffer *vb = &buf->buf.vb2_buf;
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
 
-	if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
+	if (buf->error && !uvc_no_drop_param) {
 		uvc_queue_buffer_requeue(queue, buf);
 		return;
 	}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 07f9921d83f2..ebbd8afcf136 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -316,7 +316,6 @@ struct uvc_buffer {
 };
 
 #define UVC_QUEUE_DISCONNECTED		(1 << 0)
-#define UVC_QUEUE_DROP_CORRUPTED	(1 << 1)
 
 struct uvc_video_queue {
 	struct vb2_queue queue;
@@ -674,8 +673,7 @@ extern struct uvc_driver uvc_driver;
 struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
 
 /* Video buffers queue management. */
-int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
-		   int drop_corrupted);
+int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
 void uvc_queue_release(struct uvc_video_queue *queue);
 int uvc_request_buffers(struct uvc_video_queue *queue,
 			struct v4l2_requestbuffers *rb);

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 3/4] media: uvcvideo: Allow changing noparam on the fly

[Laurent Pinchart]

On Wed, Dec 18, 2024 at 09:39:10PM +0000, Ricardo Ribalda wrote:
> Right now the parameter value is read during video_registration and
> cannot be changed afterwards, despite its permissions 0644, that makes
> the user believe that the value can be written.

Well, it can still be written, and will apply to new devices. There's
value in making it fully dynamic though.

> 
> The parameter only affects the beviour of uvc_queue_buffer_complete(),

s/beviour/behaviour/

> with only one check per buffer.
> 
> We can read the value directly from uvc_queue_buffer_complete() and
> therefore allowing changing it with sysfs on the fly.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 +-
>  drivers/media/usb/uvc/uvc_queue.c  | 6 ++----
>  drivers/media/usb/uvc/uvcvideo.h   | 4 +---
>  3 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 091145743872..10812a841587 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1995,7 +1995,7 @@ int uvc_register_video_device(struct uvc_device *dev,
>  	int ret;
>  
>  	/* Initialize the video buffers queue. */
> -	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
> +	ret = uvc_queue_init(queue, type);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index f8464f0aae1b..2ee142621042 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -208,8 +208,7 @@ static const struct vb2_ops uvc_meta_queue_qops = {
>  	.stop_streaming = uvc_stop_streaming,
>  };
>  
> -int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> -		    int drop_corrupted)
> +int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
>  {
>  	int ret;
>  
> @@ -239,7 +238,6 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
>  	mutex_init(&queue->mutex);
>  	spin_lock_init(&queue->irqlock);
>  	INIT_LIST_HEAD(&queue->irqqueue);
> -	queue->flags = drop_corrupted ? UVC_QUEUE_DROP_CORRUPTED : 0;
>  
>  	return 0;
>  }
> @@ -472,7 +470,7 @@ static void uvc_queue_buffer_complete(struct kref *ref)
>  	struct vb2_buffer *vb = &buf->buf.vb2_buf;
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
>  
> -	if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
> +	if (buf->error && !uvc_no_drop_param) {

As this is the only location where the uvc_no_drop_param variable is
read, I don't expect any race condition.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		uvc_queue_buffer_requeue(queue, buf);
>  		return;
>  	}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 07f9921d83f2..ebbd8afcf136 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -316,7 +316,6 @@ struct uvc_buffer {
>  };
>  
>  #define UVC_QUEUE_DISCONNECTED		(1 << 0)
> -#define UVC_QUEUE_DROP_CORRUPTED	(1 << 1)
>  
>  struct uvc_video_queue {
>  	struct vb2_queue queue;
> @@ -674,8 +673,7 @@ extern struct uvc_driver uvc_driver;
>  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
>  
>  /* Video buffers queue management. */
> -int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> -		   int drop_corrupted);
> +int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
>  void uvc_queue_release(struct uvc_video_queue *queue);
>  int uvc_request_buffers(struct uvc_video_queue *queue,
>  			struct v4l2_requestbuffers *rb);

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/4] media: uvcvideo: Announce the user our deprecation intentions

[Ricardo Ribalda]

If the user sets the nodrop parameter, print a deprecation warning once.
Hopefully they will come to the mailing list if it is an ABI change.

Now that we have a callback, take this chance to parse the parameter as
a boolean. We still say to userspace that it is a uint to avoid ABI
changes.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 10812a841587..d8e8675dd2cd 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2424,8 +2424,25 @@ module_param_call(clock, uvc_clock_param_set, uvc_clock_param_get,
 MODULE_PARM_DESC(clock, "Video buffers timestamp clock");
 module_param_named(hwtimestamps, uvc_hw_timestamps_param, uint, 0644);
 MODULE_PARM_DESC(hwtimestamps, "Use hardware timestamps");
-module_param_named(nodrop, uvc_no_drop_param, uint, 0644);
+
+static int param_set_nodrop(const char *val, const struct kernel_param *kp)
+{
+	pr_warn_once("uvcvideo: "
+		     DEPRECATED
+		     "nodrop parameter will be eventually removed.\n");
+	return param_set_bool(val, kp);
+}
+
+static const struct kernel_param_ops param_ops_nodrop = {
+	.set = param_set_nodrop,
+	.get = param_get_uint,
+};
+
+param_check_uint(nodrop, &uvc_no_drop_param);
+module_param_cb(nodrop, &param_ops_nodrop, &uvc_no_drop_param, 0644);
+__MODULE_PARM_TYPE(nodrop, "uint");
 MODULE_PARM_DESC(nodrop, "Don't drop incomplete frames");
+
 module_param_named(quirks, uvc_quirks_param, uint, 0644);
 MODULE_PARM_DESC(quirks, "Forced device quirks");
 module_param_named(trace, uvc_dbg_param, uint, 0644);

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 4/4] media: uvcvideo: Announce the user our deprecation intentions

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Dec 18, 2024 at 09:39:11PM +0000, Ricardo Ribalda wrote:
> If the user sets the nodrop parameter, print a deprecation warning once.
> Hopefully they will come to the mailing list if it is an ABI change.
> 
> Now that we have a callback, take this chance to parse the parameter as
> a boolean. We still say to userspace that it is a uint to avoid ABI
> changes.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 10812a841587..d8e8675dd2cd 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2424,8 +2424,25 @@ module_param_call(clock, uvc_clock_param_set, uvc_clock_param_get,
>  MODULE_PARM_DESC(clock, "Video buffers timestamp clock");
>  module_param_named(hwtimestamps, uvc_hw_timestamps_param, uint, 0644);
>  MODULE_PARM_DESC(hwtimestamps, "Use hardware timestamps");
> -module_param_named(nodrop, uvc_no_drop_param, uint, 0644);
> +
> +static int param_set_nodrop(const char *val, const struct kernel_param *kp)
> +{
> +	pr_warn_once("uvcvideo: "
> +		     DEPRECATED
> +		     "nodrop parameter will be eventually removed.\n");
> +	return param_set_bool(val, kp);
> +}
> +
> +static const struct kernel_param_ops param_ops_nodrop = {
> +	.set = param_set_nodrop,
> +	.get = param_get_uint,
> +};
> +
> +param_check_uint(nodrop, &uvc_no_drop_param);
> +module_param_cb(nodrop, &param_ops_nodrop, &uvc_no_drop_param, 0644);
> +__MODULE_PARM_TYPE(nodrop, "uint");
>  MODULE_PARM_DESC(nodrop, "Don't drop incomplete frames");
> +
>  module_param_named(quirks, uvc_quirks_param, uint, 0644);
>  MODULE_PARM_DESC(quirks, "Forced device quirks");
>  module_param_named(trace, uvc_dbg_param, uint, 0644);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/4] media: uvcvideo: Prepare deprecation of nodrop

[Hans de Goede]

Hi,

On 18-Dec-24 10:39 PM, Ricardo Ribalda wrote:
> We intend to deprecate the nodrop parameter in the future and adopt the
> default behaviour of the other media drivers: return buffers with an error
> to userspace with V4L2_BUF_FLAG_ERROR set in v4l2_buffer.flags.
> 
> Make the first step in the deprecation by changing the default value of
> the parameter and printing an error message when the value is changed.
> 
> While implementing this change, Hans found an error in
> uvc_queue_buffer_complete(). This series also fix it.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Changes in v2:
> - Improve cover letter wording.
> - Add new patch to fix vb2_buffer_done()
> - Link to v1: https://lore.kernel.org/r/20241217-uvc-deprecate-v1-0-57d353f68f8f@chromium.org

Thank you the entire v2 series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series, assuming you have tested that
the modparam magic in patch 4/4 works as expected?

Regards,

Hans




> 
> ---
> Ricardo Ribalda (4):
>       media: uvcvideo: Propagate buf->error to userspace
>       media: uvcvideo: Invert default value for nodrop module param
>       media: uvcvideo: Allow changing noparam on the fly
>       media: uvcvideo: Announce the user our deprecation intentions
> 
>  drivers/media/usb/uvc/uvc_driver.c | 23 ++++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_queue.c  |  9 ++++-----
>  drivers/media/usb/uvc/uvcvideo.h   |  4 +---
>  3 files changed, 25 insertions(+), 11 deletions(-)
> ---
> base-commit: d216d9cb4dd854ef0a2ec1701f403facb298af51
> change-id: 20241217-uvc-deprecate-fbd6228fa1e2
> 
> Best regards,

Re: [PATCH v2 0/4] media: uvcvideo: Prepare deprecation of nodrop

[Ricardo Ribalda]

Hi Hans

[Sorry for the dup email, I forgot to reply-all before]

On Wed, 18 Dec 2024 at 22:53, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 18-Dec-24 10:39 PM, Ricardo Ribalda wrote:
> > We intend to deprecate the nodrop parameter in the future and adopt the
> > default behaviour of the other media drivers: return buffers with an error
> > to userspace with V4L2_BUF_FLAG_ERROR set in v4l2_buffer.flags.
> >
> > Make the first step in the deprecation by changing the default value of
> > the parameter and printing an error message when the value is changed.
> >
> > While implementing this change, Hans found an error in
> > uvc_queue_buffer_complete(). This series also fix it.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Changes in v2:
> > - Improve cover letter wording.
> > - Add new patch to fix vb2_buffer_done()
> > - Link to v1: https://lore.kernel.org/r/20241217-uvc-deprecate-v1-0-57d353f68f8f@chromium.org
>
> Thank you the entire v2 series looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> for the series, assuming you have tested that
> the modparam magic in patch 4/4 works as expected?

Yes, I have tested v1 in real hardware using ChromeOS kernel 6.6.

looks more or less like this:
# echo 100 > nodrop
# cat nodprop
1
# echo false > nodrop
# cat nodprop
0
# echo 1 > nodrop
# cat nodprop
1

With the first echo throwing a warning.

Thanks!

>
> Regards,
>
> Hans
>
>
>
>
> >
> > ---
> > Ricardo Ribalda (4):
> >       media: uvcvideo: Propagate buf->error to userspace
> >       media: uvcvideo: Invert default value for nodrop module param
> >       media: uvcvideo: Allow changing noparam on the fly
> >       media: uvcvideo: Announce the user our deprecation intentions
> >
> >  drivers/media/usb/uvc/uvc_driver.c | 23 ++++++++++++++++++++---
> >  drivers/media/usb/uvc/uvc_queue.c  |  9 ++++-----
> >  drivers/media/usb/uvc/uvcvideo.h   |  4 +---
> >  3 files changed, 25 insertions(+), 11 deletions(-)
> > ---
> > base-commit: d216d9cb4dd854ef0a2ec1701f403facb298af51
> > change-id: 20241217-uvc-deprecate-fbd6228fa1e2
> >
> > Best regards,
>


-- 
Ricardo Ribalda