uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Marcus Glocker
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)
+ return (UMATCH_NONE);
+
  if (quirk->flags & UVIDEO_FLAG_REATTACH)
  return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
 
@@ -495,6 +501,10 @@ uvideo_match(struct device *parent, void
     id->bInterfaceSubClass == UISUBCLASS_VIDEOCONTROL)
  return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
  }
+
+ if (id->bInterfaceClass == UICLASS_VIDEO &&
+    id->bInterfaceSubClass == UISUBCLASS_VIDEOCONTROL)
+ return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
 
  return (UMATCH_NONE);
 }
Index: sys/dev/usb/usbdevs
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdevs,v
retrieving revision 1.731
diff -u -p -u -p -r1.731 usbdevs
--- sys/dev/usb/usbdevs 27 Feb 2021 03:03:40 -0000 1.731
+++ sys/dev/usb/usbdevs 8 Mar 2021 22:06:53 -0000
@@ -1336,6 +1336,7 @@ product CHICONY RTL8188CUS_3 0xaff9 RTL8
 product CHICONY RTL8188CUS_4 0xaffa RTL8188CUS
 product CHICONY RTL8188CUS_5 0xaffb RTL8188CUS
 product CHICONY RTL8188CUS_6 0xaffc RTL8188CUS
+product CHICONY IRCAMERA 0xb615 Integrated IR Camera
 
 /* CH Products */
 product CHPRODUCTS PROTHROTTLE 0x00f1 Pro Throttle

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Martijn van Duren-5
I'm in no way qualified to OK this, but I'd like to see this get in,
because it does help me out :-)

martijn@

On Mon, 2021-03-15 at 08:35 +0100, Marcus Glocker wrote:

> 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)
> +                       return (UMATCH_NONE);
> +
>                 if (quirk->flags & UVIDEO_FLAG_REATTACH)
>                         return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
>  
> @@ -495,6 +501,10 @@ uvideo_match(struct device *parent, void
>                     id->bInterfaceSubClass == UISUBCLASS_VIDEOCONTROL)
>                         return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
>         }
> +
> +       if (id->bInterfaceClass == UICLASS_VIDEO &&
> +           id->bInterfaceSubClass == UISUBCLASS_VIDEOCONTROL)
> +               return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
>  
>         return (UMATCH_NONE);
>  }
> Index: sys/dev/usb/usbdevs
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/usbdevs,v
> retrieving revision 1.731
> diff -u -p -u -p -r1.731 usbdevs
> --- sys/dev/usb/usbdevs 27 Feb 2021 03:03:40 -0000      1.731
> +++ sys/dev/usb/usbdevs 8 Mar 2021 22:06:53 -0000
> @@ -1336,6 +1336,7 @@ product CHICONY RTL8188CUS_3      0xaff9  RTL8
>  product CHICONY RTL8188CUS_4   0xaffa  RTL8188CUS
>  product CHICONY RTL8188CUS_5   0xaffb  RTL8188CUS
>  product CHICONY RTL8188CUS_6   0xaffc  RTL8188CUS
> +product CHICONY IRCAMERA       0xb615  Integrated IR Camera
>  
>  /* CH Products */
>  product CHPRODUCTS PROTHROTTLE 0x00f1  Pro Throttle
>


Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Marcus Glocker
This diff shouldn't impact any other uvideo(4) devices so I would
welcome somebody brave to OK this :-)

Thanks,
Marcus

On Fri, 26 Mar 2021 10:15:58 +0100
Martijn van Duren <[hidden email]> wrote:

