Re: [PATCH v2 3/9] v4l: platform: Add Renesas CEU driver

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

Hi Jacopo,

On Wednesday, 3 January 2018 12:47:48 EET jacopo mondi wrote:
> On Tue, Jan 02, 2018 at 03:46:27PM +0200, Laurent Pinchart wrote:
> >> +/*
> >> + * ceu_device - CEU device instance
> >> + */
> >> +struct ceu_device {
> >> +	struct device		*dev;
> >> +	struct video_device	vdev;
> >> +	struct v4l2_device	v4l2_dev;
> >> +
> >> +	/* subdevices descriptors */
> >> +	struct ceu_subdev	*subdevs;
> >> +	/* the subdevice currently in use */
> >> +	struct ceu_subdev	*sd;
> >> +	unsigned int		sd_index;
> >> +	unsigned int		num_sd;
> >> +
> >> +	/* platform specific mask with all IRQ sources flagged */
> >> +	u32			irq_mask;
> >> +
> >> +	/* currently configured field and pixel format */
> >> +	enum v4l2_field	field;
> >> +	struct v4l2_pix_format_mplane v4l2_pix;
> >> +
> >> +	/* async subdev notification helpers */
> >> +	struct v4l2_async_notifier notifier;
> >> +	/* pointers to "struct ceu_subdevice -> asd" */
> >> +	struct v4l2_async_subdev **asds;
> >> +
> >> +	/* vb2 queue, capture buffer list and active buffer pointer */
> >> +	struct vb2_queue	vb2_vq;
> >> +	struct list_head	capture;
> >> +	struct vb2_v4l2_buffer	*active;
> >> +	unsigned int		sequence;
> >> +
> >> +	/* mlock - lock device suspend/resume and videobuf2 operations */
> > 
> > In my review of v1 I commented that lock documentation should explain what
> > data is protected by the lock. As my point seems not to have come across
> > it must not have been clear enough, I'll try to fix that.
> > 
> > The purpose of a lock is to protect from concurrent access to a resource.
> > In device drivers resources are in most cases either in-memory data or
> > device registers. To design a good locking scheme you need to ask
> > yourself what resources can be accessed concurrently, and then protect
> > all accesses to those resources using locking primitives. Some accesses
> > don't need to be protected (for instance it's common to initialize
> > structure fields in the probe function where no concurrent access from
> > userspace can occur as device nodes are not registered yet), and locking
> > can then be omitted in a case by case basis.
> > 
> > Lock documentation is essential to understand the locking scheme and
> > should explain what resources are protected by the lock. It's tempting
> > (because it's easy) to instead focus on what code sections the lock
> > covers, but that's not how the locking scheme should be designed, and will
> > eventually be prone to bugs leading to race conditions.
> 
> Thanks, I got this, but that lock is used to protect concurrent
> accesses to suspend/resume (and thus interface reset) and is used as
> vb2 queue lock. I can mention it guards concurrent interfaces resets,
> but I don't see it being that much different from what I already
> mentioned.

You're right, in this case the way videobuf2 operates makes it difficult to 
find out what the lock protects exactly. That's why I don't like locks that 
cover code sections instead of protecting data, they're much harder to check 
for correctness. There's nothing you can do about it.

> > Obviously a lock will end up preventing multiple code sections from
> > running at the same time, but that's the consequence of the locking
> > scheme, it shouldn't be its cause.
> > 
> >> +	struct mutex	mlock;
> >> +
> >> +	/* lock - lock access to capture buffer queue and active buffer */
> >> +	spinlock_t	lock;
> >> +
> >> +	/* base - CEU memory base address */
> >> +	void __iomem	*base;
> >> +};

[snip]

