Re: [PATCH v7 7/8] media: raspberrypi: Add support for PiSP BE

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Jacopo,

On Mon, May 27, 2024 at 09:56:00AM +0200, Jacopo Mondi wrote:
> > > +#include <linux/media/raspberrypi/pisp_be_config.h>
> >
> > Where is the header included from? If it's just this driver, then I'd put
> > it in the driver's directory.
> >
> 
> It's the uAPI header file. Or did I miss your question ?

If it's uapi, then you should have uapi in its header path. I.e.

#include <uapi/linux...>

> > > +	/* Everything else is as supplied by the user. */
> > > +	begin =	offsetof(struct pisp_be_config, global.bayer_order)
> > > +	      / sizeof(u32);
> >
> > The slash should be on the previous line. Same elsewhere.
> >
> 
> Please, this is highly subjective and other people (like Laurent) often
> ask for the contrary. Without any polemic intent, I encourage reviewers
> (myself included) in considering how much time we spend (and
> demand) on such subjective issues. Even more when other reviewers might have
> different opinions, with the end result of pulling contributors in
> different directions.

Having binary operators at the beginning of a statement split on multiple
lines is simply uncommon, perhaps around 10 % of the cases in the media
tree based on a quick look. Keeping the coding style consistent is
beneficial for us all.

...

> > > +static void pispbe_xlate_addrs(dma_addr_t addrs[N_HW_ADDRESSES],
> > > +			       u32 hw_enables[N_HW_ENABLES],
> > > +			       struct pisp_be_tiles_config *config,
> > > +			       struct pispbe_buffer *buf[PISPBE_NUM_NODES],
> > > +			       struct pispbe_node_group *node_group)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Take a copy of the "enable" bitmaps so we can modify them. */
> > > +	hw_enables[0] = config->config.global.bayer_enables;
> > > +	hw_enables[1] = config->config.global.rgb_enables;
> >
> > I wonder if hw_enables would be better declared as a struct.
> 
> struct hw_enable {
>         u32 bayer_enable;
>         u32 rgb_enable;
> };
> 
> ?

You currently  have an array of struct hw_enable here.