> I'm in no way qualified to OK this, but I'd like to see this get in,
> because it does help me out :-)
>
> martijn@
>
> On Mon, 2021-03-15 at 08:35 +0100, Marcus Glocker wrote:
> > 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)
> > +                       return (UMATCH_NONE);
> > +
> >                 if (quirk->flags & UVIDEO_FLAG_REATTACH)
> >                         return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
> >  
> > @@ -495,6 +501,10 @@ uvideo_match(struct device *parent, void
> >                     id->bInterfaceSubClass ==
> > UISUBCLASS_VIDEOCONTROL) return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
> >         }
> > +
> > +       if (id->bInterfaceClass == UICLASS_VIDEO &&
> > +           id->bInterfaceSubClass == UISUBCLASS_VIDEOCONTROL)
> > +               return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
> >  
> >         return (UMATCH_NONE);
> >  }
> > Index: sys/dev/usb/usbdevs
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usbdevs,v
> > retrieving revision 1.731
> > diff -u -p -u -p -r1.731 usbdevs
> > --- sys/dev/usb/usbdevs 27 Feb 2021 03:03:40 -0000      1.731
> > +++ sys/dev/usb/usbdevs 8 Mar 2021 22:06:53 -0000
> > @@ -1336,6 +1336,7 @@ product CHICONY RTL8188CUS_3      0xaff9  RTL8
> >  product CHICONY RTL8188CUS_4   0xaffa  RTL8188CUS
> >  product CHICONY RTL8188CUS_5   0xaffb  RTL8188CUS
> >  product CHICONY RTL8188CUS_6   0xaffc  RTL8188CUS
> > +product CHICONY IRCAMERA       0xb615  Integrated IR Camera
> >  
> >  /* CH Products */
> >  product CHPRODUCTS PROTHROTTLE 0x00f1  Pro Throttle
> >  
>
>

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Greg Steuck-5
In reply to this post by Marcus Glocker
OK gnezdo with a usability question inline.

Marcus Glocker <[hidden email]> 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?

Thanks
Greg

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Marcus Glocker
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 <[hidden email]> 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


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);

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Mark Kettenis
> Date: Mon, 5 Apr 2021 23:15:23 +0200
> From: Marcus Glocker <[hidden email]>
>
> 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 <[hidden email]> 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.

> 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);
>
>

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Patrick Wildt-3
Am Mon, Apr 05, 2021 at 11:19:02PM +0200 schrieb Mark Kettenis:

> > Date: Mon, 5 Apr 2021 23:15:23 +0200
> > From: Marcus Glocker <[hidden email]>
> >
> > 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 <[hidden email]> 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.

Agreed.  If something like that was wanted, I'd rather have uvideo(4)
attach, but print that it's not supported, and then not attach video(4)
to it.

>
> > 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);
> >
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Mark Kettenis
In reply to this post by Mark Kettenis
> Date: Mon, 5 Apr 2021 23:19:02 +0200 (CEST)
> From: Mark Kettenis <[hidden email]>
>
> > Date: Mon, 5 Apr 2021 23:15:23 +0200
> > From: Marcus Glocker <[hidden email]>
> >
> > 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 <[hidden email]> 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);
> >
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

Marcus Glocker
On Mon, Apr 05, 2021 at 11:27:10PM +0200, Mark Kettenis wrote:

[...]

> > > > 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.

Right.  Despite the print line, maybe this way is anyway better than
doing this check in the match function.


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:51:11 -0000
@@ -490,9 +490,6 @@ uvideo_match(struct device *parent, void
  /* quirk devices */
  quirk = uvideo_lookup(uaa->vendor, uaa->product);
  if (quirk != NULL) {
- if (quirk->flags & UVIDEO_FLAG_NOATTACH)
- return (UMATCH_NONE);
-
  if (quirk->flags & UVIDEO_FLAG_REATTACH)
  return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
 
@@ -577,6 +574,11 @@ uvideo_attach(struct device *parent, str
 
  /* maybe the device has quirks */
  sc->sc_quirk = uvideo_lookup(uaa->vendor, uaa->product);
+
+ if (sc->sc_quirk && sc->sc_quirk->flags & UVIDEO_FLAG_NOATTACH) {
+ printf("%s: device not supported\n", DEVNAME(sc));
+ return;
+ }
 
  if (sc->sc_quirk && sc->sc_quirk->ucode_name)
  config_mountroot(self, uvideo_attach_hook);