Hi Jacopo, On Tue, Dec 01, 2020 at 02:24:45PM +0100, Jacopo Mondi wrote: > Hi Sakari, > > On Tue, Dec 01, 2020 at 02:57:13PM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Tue, Dec 01, 2020 at 10:26:05AM +0100, Jacopo Mondi wrote: > > > Hi Hans, Sakari, > > > > > > On Tue, Nov 10, 2020 at 06:40:31PM +0100, Jacopo Mondi wrote: > > > > Hello, > > > > new iteration following > > > > v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3768 > > > > > > > > Major changelog: > > > > - Use v4l2_dev.release and drop manual ref-counting as suggested by Dafna > > > > - Address Hans' comments on queue setup and metadata format handling function > > > > - s/MEDIA_BUS_FMT_SENSOR_DATA/MEDIA_BUS_FMT_CUSTOM_SENSOR_DATA as suggested by > > > > Hans and rebase on Dafna's fixed metadata format patch > > > > - Add a TODO file to explain why the driver is in staging. > > > > - Conditionally register the unicam-embedded video device node on the presence > > > > of the sensor's metadata source pad. > > > > > > > > The media graph, when connected to a sensor that does not report metadata > > > > looks like: > > > > > > > > Media controller API version 5.10.0 > > > > > > > > Media device information > > > > ------------------------ > > > > driver unicam > > > > model unicam > > > > serial > > > > bus info platform:fe801000.csi > > > > hw revision 0x0 > > > > driver version 5.10.0 > > > > > > > > Device topology > > > > - entity 1: ov5647 10-0036 (1 pad, 1 link) > > > > type V4L2 subdev subtype Sensor flags 0 > > > > device node name /dev/v4l-subdev0 > > > > pad0: Source > > > > [fmt:SBGGR10_1X10/640x480 field:none colorspace:srgb > > > > crop.bounds:(16,16)/2592x1944 > > > > crop:(32,16)/2560x1920] > > > > -> "unicam-image":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 3: unicam-image (1 pad, 1 link) > > > > type Node subtype V4L flags 1 > > > > device node name /dev/video0 > > > > pad0: Sink > > > > <- "ov5647 10-0036":0 [ENABLED,IMMUTABLE] > > > > > > > > > > > > If the sensor reports an additional metadata pad: > > > > > > > > Media controller API version 5.10.0 > > > > > > > > Media device information > > > > ------------------------ > > > > driver unicam > > > > model unicam > > > > serial > > > > bus info platform:fe801000.csi > > > > hw revision 0x0 > > > > driver version 5.10.0 > > > > > > > > Device topology > > > > - entity 1: imx219 10-0010 (2 pads, 2 links) > > > > type V4L2 subdev subtype Sensor flags 0 > > > > device node name /dev/v4l-subdev0 > > > > pad0: Source > > > > [fmt:SRGGB10_1X10/3280x2464 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range > > > > crop:(0,0)/3280x2464] > > > > -> "unicam-image":0 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > [fmt:unknown/16384x1 field:none > > > > crop:(0,0)/3280x2464] > > > > -> "unicam-embedded":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 4: unicam-image (1 pad, 1 link) > > > > type Node subtype V4L flags 1 > > > > device node name /dev/video0 > > > > pad0: Sink > > > > <- "imx219 10-0010":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 10: unicam-embedded (1 pad, 1 link) > > > > type Node subtype V4L flags 0 > > > > device node name /dev/video1 > > > > pad0: Sink > > > > <- "imx219 10-0010":1 [ENABLED,IMMUTABLE] > > > > > > > > Conditionally registering the metadata video node allows to simplify the > > > > code in the driver as well, removing the 'sensor_embedded_data' flag. > > > > > > > > An additional note: this version will break the libcamera pipeline handler > > > > which assume the unicam-embedded video device node to always be there. > > > > > > > > From Dave's reply to Dafna's comments I get instead that for the existing > > > > applications ecosystem, having the metadata node not registered if the sensor > > > > does not support it is not an issue. > > > > > > I think I've closed comments received on v3. > > > > > > Do you think this series is ready for being collected ? > > > > I'll try to look into this later today / this week. > > > > Thanks! > > > The problem with the approach appears, based on a quick glance, to be that > > it creates an additional way (to the more generic approach in VC support > > patchset) to support embedded data, including duplicated sensor driver > > support. That appears as a dead end. But let me look into the details > > first. > > I know :( > > That's why the driver has been moved to staging, and that's > why this last version only conditionally registers the additional video > device for metadata based on the presence of the additional sensor's > source pad. 'Regular' sensor driver will work as usual with this last > version (ie. no additional 'unicam-embedded' video node gets > registered to userspace). > > We could either wait for support for VC to be finalized, but I'm not > that hopeful this will happen any time soon, or alternatively we can Not in matter of weeks at least, but I believe there's more and more interest to get this merged. As I wrote the original set, over the years it became harder and harder to test the patches on the original hardware setup I had due to too many dependencies to unsupported stuff. > merge a version without any support for metadata and have vendors > maintain patches to re-add it. I fear this will limit the adoption of > this driver as they will probably keep using whatever they have in > their BSP (which kind of defeat the purpose of upstreaming it). > > There is one more controversial point: the MC/subdev kAPI duality. > I've tried to outline both issues in a TODO file at the end of this > series. I'll comment on that separately but I think we already do share understanding of the areas that need work. -- Regards, Sakari Ailus
