Re: [PATCH v5 09/12] media: microchip-isc: add SAMA7G5 hue and saturation controls

[Balakrishnan.S]

Hi Eugen,

On 29/05/26 8:50 pm, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 5/27/26 14:07, Balakrishnan Sambath wrote:
>> The CBHS (Contrast/Brightness/Hue/Saturation) block on SAMA7G5
>> operates in YCbCr space; expose hue and saturation as V4L2 controls
>> for the YUV/RGB output paths only. The SAMA5D2 has only the CBC
>> block (no hue/saturation), so the controls are gated on a new
>> has_cbhs flag.
>>
>> Saturation uses the Q4 fixed-point range 0..127 with default 16
>> (1.0x) directly matching the CBHS_SAT register field. The control
>> state is initialised to neutral at probe so the first config_cbc()
>> write after streaming starts does not produce a grayscale image.
>>
>> Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@xxxxxxxxxxxxx>
>> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@xxxxxxxxxxxxx>
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
>> ---
>>   .../media/platform/microchip/microchip-isc-base.c  | 75 ++++++++++++++++++++++
>>   .../media/platform/microchip/microchip-isc-regs.h  | 11 ++--
>>   drivers/media/platform/microchip/microchip-isc.h   |  3 +
>>   .../platform/microchip/microchip-sama7g5-isc.c     |  6 +-
>>   4 files changed, 88 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
>> index 04187127070d..cb338133d03f 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-base.c
>> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
>> @@ -859,6 +859,56 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
>>        return 0;
>>   }
>>
>> +static bool isc_format_has_chroma(u32 fourcc)
>> +{
>> +     switch (fourcc) {
>> +     case V4L2_PIX_FMT_YUV420:
>> +     case V4L2_PIX_FMT_YUV422P:
>> +     case V4L2_PIX_FMT_YUYV:
>> +     case V4L2_PIX_FMT_UYVY:
>> +     case V4L2_PIX_FMT_VYUY:
>> +             return true;
> RGB565 doesn't have chroma for example ?

Good point. CBHS sits after CSC in the pipeline, so it only
applies to YUV outputs. The hardware cannot do hue/saturation on
the RGB path. Filtering itself is correct, the name "has_chroma"
is misleading though. Will rename to isc_format_is_yuv() if it's
okay ?

> 
>> +     default:
>> +             return false;
>> +     }
>> +}
>> +
>> +/*
>> + * isc_update_cbc_ctrl_activity() - Activate/deactivate CBC controls
> 
> What is the purpose of 'activity' in the name ? is a bit misleading for me.

Agreed. Will rename to isc_update_cbhs_ctrls() to match the existing
isc_update_awb_ctrls() in next version.

>> + *
>> + * Called from isc_set_fmt(), isc_link_validate(), and isc_ctrl_init().
>> + * At isc_ctrl_init() time isc->config.bits_pipeline is zero (no format
>> + * has been negotiated yet), so all CBC controls are initially marked
>> + * inactive.  They become active once a format that includes CBHS in the
>> + * pipeline is configured via VIDIOC_S_FMT or link validation.
>> + */
>> +static void isc_update_cbc_ctrl_activity(struct isc_device *isc)
>> +{
>> +     struct v4l2_ctrl_handler *hdl = &isc->ctrls.handler;
>> +     struct v4l2_ctrl *brightness;
>> +     struct v4l2_ctrl *contrast;
>> +     struct v4l2_ctrl *hue;
>> +     struct v4l2_ctrl *saturation;
>> +     bool cbc_active = isc->config.bits_pipeline & CBHS_ENABLE;
>> +     bool chroma_active = cbc_active && isc_format_has_chroma(isc->config.fourcc);
>> +
>> +     brightness = v4l2_ctrl_find(hdl, V4L2_CID_BRIGHTNESS);
>> +     if (brightness)
>> +             v4l2_ctrl_activate(brightness, cbc_active);
>> +
>> +     contrast = v4l2_ctrl_find(hdl, V4L2_CID_CONTRAST);
>> +     if (contrast)
>> +             v4l2_ctrl_activate(contrast, cbc_active);
>> +
>> +     hue = v4l2_ctrl_find(hdl, V4L2_CID_HUE);
>> +     if (hue)
>> +             v4l2_ctrl_activate(hue, chroma_active);
>> +
>> +     saturation = v4l2_ctrl_find(hdl, V4L2_CID_SATURATION);
>> +     if (saturation)
>> +             v4l2_ctrl_activate(saturation, chroma_active);
> 
> Usage of v4l2_ctrl_find is a bit odd, don't you have the ctrls already
> in the isc struct ? e.g. isc->ctrls.brightness, etc.

You are right, I missed it. AWB controls already have direct
pointers, CBHS ones (brightness, contrast, hue, saturation) don't.
I went with the existing brightness/contrast pattern when adding
hue and saturation. Will add direct pointers for all four of them and 
use it here.

> 
>> +}
>> +
>>   static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
>>   {
>>        struct v4l2_pix_format *pixfmt = &f->fmt.pix;
>> @@ -902,6 +952,7 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>>        /* make the try configuration active */
>>        isc->config = isc->try_config;
>>        isc->fmt = isc->try_fmt;
>> +     isc_update_cbc_ctrl_activity(isc);
>>
>>        dev_dbg(isc->dev, "ISC set_fmt to %.4s @%dx%d\n",
>>                (char *)&f->fmt.pix.pixelformat,
>> @@ -989,6 +1040,7 @@ static int isc_link_validate(struct media_link *link)
>>                return ret;
>>
>>        isc->config = isc->try_config;
>> +     isc_update_cbc_ctrl_activity(isc);
>>
>>        dev_dbg(isc->dev, "New ISC configuration in place\n");
>>
>> @@ -1457,6 +1509,14 @@ static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
>>        case V4L2_CID_CONTRAST:
>>                ctrls->contrast = ctrl->val & ISC_CBC_CONTRAST_MASK;
>>                break;
>> +     case V4L2_CID_HUE:
>> +             if (isc->has_cbhs)
>> +                     ctrls->hue = ctrl->val & ISC_CBHS_HUE_MASK;
>> +             break;
>> +     case V4L2_CID_SATURATION:
>> +             if (isc->has_cbhs)
>> +                     ctrls->saturation = ctrl->val & ISC_CBHS_SAT_MASK;
>> +             break;
>>        case V4L2_CID_GAMMA:
>>                ctrls->gamma_index = ctrl->val;
>>                break;
>> @@ -1464,6 +1524,7 @@ static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
>>                return -EINVAL;
>>        }
>>
>> +     /* config_cbc() flushes ctrls to hardware at stream start. */
> 
> What's the meaning of this comment here . s_ctrl configures multiple
> controls, including gamma, etc, so why mention config_cbc() ?

In fact looks stale for me too will fix it in next version.
> 
>>        return 0;
>>   }
>>
>> @@ -1647,6 +1708,19 @@ static int isc_ctrl_init(struct isc_device *isc)
>>        ctrls->brightness = 0;
>>
>>        v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
>> +     if (isc->has_cbhs) {
>> +             /*
>> +              * CBHS_HUE is a signed 9-bit value in degrees.
>> +              * CBHS_SAT is Q4 unsigned 7-bit, 16 = 1.0x.
>> +              * Initialize the kernel-side state to neutral here so the
>> +              * first config_cbc() call after streaming starts does not
>> +              * write zero (grayscale) to the hardware.
>> +              */
>> +             ctrls->hue = 0;
>> +             ctrls->saturation = 16;
>> +             v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HUE, -180, 180, 1, 0);
>> +             v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION, 0, 127, 1, 16);
>> +     }
>>        v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, isc->gamma_max, 1,
>>                          isc->gamma_default);
>>        isc->awb_ctrl = v4l2_ctrl_new_std(hdl, &isc_awb_ops,
>> @@ -1665,6 +1739,7 @@ static int isc_ctrl_init(struct isc_device *isc)
>>        }
>>
>>        v4l2_ctrl_activate(isc->do_wb_ctrl, false);
>> +     isc_update_cbc_ctrl_activity(isc);
>>
>>        isc->r_gain_ctrl = v4l2_ctrl_new_custom(hdl, &isc_r_gain_ctrl, NULL);
>>        isc->b_gain_ctrl = v4l2_ctrl_new_custom(hdl, &isc_b_gain_ctrl, NULL);
>> diff --git a/drivers/media/platform/microchip/microchip-isc-regs.h b/drivers/media/platform/microchip/microchip-isc-regs.h
>> index e77e1d9a1db8..7f5c2e50e74b 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-regs.h
>> +++ b/drivers/media/platform/microchip/microchip-isc-regs.h
>> @@ -268,10 +268,13 @@
>>   #define ISC_CBC_CONTRAST     0x000003c0
>>   #define ISC_CBC_CONTRAST_MASK        GENMASK(11, 0)
>>
>> -/* Hue Register */
>> -#define ISC_CBCHS_HUE        0x4e0
>> -/* Saturation Register */
>> -#define ISC_CBCHS_SAT        0x4e4
>> +/* Hue Register: signed 9-bit two's complement, covers -180 to +180 degrees */
>> +#define ISC_CBHS_HUE         0x4e0
>> +#define ISC_CBHS_HUE_MASK    GENMASK(8, 0)
>> +
>> +/* Saturation Register: unsigned Q4 fixed-point (1.0 = 16, V4L2 range 0..127) */
>> +#define ISC_CBHS_SAT         0x4e4
>> +#define ISC_CBHS_SAT_MASK    GENMASK(6, 0)
>>
>>   /* Offset for SUB422 register specific to sama5d2 product */
>>   #define ISC_SAMA5D2_SUB422_OFFSET    0
>> diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
>> index 2282ef7dd596..36a9c0cb241f 100644
>> --- a/drivers/media/platform/microchip/microchip-isc.h
>> +++ b/drivers/media/platform/microchip/microchip-isc.h
>> @@ -139,6 +139,8 @@ struct isc_ctrls {
>>
>>        u32 brightness;
>>        u32 contrast;
>> +     u32 hue;
>> +     u32 saturation;
>>        u8 gamma_index;
>>   #define ISC_WB_NONE  0
>>   #define ISC_WB_AUTO  1
>> @@ -343,6 +345,7 @@ struct isc_device {
>>        const u32       (*gamma_table)[GAMMA_ENTRIES];
>>        u32             gamma_max;
>>        u32             gamma_default;
>> +     bool            has_cbhs;
>>
>>        u32             max_width;
>>        u32             max_height;
>> diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
>> index 06aa801b88f9..f51c7cac25df 100644
>> --- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
>> +++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
>> @@ -257,9 +257,8 @@ static void isc_sama7g5_config_cbc(struct isc_device *isc)
>>        /* Configure what is set via v4l2 ctrls */
>>        regmap_write(regmap, ISC_CBC_BRIGHT + isc->offsets.cbc, isc->ctrls.brightness);
>>        regmap_write(regmap, ISC_CBC_CONTRAST + isc->offsets.cbc, isc->ctrls.contrast);
>> -     /* Configure Hue and Saturation as neutral midpoint */
>> -     regmap_write(regmap, ISC_CBCHS_HUE, 0);
>> -     regmap_write(regmap, ISC_CBCHS_SAT, (1 << 4));
>> +     regmap_write(regmap, ISC_CBHS_HUE, isc->ctrls.hue);
>> +     regmap_write(regmap, ISC_CBHS_SAT, isc->ctrls.saturation);
>>   }
>>
>>   static void isc_sama7g5_config_cc(struct isc_device *isc)
>> @@ -463,6 +462,7 @@ static int microchip_xisc_probe(struct platform_device *pdev)
>>        isc->gamma_max = 2;
>>        /* Index 1 in the SAMA7G5 table is gamma 1/2.2 (sRGB). */
>>        isc->gamma_default = 1;
>> +     isc->has_cbhs = true;
>>
>>        if (of_machine_is_compatible("microchip,sam9x7")) {
>>                isc->max_width = ISC_SAM9X7_MAX_SUPPORT_WIDTH;
>>
>