Re: [PATCH v6 17/18] media: platform: Add mali-c55 parameters video node

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

On Wed, Jul 31, 2024 at 04:12:41PM +0100, Daniel Scally wrote:
> On 30/07/2024 23:16, Laurent Pinchart wrote:
> > On Tue, Jul 09, 2024 at 02:29:05PM +0100, Daniel Scally wrote:
> >> Add a new code file to the mali-c55 driver that registers an output
> >> video node for userspace to queue buffers of parameters to. Handlers
> >> are included to program the statistics generation plus the white
> >> balance, black level correction and mesh shading correction blocks.
> >>
> >> Update the rest of the driver to register and link the new video node
> >>
> >> Acked-by: Nayden Kanchev  <nayden.kanchev@arm.com>
> >> Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >> Changes in v6:
> >>
> >> 	- Used a union to generalise the block pointer rather than resorting to
> >> 	  casting everywhere - fantastic idea Sakari, this made it much cleaner.
> >> 	- Reworked the loop in mali_c55_params_write_config() so that we can be
> >> 	  sure there's remaining space for the next block header.
> >>
> >> Changes in v5:
> >>
> >> 	- New patch
> >>
> >>   drivers/media/platform/arm/mali-c55/Makefile  |   1 +
> >>   .../platform/arm/mali-c55/mali-c55-common.h   |  20 +
> >>   .../platform/arm/mali-c55/mali-c55-core.c     |  23 +
> >>   .../platform/arm/mali-c55/mali-c55-isp.c      |  21 +-
> >>   .../platform/arm/mali-c55/mali-c55-params.c   | 671 ++++++++++++++++++
> >>   .../arm/mali-c55/mali-c55-registers.h         | 128 ++++
> >>   6 files changed, 863 insertions(+), 1 deletion(-)
> >>   create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-params.c

[snip]

> >> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> >> index ed0db34767a4..55b3cbf53791 100644
> >> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> >> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> >> @@ -384,6 +384,16 @@ static int mali_c55_create_links(struct mali_c55 *mali_c55)
> >>   		goto err_remove_links;
> >>   	}
> >>   
> >> +	ret = media_create_pad_link(&mali_c55->params.vdev.entity, 0,
> >> +				    &mali_c55->isp.sd.entity,
> >> +				    MALI_C55_ISP_PAD_SINK_PARAMS,
> >> +				    MEDIA_LNK_FL_ENABLED);
> > Should this be immutable, or do you think it makes sense to support
> > operating the ISP without parameters ? I know we did so when developing
> > the driver to test the initial code, but are there real use cases now ?
> 
> If all you're interested in is RAW data bypassed through the ISP then
> you could skip the parameters and statistics - which would be the case
> for RGB/YUV cameras feeding the ISP too. Is that sufficient use-case
> to leave them mutable?

I suppose that's a good point, yes. Fine with me.

> >> +	if (ret) {
> >> +		dev_err(mali_c55->dev,
> >> +			"failed to link ISP and parameters video node\n");
> >> +		goto err_remove_links;
> >> +	}
> >> +
> >>   	return 0;
> >>   
> >>   err_remove_links:

[snip]

-- 
Regards,

Laurent Pinchart