On Mon, May 27, 2024 at 08:14:00AM +0000, Sakari Ailus wrote:
> 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.
I've been slowly but steadily working on increasing that number :-) I
think the style above is the most readable, and I would leave it to
driver authors (as long as they're consistent within a driver).
>
> ...
>
> > > > +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.
For drivers such as sensor drivers that need to work on a wide variety
of platforms, with varying configurations, I agree that not depending on
CONFIG_PM is a good thing (I reserve the right to change my mind though
:-)). For this driver, I don't think the dependency is an issue.
--
Regards,
Laurent Pinchart
