Message ID | 20241112112926.17848-1-isaac.scott@ideasonboard.com |
---|---|
Headers | show |
Series | Fix Sonix Technology MJPEG streams | expand |
Hi Isaac I am curious... How is this v7? Remember to run scripts/checkpatch.pl --strict -g HEAD It is complaining about an open parenthesis match here. Not the end of the world, but rather fix it locally than discussing :) Totally optional... maybe you can add to the cover-letter a run of yavta -c /dev/videoX with and without the patch. So people can see practically what changes. Regards! On Tue, 12 Nov 2024 at 12:33, Isaac Scott <isaac.scott@ideasonboard.com> wrote: > > Some cameras, such as the Sonix Technology Co. 292A, exhibit issues when > running two parallel streams, causing USB packets to be dropped when an > H.264 stream posts a keyframe while an MJPEG stream is running > simultaneously. This occasionally causes the driver to erroneously > output two consecutive JPEG images as a single frame. > > To fix this, we inspect the buffer, and trigger a new frame when we > find an SOI. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_video.c | 27 ++++++++++++++++++++++++++- > drivers/media/usb/uvc/uvcvideo.h | 1 + > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index e00f38dd07d9..9bb41362c48d 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -20,6 +20,7 @@ > #include <linux/atomic.h> > #include <linux/unaligned.h> > > +#include <media/jpeg.h> > #include <media/v4l2-common.h> > > #include "uvcvideo.h" > @@ -1116,6 +1117,7 @@ static void uvc_video_stats_stop(struct uvc_streaming *stream) > static int uvc_video_decode_start(struct uvc_streaming *stream, > struct uvc_buffer *buf, const u8 *data, int len) > { > + u8 header_len; > u8 fid; > > /* > @@ -1129,6 +1131,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > return -EINVAL; > } > > + header_len = data[0]; > fid = data[1] & UVC_STREAM_FID; > > /* > @@ -1210,9 +1213,31 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > return -EAGAIN; > } > > + /* > + * Some cameras, when running two parallel streams (one MJPEG alongside > + * another non-MJPEG stream), are known to lose the EOF packet for a frame. > + * We can detect the end of a frame by checking for a new SOI marker, as > + * the SOI always lies on the packet boundary between two frames for > + * these devices. > + */ > + if (stream->dev->quirks & UVC_QUIRK_MJPEG_NO_EOF && > + (stream->cur_format->fcc == V4L2_PIX_FMT_MJPEG || > + stream->cur_format->fcc == V4L2_PIX_FMT_JPEG)) { > + const u8 *packet = ((const u8 *)data) + header_len; Do you need the (const u8 *) casting here? . I believe data has exactly that datatype > + > + if (len >= header_len + 2 && > + packet[0] == 0xff && packet[1] == JPEG_MARKER_SOI && > + buf->bytesused != 0) { > + buf->state = UVC_BUF_STATE_READY; > + buf->error = 1; > + stream->last_fid ^= UVC_STREAM_FID; > + return -EAGAIN; > + } > + } > + > stream->last_fid = fid; > > - return data[0]; > + return header_len; > } > > static inline enum dma_data_direction uvc_stream_dir( > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index b7d24a853ce4..040073326a24 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -76,6 +76,7 @@ > #define UVC_QUIRK_NO_RESET_RESUME 0x00004000 > #define UVC_QUIRK_DISABLE_AUTOSUSPEND 0x00008000 > #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00010000 > +#define UVC_QUIRK_MJPEG_NO_EOF 0x00020000 > > /* Format flags */ > #define UVC_FMT_FLAG_COMPRESSED 0x00000001 > -- > 2.43.0 > >
Hi Isaac, Thank you for the patch. On Tue, Nov 12, 2024 at 11:29:26AM +0000, Isaac Scott wrote: > Add the definition of a new quirk that supports the Sonix Technology > Co. 292A camera, which uses the AR0330 sensor. The camera supports the > output of two simultaneous streams, which need to be handled > appropriately by the driver. The fact that the camera can output two streams isn't an issue, the problem is that it suffers from the packet drop issue. I would write something along the lines of The Sonix Technology Co. 292A camera (which uses an AR0330 sensor), can produce MJPEG and H.264 streams concurrently. When doing so, it drops the last packets of MJPEG frames every time the H.264 stream generates a key frame. Set the UVC_QUIRK_MJPEG_NO_EOF quick to work around the issue. If you're fine with that, you can just let me know and I'll make the change when applying the patch. There's no need to resubmit the series just for this. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 0fac689c6350..15aee3f2b5f9 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2752,6 +2752,15 @@ static const struct usb_device_id uvc_ids[] = { > .bInterfaceSubClass = 1, > .bInterfaceProtocol = 0, > .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax }, > + /* Sonix Technology Co. Ltd. - 292A IPC AR0330 */ > + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE > + | USB_DEVICE_ID_MATCH_INT_INFO, > + .idVendor = 0x0c45, > + .idProduct = 0x6366, > + .bInterfaceClass = USB_CLASS_VIDEO, > + .bInterfaceSubClass = 1, > + .bInterfaceProtocol = 0, > + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_MJPEG_NO_EOF) }, > /* MT6227 */ > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE > | USB_DEVICE_ID_MATCH_INT_INFO,
On Thu, Nov 14, 2024 at 08:47:52PM +0100, Ricardo Ribalda wrote: > Hi Isaac > > I am curious... How is this v7? I see on the list v1 (implicit, it's not versioned), v5 and v7. I'm curious to know if there's a pattern, we need a few more versions to figure it out :-) Jokes aside, Isaac, you should increment the version number only when sending a new public version. > Remember to run > scripts/checkpatch.pl --strict -g HEAD > > It is complaining about an open parenthesis match here. Not the end of > the world, but rather fix it locally than discussing :) > > Totally optional... maybe you can add to the cover-letter a run of > yavta -c /dev/videoX > with and without the patch. So people can see practically what changes. > > Regards! > > On Tue, 12 Nov 2024 at 12:33, Isaac Scott <isaac.scott@ideasonboard.com> wrote: > > > > Some cameras, such as the Sonix Technology Co. 292A, exhibit issues when > > running two parallel streams, causing USB packets to be dropped when an > > H.264 stream posts a keyframe while an MJPEG stream is running > > simultaneously. This occasionally causes the driver to erroneously > > output two consecutive JPEG images as a single frame. > > > > To fix this, we inspect the buffer, and trigger a new frame when we > > find an SOI. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > --- > > drivers/media/usb/uvc/uvc_video.c | 27 ++++++++++++++++++++++++++- > > drivers/media/usb/uvc/uvcvideo.h | 1 + > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index e00f38dd07d9..9bb41362c48d 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -20,6 +20,7 @@ > > #include <linux/atomic.h> > > #include <linux/unaligned.h> > > > > +#include <media/jpeg.h> > > #include <media/v4l2-common.h> > > > > #include "uvcvideo.h" > > @@ -1116,6 +1117,7 @@ static void uvc_video_stats_stop(struct uvc_streaming *stream) > > static int uvc_video_decode_start(struct uvc_streaming *stream, > > struct uvc_buffer *buf, const u8 *data, int len) > > { > > + u8 header_len; > > u8 fid; > > > > /* > > @@ -1129,6 +1131,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > return -EINVAL; > > } > > > > + header_len = data[0]; > > fid = data[1] & UVC_STREAM_FID; > > > > /* > > @@ -1210,9 +1213,31 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > return -EAGAIN; > > } > > > > + /* > > + * Some cameras, when running two parallel streams (one MJPEG alongside > > + * another non-MJPEG stream), are known to lose the EOF packet for a frame. > > + * We can detect the end of a frame by checking for a new SOI marker, as > > + * the SOI always lies on the packet boundary between two frames for > > + * these devices. > > + */ > > + if (stream->dev->quirks & UVC_QUIRK_MJPEG_NO_EOF && > > + (stream->cur_format->fcc == V4L2_PIX_FMT_MJPEG || > > + stream->cur_format->fcc == V4L2_PIX_FMT_JPEG)) { > > + const u8 *packet = ((const u8 *)data) + header_len; > > Do you need the (const u8 *) casting here? . I believe data has > exactly that datatype It doesn't seem to be needed indeed. Can you send a new version that fixes these two issues (checkpatch warning and unnecessary cast), as well as address the comment on 2/2 ? > > + > > + if (len >= header_len + 2 && > > + packet[0] == 0xff && packet[1] == JPEG_MARKER_SOI && > > + buf->bytesused != 0) { > > + buf->state = UVC_BUF_STATE_READY; > > + buf->error = 1; > > + stream->last_fid ^= UVC_STREAM_FID; > > + return -EAGAIN; > > + } > > + } > > + > > stream->last_fid = fid; > > > > - return data[0]; > > + return header_len; > > } > > > > static inline enum dma_data_direction uvc_stream_dir( > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index b7d24a853ce4..040073326a24 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -76,6 +76,7 @@ > > #define UVC_QUIRK_NO_RESET_RESUME 0x00004000 > > #define UVC_QUIRK_DISABLE_AUTOSUSPEND 0x00008000 > > #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00010000 > > +#define UVC_QUIRK_MJPEG_NO_EOF 0x00020000 > > > > /* Format flags */ > > #define UVC_FMT_FLAG_COMPRESSED 0x00000001