Re: [PATCH v3 3/5] media: uapi: Add ST VGXY61 header file

From: Benjamin MUGNIER <benjamin.mugnier@foss.st.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: <linux-media@vger.kernel.org>, <alain.volmat@foss.st.com>,
	<hugues.fruchet@foss.st.com>, <sylvain.petinot@foss.st.com>,
	<dave.stevenson@raspberrypi.com>,
	<laurent.pinchart@ideasonboard.com>,
	<kieran.bingham@ideasonboard.com>
Subject: Re: [PATCH v3 3/5] media: uapi: Add ST VGXY61 header file
Date: Fri, 26 Aug 2022 16:17:11 +0200	[thread overview]
Message-ID: <1191003f-2e95-6ce0-7fbf-323a03496359@foss.st.com> (raw)
In-Reply-To: <YwNcUpsw1psudCOC@paasikivi.fi.intel.com>

On 8/22/22 12:37, Sakari Ailus wrote:
> Hi Benjamin,
> 
> On Thu, May 12, 2022 at 09:40:35AM +0200, Benjamin Mugnier wrote:
>> Define an HDR control in it, and adds its documentation in st-vgxy61.rst
>> file.
>>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>   .../userspace-api/media/drivers/st-vgxy61.rst | 23 +++++++++++++++++++
>>   include/uapi/linux/st-vgxy61.h                | 15 ++++++++++++
>>   2 files changed, 38 insertions(+)
>>   create mode 100644 Documentation/userspace-api/media/drivers/st-vgxy61.rst
>>   create mode 100644 include/uapi/linux/st-vgxy61.h
>>
>> diff --git a/Documentation/userspace-api/media/drivers/st-vgxy61.rst b/Documentation/userspace-api/media/drivers/st-vgxy61.rst
>> new file mode 100644
>> index 000000000000..7a11adbb558f
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/drivers/st-vgxy61.rst
>> @@ -0,0 +1,23 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +ST VGXY61 camera sensor driver
>> +==============================
>> +
>> +The ST VGXY61 driver implements the following driver-specific controls:
>> +
>> +``V4L2_CID_STVGXY61_HDR``
>> +-------------------------------
>> +    Change the sensor HDR mode. A HDR picture is obtained by merging two captures of the same scene
>> +    using two different exposure periods.
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       1 4
>> +
>> +    * - HDR linearize
>> +      - The merger outputs a long exposure capture as long as it is not saturated.
>> +    * - HDR substraction
>> +      - This involves subtracting the short exposure frame from the long exposure frame.
>> +    * - "no HDR"
>> +      - This mode is used for standard dynamic range (SDR) exposures.
> 
> I wonder how many different kinds of HDR implementations there are that
> could be meaningfully controlled using a single control. We have controls
> such as scene mode that are much more generic than this.
> 
> Could this be standardised, even if the menu items wouldn't be? Say, call
> it e.g. V4L2_CID_HDR_MODE? Raw sensors have different configuration with
> more parameters though.
> 
> I wonder what others think.
> 

I agree.
I would like to standardize the control and not the menu items just like 
you said, as some sensors are missing some modes and/or have some 
others. This is the way to go.

I will submit something for v4 if nobody is against this idea.

>> diff --git a/include/uapi/linux/st-vgxy61.h b/include/uapi/linux/st-vgxy61.h
>> new file mode 100644
>> index 000000000000..fbabe2cb64ac
>> --- /dev/null
>> +++ b/include/uapi/linux/st-vgxy61.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022 STMicroelectronics SA
>> + *
>> + */
>> +
>> +#ifndef __UAPI_STVGXY61_H_
>> +#define __UAPI_STVGXY61_H_
>> +
>> +#include <linux/v4l2-controls.h>
>> +
>> +/* Control HDR mode */
>> +#define V4L2_CID_STVGXY61_HDR	(V4L2_CID_USER_STVGXY61_BASE + 0)
>> +
>> +#endif /* __UAPI_STVGXY61_H_ */
> 