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

Hi Dan,

On Wed, Jul 31, 2024 at 04:12:41PM +0100, Daniel Scally wrote:

On 30/07/2024 23:16, Laurent Pinchart wrote:

quoted

On Tue, Jul 09, 2024 at 02:29:05PM +0100, Daniel Scally wrote:

quoted

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  <redacted>
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]

quoted

quoted

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.

quoted

quoted

+	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