| Message ID | 20220909221335.15033-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: gadget: uvc: parse configfs entries and implement v4l2 enum api calls | expand |
Hi Michael, On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: > This series improves the uvc video gadget by parsing the configfs > entries. With the configfs data, the userspace now is able to use simple > v4l2 api calls like enum and try_format to check for valid configurations > initially set by configfs. I've realized that this whole series got merged, despite my multiple attempts to explain why I think it's not a good idea. The UVC gadget requires userspace support, and there's no point in trying to move all these things to the kernel side. It only bloats the kernel, makes the code more complex, more difficult to maintain, and will make UVC 1.5 support more difficult. I'm fairly unhappy with this, it will lower my trust towards your patches. > Michael Grzeschik (4): > media: v4l: move helper functions for fractions from uvc to > v4l2-common > media: uvcvideo: move uvc_format_desc to common header > usb: gadget: uvc: add v4l2 enumeration api calls > usb: gadget: uvc: add v4l2 try_format api call > > drivers/media/usb/uvc/uvc_ctrl.c | 1 + > drivers/media/usb/uvc/uvc_driver.c | 290 +------------------- > drivers/media/usb/uvc/uvc_v4l2.c | 14 +- > drivers/media/usb/uvc/uvcvideo.h | 147 ---------- > drivers/media/v4l2-core/v4l2-common.c | 86 ++++++ > drivers/usb/gadget/function/f_uvc.c | 30 +++ > drivers/usb/gadget/function/uvc.h | 2 + > drivers/usb/gadget/function/uvc_v4l2.c | 286 ++++++++++++++++++++ > include/media/v4l2-common.h | 4 + > include/media/v4l2-uvc.h | 359 +++++++++++++++++++++++++ > 10 files changed, 776 insertions(+), 443 deletions(-) > create mode 100644 include/media/v4l2-uvc.h
Hi Laurent, On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote: >On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: >> This series improves the uvc video gadget by parsing the configfs >> entries. With the configfs data, the userspace now is able to use simple >> v4l2 api calls like enum and try_format to check for valid configurations >> initially set by configfs. > >I've realized that this whole series got merged, despite my multiple >attempts to explain why I think it's not a good idea. The UVC gadget >requires userspace support, and there's no point in trying to move all >these things to the kernel side. It only bloats the kernel, makes the >code more complex, more difficult to maintain, and will make UVC 1.5 >support more difficult. Those patches that got merged are already a compromise. And I think a better one. Since the last rounds I realized that many steps that I thought would be needed in the kernel, can indeed be made in userspace. So beside thise code that only adds the typical vidioc functions to parse what is configured in configfs. There is nothing that changed the working function of the gadget. That said, it is up to you to use the vidiocs or your application sticks with own parsing of the configfs. So, with that said, I am unsure what you are exactly unhappy about. Beside the points you mentioned in the previous mail. >I'm fairly unhappy with this, it will lower my trust towards your >patches. If you don't trust my patches, than review them or at least nack them with a comment so we have an object of disscussion. >> Michael Grzeschik (4): >> media: v4l: move helper functions for fractions from uvc to >> v4l2-common >> media: uvcvideo: move uvc_format_desc to common header >> usb: gadget: uvc: add v4l2 enumeration api calls >> usb: gadget: uvc: add v4l2 try_format api call >> >> drivers/media/usb/uvc/uvc_ctrl.c | 1 + >> drivers/media/usb/uvc/uvc_driver.c | 290 +------------------- >> drivers/media/usb/uvc/uvc_v4l2.c | 14 +- >> drivers/media/usb/uvc/uvcvideo.h | 147 ---------- >> drivers/media/v4l2-core/v4l2-common.c | 86 ++++++ >> drivers/usb/gadget/function/f_uvc.c | 30 +++ >> drivers/usb/gadget/function/uvc.h | 2 + >> drivers/usb/gadget/function/uvc_v4l2.c | 286 ++++++++++++++++++++ >> include/media/v4l2-common.h | 4 + >> include/media/v4l2-uvc.h | 359 +++++++++++++++++++++++++ >> 10 files changed, 776 insertions(+), 443 deletions(-) >> create mode 100644 include/media/v4l2-uvc.h > >-- >Regards, > >Laurent Pinchart >
On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote: > Hi Michael, > > On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: > > This series improves the uvc video gadget by parsing the configfs > > entries. With the configfs data, the userspace now is able to use simple > > v4l2 api calls like enum and try_format to check for valid configurations > > initially set by configfs. > > I've realized that this whole series got merged, despite my multiple > attempts to explain why I think it's not a good idea. The UVC gadget > requires userspace support, and there's no point in trying to move all > these things to the kernel side. It only bloats the kernel, makes the > code more complex, more difficult to maintain, and will make UVC 1.5 > support more difficult. I can easily revert them, but I did not see any objections to them originally and so I merged them as is the normal method :) thanks, greg k-h
On Sun, Dec 04, 2022 at 09:29:16AM +0100, Greg KH wrote: > On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote: > > Hi Michael, > > > > On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: > > > This series improves the uvc video gadget by parsing the configfs > > > entries. With the configfs data, the userspace now is able to use simple > > > v4l2 api calls like enum and try_format to check for valid configurations > > > initially set by configfs. > > > > I've realized that this whole series got merged, despite my multiple > > attempts to explain why I think it's not a good idea. The UVC gadget > > requires userspace support, and there's no point in trying to move all > > these things to the kernel side. It only bloats the kernel, makes the > > code more complex, more difficult to maintain, and will make UVC 1.5 > > support more difficult. > > I can easily revert them, but I did not see any objections to them > originally and so I merged them as is the normal method :) I don't think a revert is needed. The issue I pointed out regarding the duplication of static const data can be solved on top. The API additions from this series are, in my opinion, not a good idea for the reasons I explained, but they don't hurt so much that we need to go nuclear on this. Michael, will you be addressing the static const data issue ?
On Mon, Dec 05, 2022 at 11:17:15PM +0200, Laurent Pinchart wrote: >On Sun, Dec 04, 2022 at 09:29:16AM +0100, Greg KH wrote: >> On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote: >> > Hi Michael, >> > >> > On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: >> > > This series improves the uvc video gadget by parsing the configfs >> > > entries. With the configfs data, the userspace now is able to use simple >> > > v4l2 api calls like enum and try_format to check for valid configurations >> > > initially set by configfs. >> > >> > I've realized that this whole series got merged, despite my multiple >> > attempts to explain why I think it's not a good idea. The UVC gadget >> > requires userspace support, and there's no point in trying to move all >> > these things to the kernel side. It only bloats the kernel, makes the >> > code more complex, more difficult to maintain, and will make UVC 1.5 >> > support more difficult. >> >> I can easily revert them, but I did not see any objections to them >> originally and so I merged them as is the normal method :) > >I don't think a revert is needed. The issue I pointed out regarding the >duplication of static const data can be solved on top. The API additions >from this series are, in my opinion, not a good idea for the reasons I >explained, but they don't hurt so much that we need to go nuclear on >this. > >Michael, will you be addressing the static const data issue ? Yes. I will also move the uvc_fmts[] array and uvc_format_by_guid to its own compile unit. I will go with drivers/media/usb/uvc.c While at it the headerfile will better also be moved from include/media/v4l2-uvc.h to linux/usb/uvc.h. Thanks, Michael
Hi Michael On Tue, 6 Dec 2022 at 18:09, Michael Grzeschik <mgr@pengutronix.de> wrote: > > On Mon, Dec 05, 2022 at 11:17:15PM +0200, Laurent Pinchart wrote: > >On Sun, Dec 04, 2022 at 09:29:16AM +0100, Greg KH wrote: > >> On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote: > >> > Hi Michael, > >> > > >> > On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: > >> > > This series improves the uvc video gadget by parsing the configfs > >> > > entries. With the configfs data, the userspace now is able to use simple > >> > > v4l2 api calls like enum and try_format to check for valid configurations > >> > > initially set by configfs. > >> > > >> > I've realized that this whole series got merged, despite my multiple > >> > attempts to explain why I think it's not a good idea. The UVC gadget > >> > requires userspace support, and there's no point in trying to move all > >> > these things to the kernel side. It only bloats the kernel, makes the > >> > code more complex, more difficult to maintain, and will make UVC 1.5 > >> > support more difficult. > >> > >> I can easily revert them, but I did not see any objections to them > >> originally and so I merged them as is the normal method :) > > > >I don't think a revert is needed. The issue I pointed out regarding the > >duplication of static const data can be solved on top. The API additions > >from this series are, in my opinion, not a good idea for the reasons I > >explained, but they don't hurt so much that we need to go nuclear on > >this. > > > >Michael, will you be addressing the static const data issue ? > > Yes. I will also move the uvc_fmts[] array and uvc_format_by_guid to its > own compile unit. > > I will go with drivers/media/usb/uvc.c > > While at it the headerfile will better also be moved from > include/media/v4l2-uvc.h to linux/usb/uvc.h. And since it is free to ask :P.... Could you also try to remove the string definition and use the one from v4l2-ioctl.c. ? Or maybe it is not feasible? Thanks! > > Thanks, > Michael > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Michael, On Tue, Dec 06, 2022 at 06:07:21PM +0100, Michael Grzeschik wrote: > On Mon, Dec 05, 2022 at 11:17:15PM +0200, Laurent Pinchart wrote: > >On Sun, Dec 04, 2022 at 09:29:16AM +0100, Greg KH wrote: > >> On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote: > >> > On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote: > >> > > This series improves the uvc video gadget by parsing the configfs > >> > > entries. With the configfs data, the userspace now is able to use simple > >> > > v4l2 api calls like enum and try_format to check for valid configurations > >> > > initially set by configfs. > >> > > >> > I've realized that this whole series got merged, despite my multiple > >> > attempts to explain why I think it's not a good idea. The UVC gadget > >> > requires userspace support, and there's no point in trying to move all > >> > these things to the kernel side. It only bloats the kernel, makes the > >> > code more complex, more difficult to maintain, and will make UVC 1.5 > >> > support more difficult. > >> > >> I can easily revert them, but I did not see any objections to them > >> originally and so I merged them as is the normal method :) > > > > I don't think a revert is needed. The issue I pointed out regarding the > > duplication of static const data can be solved on top. The API additions > > from this series are, in my opinion, not a good idea for the reasons I > > explained, but they don't hurt so much that we need to go nuclear on > > this. > > > > Michael, will you be addressing the static const data issue ? > > Yes. I will also move the uvc_fmts[] array and uvc_format_by_guid to its > own compile unit. Thank you. > I will go with drivers/media/usb/uvc.c > > While at it the headerfile will better also be moved from > include/media/v4l2-uvc.h to linux/usb/uvc.h. Works for me, especially for the GUIDs. For the structure and function prototype, I don't mind using include/media/, up to you.
This series improves the uvc video gadget by parsing the configfs entries. With the configfs data, the userspace now is able to use simple v4l2 api calls like enum and try_format to check for valid configurations initially set by configfs. Michael Grzeschik (4): media: v4l: move helper functions for fractions from uvc to v4l2-common media: uvcvideo: move uvc_format_desc to common header usb: gadget: uvc: add v4l2 enumeration api calls usb: gadget: uvc: add v4l2 try_format api call drivers/media/usb/uvc/uvc_ctrl.c | 1 + drivers/media/usb/uvc/uvc_driver.c | 290 +------------------- drivers/media/usb/uvc/uvc_v4l2.c | 14 +- drivers/media/usb/uvc/uvcvideo.h | 147 ---------- drivers/media/v4l2-core/v4l2-common.c | 86 ++++++ drivers/usb/gadget/function/f_uvc.c | 30 +++ drivers/usb/gadget/function/uvc.h | 2 + drivers/usb/gadget/function/uvc_v4l2.c | 286 ++++++++++++++++++++ include/media/v4l2-common.h | 4 + include/media/v4l2-uvc.h | 359 +++++++++++++++++++++++++ 10 files changed, 776 insertions(+), 443 deletions(-) create mode 100644 include/media/v4l2-uvc.h