> 
> >
> > > +
> > > +	/*
> > > +	 * Main input first. There are 3 address pointers, corresponding to up
> > > +	 * to 3 planes.
> > > +	 */
> > > +	ret = pispbe_get_planes_addr(addrs, buf[MAIN_INPUT_NODE],
> > > +				     &node_group->node[MAIN_INPUT_NODE]);
> > > +	if (ret <= 0) {
> > > +		/*
> > > +		 * This shouldn't happen; pispbe_schedule_internal should insist
> > > +		 * on an input.
> > > +		 */
> > > +		dev_warn(node_group->pispbe->dev, "ISP-BE missing input\n");
> > > +		hw_enables[0] = 0;
> > > +		hw_enables[1] = 0;
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Now TDN/Stitch inputs and outputs. These are single-plane and only
> > > +	 * used with Bayer input. Input enables must match the requirements
> > > +	 * of the processing stages, otherwise the hardware can lock up!
> > > +	 */
> > > +	if (hw_enables[0] & PISP_BE_BAYER_ENABLE_INPUT) {
> > > +		addrs[3] = pispbe_get_addr(buf[TDN_INPUT_NODE]);
> > > +		if (addrs[3] == 0 ||
> > > +		    !(hw_enables[0] & PISP_BE_BAYER_ENABLE_TDN_INPUT) ||
> > > +		    !(hw_enables[0] & PISP_BE_BAYER_ENABLE_TDN) ||
> > > +		    (config->config.tdn.reset & 1)) {
> > > +			hw_enables[0] &= ~(PISP_BE_BAYER_ENABLE_TDN_INPUT |
> > > +					   PISP_BE_BAYER_ENABLE_TDN_DECOMPRESS);
> > > +			if (!(config->config.tdn.reset & 1))
> > > +				hw_enables[0] &= ~PISP_BE_BAYER_ENABLE_TDN;
> > > +		}
> > > +
> > > +		addrs[4] = pispbe_get_addr(buf[STITCH_INPUT_NODE]);
> > > +		if (addrs[4] == 0 ||
> > > +		    !(hw_enables[0] & PISP_BE_BAYER_ENABLE_STITCH_INPUT) ||
> > > +		    !(hw_enables[0] & PISP_BE_BAYER_ENABLE_STITCH)) {
> > > +			hw_enables[0] &=
> > > +				~(PISP_BE_BAYER_ENABLE_STITCH_INPUT |
> > > +				  PISP_BE_BAYER_ENABLE_STITCH_DECOMPRESS |
> > > +				  PISP_BE_BAYER_ENABLE_STITCH);
> > > +		}
> > > +
> > > +		addrs[5] = pispbe_get_addr(buf[TDN_OUTPUT_NODE]);
> > > +		if (addrs[5] == 0)
> > > +			hw_enables[0] &= ~(PISP_BE_BAYER_ENABLE_TDN_COMPRESS |
> > > +					   PISP_BE_BAYER_ENABLE_TDN_OUTPUT);
> > > +
> > > +		addrs[6] = pispbe_get_addr(buf[STITCH_OUTPUT_NODE]);
> > > +		if (addrs[6] == 0)
> > > +			hw_enables[0] &=
> > > +				~(PISP_BE_BAYER_ENABLE_STITCH_COMPRESS |
> > > +				  PISP_BE_BAYER_ENABLE_STITCH_OUTPUT);
> > > +	} else {
> > > +		/* No Bayer input? Disable entire Bayer pipe (else lockup) */
> > > +		hw_enables[0] = 0;
> > > +	}
> > > +
> > > +	/* Main image output channels. */
> > > +	for (unsigned int i = 0; i < PISP_BACK_END_NUM_OUTPUTS; i++) {
> > > +		ret = pispbe_get_planes_addr(addrs + 7 + 3 * i,
> > > +					     buf[OUTPUT0_NODE + i],
> > > +					     &node_group->node[OUTPUT0_NODE + i]);
> > > +		if (ret <= 0)
> > > +			hw_enables[1] &= ~(PISP_BE_RGB_ENABLE_OUTPUT0 << i);
> > > +	}
> > > +}

...

> > > +static void pispbe_node_def_fmt(struct pispbe_node *node)
> > > +{
> > > +	if (NODE_IS_META(node) && NODE_IS_OUTPUT(node)) {
> > > +		/* Config node */
> > > +		struct v4l2_format *f = &node->format;
> > > +
> > > +		f->fmt.meta.dataformat = V4L2_META_FMT_RPI_BE_CFG;
> > > +		f->fmt.meta.buffersize = sizeof(struct pisp_be_tiles_config);
> > > +		f->type = node->buf_type;
> > > +	} else {
> > > +		struct v4l2_format f = {0};
> > > +
> > > +		f.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_YUV420;
> > > +		f.fmt.pix_mp.width = 1920;
> > > +		f.fmt.pix_mp.height = 1080;
> > > +		f.type = node->buf_type;
> >
> > You can assign these in the declaration. The same above.
> 
> Here indeed I can. Above I don't think I can (if you mean in the if()
> branch)

Ack.

...

> > > +	/* Hardware initialisation */
> > > +	pm_runtime_set_autosuspend_delay(pispbe->dev, 200);
> > > +	pm_runtime_use_autosuspend(pispbe->dev);
> > > +	pm_runtime_enable(pispbe->dev);
> > > +
> > > +	ret = pm_runtime_resume_and_get(pispbe->dev);
> >
> > You'll need to call the driver's resume function manually instead. The
> > above depends on CONFIG_PM.
> >
> 
> The driver selects CONFIG_PM, doesn't it ?

It depends on PM.

It'd be trivial to remove that dependency.

-- 
Regards,

Sakari Ailus