From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Michael Grzeschik <mgr@pengutronix.de>
Cc: gregkh@linuxfoundation.org, mchehab@kernel.org,
hverkuil-cisco@xs4all.nl, linux-usb@vger.kernel.org,
linux-media@vger.kernel.org, kernel@pengutronix.de,
Daniel Scally <dan.scally@ideasonboard.com>
Subject: Re: [PATCH v2 5/5] usb: uvc: use v4l2_fill_fmtdesc instead of open coded format name
Date: Mon, 9 Jan 2023 17:52:40 +0200 [thread overview]
Message-ID: <Y7w4SJrOcMOhxcED@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230109131905.GA19093@pengutronix.de>
Hi Michael,
On Mon, Jan 09, 2023 at 02:19:05PM +0100, Michael Grzeschik wrote:
> On Wed, Dec 28, 2022 at 03:50:47AM +0200, Laurent Pinchart wrote:
> > On Thu, Dec 15, 2022 at 11:45:14PM +0100, Michael Grzeschik wrote:
> >> Since we have the helper function v4l2_fill_fmtdesc, we can use this to
> >> get the corresponding descriptive string for the pixelformat and set the
> >> compressed flag. This patch is removing the redundant name field in
> >> uvc_format_desc and makes use of v4l2_fill_fmtdesc instead.
> >
> > I really like that.
> >
> >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >>
> >> ---
> >> v1 -> v2: - added reviewed and tested tags
> >> ---
> >> drivers/media/common/uvc.c | 37 --------------------------
> >> drivers/media/usb/uvc/uvc_driver.c | 8 +++++-
> >> drivers/usb/gadget/function/uvc_v4l2.c | 6 +----
> >> include/linux/usb/uvc.h | 1 -
> >> 4 files changed, 8 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/drivers/media/common/uvc.c b/drivers/media/common/uvc.c
> >> index a9d587490de8d5..ab2637b9b39b2a 100644
> >> --- a/drivers/media/common/uvc.c
> >> +++ b/drivers/media/common/uvc.c
> >> @@ -13,187 +13,150 @@
> >>
> >> static const struct uvc_format_desc uvc_fmts[] = {
> >> {
> >> - .name = "YUV 4:2:2 (YUYV)",
> >> .guid = UVC_GUID_FORMAT_YUY2,
> >> .fcc = V4L2_PIX_FMT_YUYV,
> >> },
> >> {
> >> - .name = "YUV 4:2:2 (YUYV)",
> >> .guid = UVC_GUID_FORMAT_YUY2_ISIGHT,
> >> .fcc = V4L2_PIX_FMT_YUYV,
> >> },
> >> {
> >> - .name = "YUV 4:2:0 (NV12)",
> >> .guid = UVC_GUID_FORMAT_NV12,
> >> .fcc = V4L2_PIX_FMT_NV12,
> >> },
> >> {
> >> - .name = "MJPEG",
> >> .guid = UVC_GUID_FORMAT_MJPEG,
> >> .fcc = V4L2_PIX_FMT_MJPEG,
> >> },
> >> {
> >> - .name = "YVU 4:2:0 (YV12)",
> >> .guid = UVC_GUID_FORMAT_YV12,
> >> .fcc = V4L2_PIX_FMT_YVU420,
> >> },
> >> {
> >> - .name = "YUV 4:2:0 (I420)",
> >> .guid = UVC_GUID_FORMAT_I420,
> >> .fcc = V4L2_PIX_FMT_YUV420,
> >> },
> >> {
> >> - .name = "YUV 4:2:0 (M420)",
> >> .guid = UVC_GUID_FORMAT_M420,
> >> .fcc = V4L2_PIX_FMT_M420,
> >> },
> >> {
> >> - .name = "YUV 4:2:2 (UYVY)",
> >> .guid = UVC_GUID_FORMAT_UYVY,
> >> .fcc = V4L2_PIX_FMT_UYVY,
> >> },
> >> {
> >> - .name = "Greyscale 8-bit (Y800)",
> >> .guid = UVC_GUID_FORMAT_Y800,
> >> .fcc = V4L2_PIX_FMT_GREY,
> >> },
> >> {
> >> - .name = "Greyscale 8-bit (Y8 )",
> >> .guid = UVC_GUID_FORMAT_Y8,
> >> .fcc = V4L2_PIX_FMT_GREY,
> >> },
> >> {
> >> - .name = "Greyscale 8-bit (D3DFMT_L8)",
> >> .guid = UVC_GUID_FORMAT_D3DFMT_L8,
> >> .fcc = V4L2_PIX_FMT_GREY,
> >> },
> >> {
> >> - .name = "IR 8-bit (L8_IR)",
> >> .guid = UVC_GUID_FORMAT_KSMEDIA_L8_IR,
> >> .fcc = V4L2_PIX_FMT_GREY,
> >> },
> >> {
> >> - .name = "Greyscale 10-bit (Y10 )",
> >> .guid = UVC_GUID_FORMAT_Y10,
> >> .fcc = V4L2_PIX_FMT_Y10,
> >> },
> >> {
> >> - .name = "Greyscale 12-bit (Y12 )",
> >> .guid = UVC_GUID_FORMAT_Y12,
> >> .fcc = V4L2_PIX_FMT_Y12,
> >> },
> >> {
> >> - .name = "Greyscale 16-bit (Y16 )",
> >> .guid = UVC_GUID_FORMAT_Y16,
> >> .fcc = V4L2_PIX_FMT_Y16,
> >> },
> >> {
> >> - .name = "BGGR Bayer (BY8 )",
> >> .guid = UVC_GUID_FORMAT_BY8,
> >> .fcc = V4L2_PIX_FMT_SBGGR8,
> >> },
> >> {
> >> - .name = "BGGR Bayer (BA81)",
> >> .guid = UVC_GUID_FORMAT_BA81,
> >> .fcc = V4L2_PIX_FMT_SBGGR8,
> >> },
> >> {
> >> - .name = "GBRG Bayer (GBRG)",
> >> .guid = UVC_GUID_FORMAT_GBRG,
> >> .fcc = V4L2_PIX_FMT_SGBRG8,
> >> },
> >> {
> >> - .name = "GRBG Bayer (GRBG)",
> >> .guid = UVC_GUID_FORMAT_GRBG,
> >> .fcc = V4L2_PIX_FMT_SGRBG8,
> >> },
> >> {
> >> - .name = "RGGB Bayer (RGGB)",
> >> .guid = UVC_GUID_FORMAT_RGGB,
> >> .fcc = V4L2_PIX_FMT_SRGGB8,
> >> },
> >> {
> >> - .name = "RGB565",
> >> .guid = UVC_GUID_FORMAT_RGBP,
> >> .fcc = V4L2_PIX_FMT_RGB565,
> >> },
> >> {
> >> - .name = "BGR 8:8:8 (BGR3)",
> >> .guid = UVC_GUID_FORMAT_BGR3,
> >> .fcc = V4L2_PIX_FMT_BGR24,
> >> },
> >> {
> >> - .name = "H.264",
> >> .guid = UVC_GUID_FORMAT_H264,
> >> .fcc = V4L2_PIX_FMT_H264,
> >> },
> >> {
> >> - .name = "H.265",
> >> .guid = UVC_GUID_FORMAT_H265,
> >> .fcc = V4L2_PIX_FMT_HEVC,
> >> },
> >> {
> >> - .name = "Greyscale 8 L/R (Y8I)",
> >> .guid = UVC_GUID_FORMAT_Y8I,
> >> .fcc = V4L2_PIX_FMT_Y8I,
> >> },
> >> {
> >> - .name = "Greyscale 12 L/R (Y12I)",
> >> .guid = UVC_GUID_FORMAT_Y12I,
> >> .fcc = V4L2_PIX_FMT_Y12I,
> >> },
> >> {
> >> - .name = "Depth data 16-bit (Z16)",
> >> .guid = UVC_GUID_FORMAT_Z16,
> >> .fcc = V4L2_PIX_FMT_Z16,
> >> },
> >> {
> >> - .name = "Bayer 10-bit (SRGGB10P)",
> >> .guid = UVC_GUID_FORMAT_RW10,
> >> .fcc = V4L2_PIX_FMT_SRGGB10P,
> >> },
> >> {
> >> - .name = "Bayer 16-bit (SBGGR16)",
> >> .guid = UVC_GUID_FORMAT_BG16,
> >> .fcc = V4L2_PIX_FMT_SBGGR16,
> >> },
> >> {
> >> - .name = "Bayer 16-bit (SGBRG16)",
> >> .guid = UVC_GUID_FORMAT_GB16,
> >> .fcc = V4L2_PIX_FMT_SGBRG16,
> >> },
> >> {
> >> - .name = "Bayer 16-bit (SRGGB16)",
> >> .guid = UVC_GUID_FORMAT_RG16,
> >> .fcc = V4L2_PIX_FMT_SRGGB16,
> >> },
> >> {
> >> - .name = "Bayer 16-bit (SGRBG16)",
> >> .guid = UVC_GUID_FORMAT_GR16,
> >> .fcc = V4L2_PIX_FMT_SGRBG16,
> >> },
> >> {
> >> - .name = "Depth data 16-bit (Z16)",
> >> .guid = UVC_GUID_FORMAT_INVZ,
> >> .fcc = V4L2_PIX_FMT_Z16,
> >> },
> >> {
> >> - .name = "Greyscale 10-bit (Y10 )",
> >> .guid = UVC_GUID_FORMAT_INVI,
> >> .fcc = V4L2_PIX_FMT_Y10,
> >> },
> >> {
> >> - .name = "IR:Depth 26-bit (INZI)",
> >> .guid = UVC_GUID_FORMAT_INZI,
> >> .fcc = V4L2_PIX_FMT_INZI,
> >> },
> >> {
> >> - .name = "4-bit Depth Confidence (Packed)",
> >> .guid = UVC_GUID_FORMAT_CNF4,
> >> .fcc = V4L2_PIX_FMT_CNF4,
> >> },
> >> {
> >> - .name = "HEVC",
> >> .guid = UVC_GUID_FORMAT_HEVC,
> >> .fcc = V4L2_PIX_FMT_HEVC,
> >> },
> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >> index 12b6ad0966d94a..af92e730bde7c7 100644
> >> --- a/drivers/media/usb/uvc/uvc_driver.c
> >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> @@ -251,7 +251,13 @@ static int uvc_parse_format(struct uvc_device *dev,
> >> fmtdesc = uvc_format_by_guid(&buffer[5]);
> >>
> >> if (fmtdesc != NULL) {
> >> - strscpy(format->name, fmtdesc->name,
> >> + struct v4l2_fmtdesc fmt;
> >> +
> >> + fmt.pixelformat = fmtdesc->fcc;
> >> +
> >> + v4l2_fill_fmtdesc(&fmt);
> >> +
> >> + strscpy(format->name, fmt.description,
> >> sizeof(format->name));
> >> format->fcc = fmtdesc->fcc;
> >> } else {
> >
> > I've just sent "[PATCH v1] media: uvcvideo: Remove format descriptions"
> > which drops usage of the name in the uvcvideo driver without having to
> > call v4l2_fill_fmtdesc(). With that merged, changes to uvc_driver.c can
> > be dropped in this patch.
>
> Right.
>
> >> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> >> index 21e573e628f4e7..6e46fa1695f212 100644
> >> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> >> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> >> @@ -374,14 +374,10 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> >> if (!uformat)
> >> return -EINVAL;
> >>
> >> - if (uformat->type != UVCG_UNCOMPRESSED)
> >> - f->flags |= V4L2_FMT_FLAG_COMPRESSED;
> >> -
> >> fmtdesc = to_uvc_format(uformat);
> >> f->pixelformat = fmtdesc->fcc;
> >>
> >> - strscpy(f->description, fmtdesc->name, sizeof(f->description));
> >> - f->description[strlen(fmtdesc->name) - 1] = 0;
> >> + v4l2_fill_fmtdesc(f);
> >
> >
> > v4l_fill_fmtdesc() is actually called by v4l_enum_fmt() after calling
> > the driver's .vidioc_enum_fmt_vid_out() operation, so you don't have to
> > call it manually here.
> >
> > By dropping the manual calls to v4l_fill_fmtdesc(), you can also drop
> > patch 4/5 in the series.
>
> This is a good point. I have dropped patch 4/5 in the stack.
>
> > This creates a dependency between the patch I've just sent and this
> > series. As I don't want to introduce any further delay, I'll create a
> > stable branch based on v6.2-rc1 as soon as my patch gets reviewed, you
> > you can then base the next version of this series on top of it. An
> > alternative would be to merge this series through the media tree if Greg
> > is OK with that.
>
> I found your v3 patch. I sure can rebase my work on your patch if your
> stable branch is available.
Could you review the uvcvideo patch ? I will then add your Reviewed-by
tag and create a stable branch.
> >> return 0;
> >> }
> >> diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
> >> index 227a03f252a5c0..e407a7b8a91c70 100644
> >> --- a/include/linux/usb/uvc.h
> >> +++ b/include/linux/usb/uvc.h
> >> @@ -146,7 +146,6 @@
> >> 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
> >>
> >> struct uvc_format_desc {
> >> - char *name;
> >> u8 guid[16];
> >> u32 fcc;
> >> };
--
Regards,
Laurent Pinchart
prev parent reply other threads:[~2023-01-09 15:52 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-15 22:45 [PATCH v2 0/5] usb: uvc: improve header files and function use Michael Grzeschik
2022-12-15 22:45 ` [PATCH v2 1/5] usb: uvc: move media/v4l2-uvc.h to usb/uvc.h Michael Grzeschik
2022-12-15 22:45 ` [PATCH v2 2/5] usb: uvc: move uvc_fmts and uvc_format_by_guid to own compile unit Michael Grzeschik
2022-12-15 22:45 ` [PATCH v2 3/5] usb: uvc: make uvc_format_desc table const Michael Grzeschik
2022-12-15 22:45 ` [PATCH v2 4/5] media: v4l2: move v4l_fill_fmtdesc to common v4l2_fill_fmtdesc function Michael Grzeschik
2022-12-15 22:45 ` [PATCH v2 5/5] usb: uvc: use v4l2_fill_fmtdesc instead of open coded format name Michael Grzeschik
2022-12-28 1:50 ` Laurent Pinchart
2023-01-09 13:19 ` Michael Grzeschik
2023-01-09 15:52 ` Laurent Pinchart [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y7w4SJrOcMOhxcED@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=dan.scally@ideasonboard.com \
--cc=gregkh@linuxfoundation.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=kernel@pengutronix.de \
--cc=linux-media@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=mgr@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox