> Date: Mon, 5 Apr 2021 23:19:02 +0200 (CEST)
> From: Mark Kettenis <mark.kette...@xs4all.nl>
>
> > Date: Mon, 5 Apr 2021 23:15:23 +0200
> > From: Marcus Glocker <mar...@nazgul.ch>
> >
> > On Mon, Apr 05, 2021 at 07:30:43AM -0700, Greg Steuck wrote:
> >
> > > OK gnezdo with a usability question inline.
> >
> > Thanks. See below.
> >
> > > Marcus Glocker <mar...@nazgul.ch> writes:
> > >
> > > > martijn@ has recently reported that in his machine he has two cams
> > > > of which one is doing IR, which isn't really supported by uvideo(4).
> > > > This IR device attaches always first as uvideo0, so he needs to swap
> > > > that regularly with his working cam which by default attaches to
> > > > uvideo1.
> > > >
> > > > I came up with a new quirk flag to *not* attach certain devices. Tested
> > > > successfully by martijn@, the IR cam attaches to ugen0 and the supported
> > > > cam to uvideo0.
> > > >
> > > > This patch shouldn't affect any supported uvideo(4) devices.
> > > >
> > > > OK?
> > > >
> > > > Index: sys/dev/usb/uvideo.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> > > > retrieving revision 1.211
> > > > diff -u -p -u -p -r1.211 uvideo.c
> > > > --- sys/dev/usb/uvideo.c 27 Jan 2021 17:28:19 -0000 1.211
> > > > +++ sys/dev/usb/uvideo.c 8 Mar 2021 22:06:51 -0000
> > > > @@ -307,6 +307,7 @@ struct video_hw_if uvideo_hw_if = {
> > > > #define UVIDEO_FLAG_ISIGHT_STREAM_HEADER 0x1
> > > > #define UVIDEO_FLAG_REATTACH 0x2
> > > > #define UVIDEO_FLAG_VENDOR_CLASS 0x4
> > > > +#define UVIDEO_FLAG_NOATTACH 0x8
> > > > struct uvideo_devs {
> > > > struct usb_devno uv_dev;
> > > > char *ucode_name;
> > > > @@ -382,6 +383,12 @@ struct uvideo_devs {
> > > > NULL,
> > > > UVIDEO_FLAG_VENDOR_CLASS
> > > > },
> > > > + { /* Infrared camera not supported */
> > > > + { USB_VENDOR_CHICONY, USB_PRODUCT_CHICONY_IRCAMERA },
> > > > + NULL,
> > > > + NULL,
> > > > + UVIDEO_FLAG_NOATTACH
> > > > + },
> > > > };
> > > > #define uvideo_lookup(v, p) \
> > > > ((struct uvideo_devs *)usb_lookup(uvideo_devs, v, p))
> > > > @@ -480,13 +487,12 @@ uvideo_match(struct device *parent, void
> > > > if (id == NULL)
> > > > return (UMATCH_NONE);
> > > >
> > > > - if (id->bInterfaceClass == UICLASS_VIDEO &&
> > > > - id->bInterfaceSubClass == UISUBCLASS_VIDEOCONTROL)
> > > > - return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
> > > > -
> > > > - /* quirk devices which we want to attach */
> > > > + /* quirk devices */
> > > > quirk = uvideo_lookup(uaa->vendor, uaa->product);
> > > > if (quirk != NULL) {
> > > > + if (quirk->flags & UVIDEO_FLAG_NOATTACH)
> > >
> > > How common is it to explain the system behavior in cases like this?
> > > Would it be less surprising (generate less misc@ traffic) if we printed
> > > a note explaining why the camera was skipped?
> >
> > I wouldn't print a specific message per unsupported device, but I think
> > a generic message that the video device isn't supported would make
> > sense. Something like that maybe? Obviously this can print more than
> > once, but I'm not sure if it's worth to make that a unique print.
> >
> > E.g.:
> >
> > vmm0 at mainbus0: VMX/EPT
> > uvideo: device 13d3:56b2 isn't supported
> > uvideo: device 13d3:56b2 isn't supported
> > ugen0 at uhub0 port 8 "SunplusIT Inc Integrated Camera" rev 2.01/17.11 addr
> > 2
>
> No; match functions shouldn't print stuff.
If you really wanted to print a message, you'll need to let uvideo(4)
attach and then print a "not supported" message early on in the attach
function and return.
> > Index: sys/dev/usb/uvideo.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> > retrieving revision 1.212
> > diff -u -p -u -p -r1.212 uvideo.c
> > --- sys/dev/usb/uvideo.c 5 Apr 2021 20:45:49 -0000 1.212
> > +++ sys/dev/usb/uvideo.c 5 Apr 2021 21:09:36 -0000
> > @@ -490,8 +490,11 @@ uvideo_match(struct device *parent, void
> > /* quirk devices */
> > quirk = uvideo_lookup(uaa->vendor, uaa->product);
> > if (quirk != NULL) {
> > - if (quirk->flags & UVIDEO_FLAG_NOATTACH)
> > + if (quirk->flags & UVIDEO_FLAG_NOATTACH) {
> > + printf("uvideo: device %x:%x isn't supported\n",
> > + uaa->vendor, uaa->product);
> > return (UMATCH_NONE);
> > + }
> >
> > if (quirk->flags & UVIDEO_FLAG_REATTACH)
> > return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
> >
> >
>
>