> >> +/* CEU Capture Operations */
> > 
> > Just curious, why have you replaced the block comments by single-line
> > comments ? I pointed out that the format was wrong as you started them
> > with /** and they were not kerneldoc, but I have nothing against
> > splitting the code in sections with headers such as
> > 
> > /* -----------------------------------------------------------------------
> >  * CEU Capture Operations
> >  */
> > 
> > as I do so routinely in my drivers. If that's your preferred style and you
> > thought I asked for a change you can switch back, if you prefer
> > single-line comments that's fine with me too.
> 
> Yes I borrowed that commenting style from other Renesas drivers you
> wrote, so I went for it for consistency.
> 
> I recently read about some discussions on block comments, when Mauro
> was trying to replace /***...***/ block comments with a script, and
> I had the feeling there's not that much love for block comments around
> here, and I also find them a bit invasive.
> 
> I used the
> /* --- Code section --- */
> style in the past which I find is a good balance between
> intrusiveness and noticeability but I don't want to introduce
> yet-another-comment-style so I went for single line and that's it.

I think this is a matter of personal preference and best left to the driver 
author. Of course minimizing the number of style differences is a good idea, 
but I don't see anything wrong with a block comment (quite obviously as I use 
them ;-)) or your /* --- Code section --- */ comment. I find that having a 
comment that stands out improves readability in long source files. It's up to 
you.

> > [snip]
> > 
> >> +/*
> >> + * ceu_calc_plane_sizes() - Fill per-plane 'struct
> >> v4l2_plane_pix_format'
> >> + *			    information according to the currently configured
> >> + *			    pixel format.
> >> + * @ceu_device: CEU device.
> >> + * @ceu_fmt: Active image format.
> >> + * @pix: Pixel format information (store line width and image sizes)
> >> + *
> >> + * Returns 0 for success.
> >> + */
> >> +static int ceu_calc_plane_sizes(struct ceu_device *ceudev,
> >> +				const struct ceu_fmt *ceu_fmt,
> >> +				struct v4l2_pix_format_mplane *pix)
> >> +{
> >> +	unsigned int bpl, szimage;
> >> +
> >> +	switch (pix->pixelformat) {
> >> +	case V4L2_PIX_FMT_YUYV:
> >> +		pix->num_planes	= 1;
> >> +		bpl		= pix->width * ceu_fmt->bpp / 8;
> >> +		szimage		= pix->height * bpl;
> >> +		ceu_update_plane_sizes(&pix->plane_fmt[0], bpl, szimage);
> >> +		break;
> >> +
> >> +	case V4L2_PIX_FMT_NV16:
> >> +	case V4L2_PIX_FMT_NV61:
> >> +		pix->num_planes	= 2;
> >> +		bpl		= pix->width;
> >> +		szimage		= pix->height * pix->width;
> >> +		ceu_update_plane_sizes(&pix->plane_fmt[0], bpl, szimage);
> >> +		ceu_update_plane_sizes(&pix->plane_fmt[1], bpl, szimage);
> >> +		break;
> >> +
> >> +	case V4L2_PIX_FMT_NV12:
> >> +	case V4L2_PIX_FMT_NV21:
> >> +		pix->num_planes	= 2;
> >> +		bpl		= pix->width;
> >> +		szimage		= pix->height * pix->width;
> >> +		ceu_update_plane_sizes(&pix->plane_fmt[0], bpl, szimage);
> >> +		ceu_update_plane_sizes(&pix->plane_fmt[1], bpl, szimage / 2);
> >> +		break;
> >> +
> >> +	default:
> >> +		pix->num_planes	= 0;
> >> +		dev_err(ceudev->dev,
> >> +			"Format 0x%x not supported\n", pix->pixelformat);
> >> +		return -EINVAL;
> > 
> > I think you can remove the default case as ceu_try_fmt() should have
> > validated the format already. The compiler will then likely warn so you
> > need to keep a default cause, but it will never be hit, so it can default
> > to any format you want. The function can then be turned into a void.
> 
> Yes, that was only to silence the compiler actually...

Which has to be done, but let's try to not add dead code :-) Putting the 
default case right below the pixel format that the try format function 
defaults to should be enough for instance.

> >> +	}
> >> +
> >> +	return 0;
> >> +}

-- 
Regards,

Laurent Pinchart