Re: [PATCH v2 11/15] media: microchip-isc: add per-channel gamma LUT controls

[Balakrishnan.S]

Hi Sakari,

Thanks for the feedback.

On 15/05/26 3:56 pm, Sakari Ailus wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Balakrishnan,
> 
> Thanks for the set.
> 
> On Tue, May 12, 2026 at 09:13:35PM +0530, Balakrishnan Sambath wrote:
>> Add 64-entry gamma LUT controls for R/G/B channels. Setting any LUT
>> overrides V4L2_CID_GAMMA; writing V4L2_CID_GAMMA restores presets.
>> Supports SAMA7G5 bipartite encoding.
> 
> I'd say hardware this complicated should use parameter buffers like other
> similar hardware does. The reasoning is that the userspace gets an
> interface that allows passing all relevant parameters to the driver at one
> go, with a reasonable expectation on which frame it can be applied to.
> Controls provide neither of these.

I exposed the gamma LUT and CC matrix as v4l2 controls to keep the
driver usable from existing v4l2-ctl based flows without introducing
a new interface initially. However I agree this is not the right fit
for LUT tables and matrices.

I went back to the datasheet and the ISC has a profile update
mechanism (ISC_CTRLEN.UPPRO, datasheet section 43.6.6.1) where the
configuration registers are double buffered and latched on the next
vsync. The parameter buffer interface is a good match for this
hardware. I actually overlooked this aspect. Thanks for pointing this out.

> 
> On top of this, the control framework, while useful for high level
> configuration parameters, introduces quite a bit of validation overhead
> that can affect performance while the effect of a bad parameter value is
> simply bad output and the software setting these values is purpose-built so
> it can already take the ranges and steps into account.

Agreed.

For now, for v4 I will drop the gamma LUT and CC matrix controls patches 
and keep the remYeaining patches in the series as it is.

I'll resubmit the gamma LUT and CC matrix as a follow up series
adding a parameter buffer node, latched at vsync via UPPRO.


> 
> --
> Kind regards,
> 
> Sakari Ailus
-- 
Best regards,
Balakrishnan S