* [PATCH] media: atomisp: remove private white balance IOCTLs @ 2025-12-31 5:24 Karthikey D Kadati 2026-01-12 11:00 ` Sakari Ailus 0 siblings, 1 reply; 3+ messages in thread From: Karthikey D Kadati @ 2025-12-31 5:24 UTC (permalink / raw) To: Hans de Goede, Mauro Carvalho Chehab Cc: Sakari Ailus, Andy Shevchenko, Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel, Karthikey D Kadati This patch resolves a MUST-FIX graduation blocker identified in the atomisp TODO by removing the private ATOMISP_IOC_G_ISP_WHITE_BALANCE and ATOMISP_IOC_S_ISP_WHITE_BALANCE and replacing them with standard V4L2 control handling. The private IOCTLs were used to set white balance parameters. This functionality is now mapped to the standard V4L2 controls V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE. A helper function `atomisp_v4l2_set_wb` is introduced to translate the V4L2 control values to the driver's internal configuration format. Signed-off-by: Karthikey D Kadati <karthikey3608@gmail.com> --- .../media/atomisp/include/linux/atomisp.h | 5 +- .../staging/media/atomisp/pci/atomisp_ioctl.c | 49 ++++++++++++++++--- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h index 3c8fa3f58..fcf116cc4 100644 --- a/drivers/staging/media/atomisp/include/linux/atomisp.h +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h @@ -741,10 +741,7 @@ enum atomisp_burst_capture_options { _IOW('v', BASE_VIDIOC_PRIVATE + 15, struct atomisp_ctc_table) /* white balance Correction */ -#define ATOMISP_IOC_G_ISP_WHITE_BALANCE \ - _IOR('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config) -#define ATOMISP_IOC_S_ISP_WHITE_BALANCE \ - _IOW('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config) + /* fpn table loading */ #define ATOMISP_IOC_S_ISP_FPN_TABLE \ diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c index bb8b2f221..5c0a1d92b 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c @@ -1083,6 +1083,38 @@ static int atomisp_g_ctrl(struct file *file, void *fh, * applications initialize the id and value fields of a struct v4l2_control * and call this ioctl. */ +static int atomisp_v4l2_set_wb(struct atomisp_sub_device *asd, int id, + int value) +{ + struct atomisp_device *isp = asd->isp; + struct atomisp_wb_config config; + int ret; + + if (atomisp_css_get_wb_config(asd, &config)) { + dev_err(isp->dev, "%s: can't get wb config\n", __func__); + return -EINVAL; + } + + switch (id) { + case V4L2_CID_BLUE_BALANCE: + config.b = value << (16 - 8 - config.integer_bits + 1); + break; + case V4L2_CID_RED_BALANCE: + config.r = value << (16 - 8 - config.integer_bits + 1); + break; + default: + return -EINVAL; + } + + ret = atomisp_white_balance_param(asd, 1, &config); + if (ret) { + dev_err(isp->dev, "%s: set wb config failed\n", __func__); + return ret; + } + + return 0; +} + static int atomisp_s_ctrl(struct file *file, void *fh, struct v4l2_control *control) { @@ -1122,6 +1154,17 @@ static int atomisp_s_ctrl(struct file *file, void *fh, case V4L2_CID_ATOMISP_LOW_LIGHT: ret = atomisp_low_light(asd, 1, &control->value); break; + case V4L2_CID_AUTO_WHITE_BALANCE: + /* + * TODO: Auto White Balance is not supported yet. + * It is currently handled by the ISP. + */ + ret = 0; + break; + case V4L2_CID_RED_BALANCE: + case V4L2_CID_BLUE_BALANCE: + ret = atomisp_v4l2_set_wb(asd, control->id, control->value); + break; default: ret = -EINVAL; break; @@ -1484,13 +1527,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh, err = atomisp_ctc(asd, 1, arg); break; - case ATOMISP_IOC_G_ISP_WHITE_BALANCE: - err = atomisp_white_balance_param(asd, 0, arg); - break; - case ATOMISP_IOC_S_ISP_WHITE_BALANCE: - err = atomisp_white_balance_param(asd, 1, arg); - break; case ATOMISP_IOC_G_3A_CONFIG: err = atomisp_3a_config_param(asd, 0, arg); -- 2.43.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] media: atomisp: remove private white balance IOCTLs 2025-12-31 5:24 [PATCH] media: atomisp: remove private white balance IOCTLs Karthikey D Kadati @ 2026-01-12 11:00 ` Sakari Ailus 2026-01-14 17:37 ` Karthikey Kadati 0 siblings, 1 reply; 3+ messages in thread From: Sakari Ailus @ 2026-01-12 11:00 UTC (permalink / raw) To: Karthikey D Kadati Cc: Hans de Goede, Mauro Carvalho Chehab, Andy Shevchenko, Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel Hi Karthikey, On Wed, Dec 31, 2025 at 10:54:27AM +0530, Karthikey D Kadati wrote: > This patch resolves a MUST-FIX graduation blocker identified in the > atomisp TODO by removing the private ATOMISP_IOC_G_ISP_WHITE_BALANCE > and ATOMISP_IOC_S_ISP_WHITE_BALANCE and replacing them with standard > V4L2 control handling. > > The private IOCTLs were used to set white balance parameters. This > functionality is now mapped to the standard V4L2 controls > V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE. > > A helper function `atomisp_v4l2_set_wb` is introduced to translate the > V4L2 control values to the driver's internal configuration format. > > Signed-off-by: Karthikey D Kadati <karthikey3608@gmail.com> > --- > .../media/atomisp/include/linux/atomisp.h | 5 +- > .../staging/media/atomisp/pci/atomisp_ioctl.c | 49 ++++++++++++++++--- > 2 files changed, 44 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h > index 3c8fa3f58..fcf116cc4 100644 > --- a/drivers/staging/media/atomisp/include/linux/atomisp.h > +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h > @@ -741,10 +741,7 @@ enum atomisp_burst_capture_options { > _IOW('v', BASE_VIDIOC_PRIVATE + 15, struct atomisp_ctc_table) > > /* white balance Correction */ > -#define ATOMISP_IOC_G_ISP_WHITE_BALANCE \ > - _IOR('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config) > -#define ATOMISP_IOC_S_ISP_WHITE_BALANCE \ > - _IOW('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config) > + > > /* fpn table loading */ > #define ATOMISP_IOC_S_ISP_FPN_TABLE \ > diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > index bb8b2f221..5c0a1d92b 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > @@ -1083,6 +1083,38 @@ static int atomisp_g_ctrl(struct file *file, void *fh, > * applications initialize the id and value fields of a struct v4l2_control > * and call this ioctl. > */ > +static int atomisp_v4l2_set_wb(struct atomisp_sub_device *asd, int id, > + int value) > +{ > + struct atomisp_device *isp = asd->isp; > + struct atomisp_wb_config config; > + int ret; > + > + if (atomisp_css_get_wb_config(asd, &config)) { I'm not sure this makes sense. How will the caller know the value of integer_bits? I'd think the IOCTL interface of this driver should probably be largely removed, to be replaced by the parameter buffer. > + dev_err(isp->dev, "%s: can't get wb config\n", __func__); > + return -EINVAL; > + } > + > + switch (id) { > + case V4L2_CID_BLUE_BALANCE: > + config.b = value << (16 - 8 - config.integer_bits + 1); > + break; > + case V4L2_CID_RED_BALANCE: > + config.r = value << (16 - 8 - config.integer_bits + 1); > + break; > + default: > + return -EINVAL; > + } > + > + ret = atomisp_white_balance_param(asd, 1, &config); > + if (ret) { > + dev_err(isp->dev, "%s: set wb config failed\n", __func__); > + return ret; > + } > + > + return 0; > +} > + > static int atomisp_s_ctrl(struct file *file, void *fh, > struct v4l2_control *control) > { > @@ -1122,6 +1154,17 @@ static int atomisp_s_ctrl(struct file *file, void *fh, > case V4L2_CID_ATOMISP_LOW_LIGHT: > ret = atomisp_low_light(asd, 1, &control->value); > break; > + case V4L2_CID_AUTO_WHITE_BALANCE: > + /* > + * TODO: Auto White Balance is not supported yet. > + * It is currently handled by the ISP. > + */ > + ret = 0; > + break; > + case V4L2_CID_RED_BALANCE: > + case V4L2_CID_BLUE_BALANCE: > + ret = atomisp_v4l2_set_wb(asd, control->id, control->value); > + break; > default: > ret = -EINVAL; > break; > @@ -1484,13 +1527,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh, > err = atomisp_ctc(asd, 1, arg); > break; > > - case ATOMISP_IOC_G_ISP_WHITE_BALANCE: > - err = atomisp_white_balance_param(asd, 0, arg); > - break; > > - case ATOMISP_IOC_S_ISP_WHITE_BALANCE: > - err = atomisp_white_balance_param(asd, 1, arg); > - break; > > case ATOMISP_IOC_G_3A_CONFIG: > err = atomisp_3a_config_param(asd, 0, arg); -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: atomisp: remove private white balance IOCTLs 2026-01-12 11:00 ` Sakari Ailus @ 2026-01-14 17:37 ` Karthikey Kadati 0 siblings, 0 replies; 3+ messages in thread From: Karthikey Kadati @ 2026-01-14 17:37 UTC (permalink / raw) To: Sakari Ailus Cc: Hans de Goede, Mauro Carvalho Chehab, Andy Shevchenko, Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel Hi Sakari, You are absolutely right. I investigated the code and confirmed that `integer_bits` in `atomisp_wb_config` is essential for the hardware's gain calculation (shift logic) and varies by configuration. Mapping this to standard V4L2 controls (like `V4L2_CID_RED_BALANCE`) would mask this parameter, making the interface unusable or forcing incorrect assumptions. I also see that `ATOMISP_IOC_S_PARAMETERS` already exists and accepts `struct atomisp_parameters`, which includes `atomisp_wb_config`. This appears to be the "parameter buffer" mechanism you referred to, which correctly handles the full configuration context. Attempting to convert these specific private IOCTLs to standard controls was the wrong approach, as they seem to be redundant legacy interfaces better served by the existing parameter buffer. I will drop this patch from the series. Thanks, Karthikey On Mon, 12 Jan 2026 at 16:30, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Karthikey, > > On Wed, Dec 31, 2025 at 10:54:27AM +0530, Karthikey D Kadati wrote: > > This patch resolves a MUST-FIX graduation blocker identified in the > > atomisp TODO by removing the private ATOMISP_IOC_G_ISP_WHITE_BALANCE > > and ATOMISP_IOC_S_ISP_WHITE_BALANCE and replacing them with standard > > V4L2 control handling. > > > > The private IOCTLs were used to set white balance parameters. This > > functionality is now mapped to the standard V4L2 controls > > V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE. > > > > A helper function `atomisp_v4l2_set_wb` is introduced to translate the > > V4L2 control values to the driver's internal configuration format. > > > > Signed-off-by: Karthikey D Kadati <karthikey3608@gmail.com> > > --- > > .../media/atomisp/include/linux/atomisp.h | 5 +- > > .../staging/media/atomisp/pci/atomisp_ioctl.c | 49 ++++++++++++++++--- > > 2 files changed, 44 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h > > index 3c8fa3f58..fcf116cc4 100644 > > --- a/drivers/staging/media/atomisp/include/linux/atomisp.h > > +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h > > @@ -741,10 +741,7 @@ enum atomisp_burst_capture_options { > > _IOW('v', BASE_VIDIOC_PRIVATE + 15, struct atomisp_ctc_table) > > > > /* white balance Correction */ > > -#define ATOMISP_IOC_G_ISP_WHITE_BALANCE \ > > - _IOR('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config) > > -#define ATOMISP_IOC_S_ISP_WHITE_BALANCE \ > > - _IOW('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config) > > + > > > > /* fpn table loading */ > > #define ATOMISP_IOC_S_ISP_FPN_TABLE \ > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > > index bb8b2f221..5c0a1d92b 100644 > > --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > > +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c > > @@ -1083,6 +1083,38 @@ static int atomisp_g_ctrl(struct file *file, void *fh, > > * applications initialize the id and value fields of a struct v4l2_control > > * and call this ioctl. > > */ > > +static int atomisp_v4l2_set_wb(struct atomisp_sub_device *asd, int id, > > + int value) > > +{ > > + struct atomisp_device *isp = asd->isp; > > + struct atomisp_wb_config config; > > + int ret; > > + > > + if (atomisp_css_get_wb_config(asd, &config)) { > > I'm not sure this makes sense. How will the caller know the value of > integer_bits? > > I'd think the IOCTL interface of this driver should probably be largely > removed, to be replaced by the parameter buffer. > > > + dev_err(isp->dev, "%s: can't get wb config\n", __func__); > > + return -EINVAL; > > + } > > + > > + switch (id) { > > + case V4L2_CID_BLUE_BALANCE: > > + config.b = value << (16 - 8 - config.integer_bits + 1); > > + break; > > + case V4L2_CID_RED_BALANCE: > > + config.r = value << (16 - 8 - config.integer_bits + 1); > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = atomisp_white_balance_param(asd, 1, &config); > > + if (ret) { > > + dev_err(isp->dev, "%s: set wb config failed\n", __func__); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > static int atomisp_s_ctrl(struct file *file, void *fh, > > struct v4l2_control *control) > > { > > @@ -1122,6 +1154,17 @@ static int atomisp_s_ctrl(struct file *file, void *fh, > > case V4L2_CID_ATOMISP_LOW_LIGHT: > > ret = atomisp_low_light(asd, 1, &control->value); > > break; > > + case V4L2_CID_AUTO_WHITE_BALANCE: > > + /* > > + * TODO: Auto White Balance is not supported yet. > > + * It is currently handled by the ISP. > > + */ > > + ret = 0; > > + break; > > + case V4L2_CID_RED_BALANCE: > > + case V4L2_CID_BLUE_BALANCE: > > + ret = atomisp_v4l2_set_wb(asd, control->id, control->value); > > + break; > > default: > > ret = -EINVAL; > > break; > > @@ -1484,13 +1527,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh, > > err = atomisp_ctc(asd, 1, arg); > > break; > > > > - case ATOMISP_IOC_G_ISP_WHITE_BALANCE: > > - err = atomisp_white_balance_param(asd, 0, arg); > > - break; > > > > - case ATOMISP_IOC_S_ISP_WHITE_BALANCE: > > - err = atomisp_white_balance_param(asd, 1, arg); > > - break; > > > > case ATOMISP_IOC_G_3A_CONFIG: > > err = atomisp_3a_config_param(asd, 0, arg); > > -- > Regards, > > Sakari Ailus ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-01-14 17:38 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-31 5:24 [PATCH] media: atomisp: remove private white balance IOCTLs Karthikey D Kadati 2026-01-12 11:00 ` Sakari Ailus 2026-01-14 17:37 ` Karthikey Kadati
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox