uapi: bcm2835-isp: Add colour denoise mode config

[naushir]

Add a field to the bcm2835_isp_denoise config structure to control
colour denoise operating modes.

Signed-off-by: Naushir Patuck naush@raspberrypi.com

[naushir]

@davidplowman and @6by9

Not ready to merge just yet though.

[6by9]

Admittedly the uapi hasn't been officially released, but adding extra fields like this does cause problems of backwards compatibility.

What's it actually controlling? This generally maps directly onto the hardware, but I don't see any differentiation of modes there.

[naushir]

Admittedly the uapi hasn't been officially released, but adding extra fields like this does cause problems of backwards compatibility.

Yes, this is a bit of a problem. I feel that we should be able to get away with it in this case given that we have not upstreamed yet, and libcamera is not yet had a stable release. But if you do feel that this could cause problems, we may have to do alternative things.

What's it actually controlling? This generally maps directly onto the hardware, but I don't see any differentiation of modes there.

There are accompanying firmware changes in the works that will make things clearer.

[naushir]

@6by9 and @davidplowman, can I get a review please?

[davidplowman]

Hi Naush, this thing is showing up with 766 files changed and 481 commits, making me a bit scared of trying to review anything. Is there a way to see just the colour denoise changes? (Surely everything else was merged ages ago??)

[6by9]

Dom has rebased the rpi-5.10.y parent branch, so naush needs fetch and rebase.
Or just look at the last one or two commits in the history.

[naushir]

Dom has rebased the rpi-5.10.y parent branch, so naush needs fetch and rebase.
Or just look at the last one or two commits in the history.

Done.

[davidplowman]

Hi Naush, I had just a couple of minor comments to consider:

1. If I interpreted the diffs correctly, did MMAL_PARAMETER_CDN seem to come in the middle of the list of enums rather than at the end? That made me slightly nervous as someone updating their firmware would have to be sure to update their linux (I suppose they would) and recompile their application (again, I expect they would). So maybe not a big deal.
2. I wondered ever so slightly about adding a strength parameter into the mmal_parameter_colour_denoise structure, as there is a possibility we might need it in future (we would hard-code a single value there for now). Or do you think it's better to wait and see on that? I'm not sure, perhaps we should leave it until we know.

[naushir]

1. If I interpreted the diffs correctly, did MMAL_PARAMETER_CDN seem to come in the middle of the list of enums rather than at the end? That made me slightly nervous as someone updating their firmware would have to be sure to update their linux (I suppose they would) and recompile their application (again, I expect they would). So maybe not a big deal.

Yes, this is indeed the case here. I did expect users would have to simultaneously update FW, kernel, and libcamera once this change went through. If that does seem too restrictive, I could put this at the end of the enum.

2. I wondered ever so slightly about adding a strength parameter into the mmal_parameter_colour_denoise structure, as there is a possibility we might need it in future (we would hard-code a single value there for now). Or do you think it's better to wait and see on that? I'm not sure, perhaps we should leave it until we know.

I could put in a strength field as a placeholder, and set the value to a constant through libcamera for now.

[naushir]

`

1. I wondered ever so slightly about adding a strength parameter into the mmal_parameter_colour_denoise structure, as there is a possibility we might need it in future (we would hard-code a single value there for now). Or do you think it's better to wait and see on that? I'm not sure, perhaps we should leave it until we know.

I could put in a strength field as a placeholder, and set the value to a constant through libcamera for now.

One question on this one - should I make it a rational value like the SDN strength?

[davidplowman]

Maybe it should just be a low/medium/strong thing? Which is probably not a bad reason for leaving it until such time as we have good reason for it...!

[naushir]

Maybe it should just be a low/medium/strong thing? Which is probably not a bad reason for leaving it until such time as we have good reason for it...!

Agreed, I won't make this change.

[davidplowman]

Have an "LGTM" from me. I'll leave to it you if you want to change the enum order, I agree that in reality it will probably matter little.

[naushir]

Latest version has the enum value at the end.