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
