Hi Jacopo, On Mon, Aug 10, 2020 at 09:28:14AM +0200, Jacopo Mondi wrote: > On Sun, Aug 09, 2020 at 06:53:11PM +0300, Laurent Pinchart wrote: > > On Wed, Aug 05, 2020 at 12:57:17PM +0200, Jacopo Mondi wrote: > > > Subject: [PATCH 0/4] media: docs: Document pixel array properties > > > > > > Hans' patch "[PATCH] imx219: selection compliance fixes" sparkled a discussion > > > on how the V4L2 selection targets have to be used in order to access an > > > image sensor pixel array properties. > > > > > > The discussion shown how much under-specified that part was, so this is > > > an attempt to provide a bit documentation for this. > > > > > > My feeling is that we're hijacking the existing targets for this use case > > > and we should probably define new ones, considering how few users we have in > > > mainline of them at the moment. > > > > Do you mean you think we should create new targets instead of reusing > > the existing ones as you do in this series ? I don't see anything really > > wrong in reusing the existing targets, provided we document them > > properly, and it's not too much of a hack (that is, the documented > > purpose reasonably matches the target name), but maybe I'm missing the > > issue. > > Yes, that's what I meant. > > To me, if you have something and you need to document it with "if > applied to X it means Y, if applied to Z it means W etcetc" feels like > we're abusing it. I don't really agree, I think this is common, and I don't really see a problem. Lots of V4L2 ioctls (or the structures they use) document how they apply to capture and output devices for instance, and I don't think that splitting ioctls into _CAPTURE and _OUTPUT variants would help much. If you were using a COMPOSE rectangle to specify a crop operation when used with camera sensors it would of course be an API abuse, but as long as CROP is crop, I see nothing wrong in differences in details of how the rectangles apply to different types of devices. > Having sensor's specific targets would make clear > what a target represents without the ambiguities of defining the > symbol semantic depending on the device it applies to (which means > userspace would need to know what kind of device it's dealing with > precisely). Userspace would always need that, in order to pick the applicable target. > That said, my preference would be using a different API, but see my > reply to your other email for that. > > > > On top Hans' patch with reworded commit message and minor updates. > > > > > > For reference, we're using the V4L2 selection targets in libcamera to retrieve > > > the sensor pixel array properties to be reported to applications for > > > calibration purposes. The currently defined pixel properties for libcamera > > > are available here: > > > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/property_ids.yaml#n390 > > > > > > Thanks > > > j > > > > > > Hans Verkuil (1): > > > media: i2c: imx219: Selection compliance fixes > > > > > > Jacopo Mondi (3): > > > media: docs: Describe pixel array properties > > > media: docs: Describe targets for sensor properties > > > media: docs: USe SUBDEV_G_SELECTION for sensor properties > > > > > > .../userspace-api/media/v4l/dev-subdev.rst | 85 +++++++++++++++++++ > > > .../media/v4l/v4l2-selection-targets.rst | 49 +++++++++++ > > > .../media/v4l/vidioc-subdev-g-selection.rst | 4 + > > > drivers/media/i2c/imx219.c | 17 ++-- > > > 4 files changed, 147 insertions(+), 8 deletions(-) -- Regards, Laurent Pinchart
