Message-ID: <20201119054544.GA590258@chromium.org>
Date:   Thu, 19 Nov 2020 14:45:44 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Helen Koike <helen.koike@...labora.com>
Cc:     mchehab@...nel.org, hans.verkuil@...co.com,
        laurent.pinchart@...asonboard.com, sakari.ailus@....fi,
        linux-media@...r.kernel.org,
        Boris Brezillon <boris.brezillon@...labora.com>,
        hiroh@...omium.org, nicolas@...fresne.ca, Brian.Starkey@....com,
        kernel@...labora.com, narmstrong@...libre.com,
        linux-kernel@...r.kernel.org, frkoenig@...omium.org,
        mjourdan@...libre.com, stanimir.varbanov@...aro.org
Subject: Re: [PATCH v5 1/7] media: v4l2: Extend pixel formats to unify
 single/multi-planar handling (and more)

On Sat, Nov 14, 2020 at 11:21:26AM -0300, Helen Koike wrote:
> Hi Tomasz,
> 
> On 10/2/20 4:49 PM, Tomasz Figa wrote:
> > Hi Helen,
> > 
> > On Tue, Aug 04, 2020 at 04:29:33PM -0300, Helen Koike wrote:
[snip]
> >> +static void v4l_print_ext_pix_format(const void *arg, bool write_only)
> >> +{
> >> +	const struct v4l2_ext_pix_format *pix = arg;
> >> +	unsigned int i;
> >> +
> >> +	pr_cont("type=%s, width=%u, height=%u, format=%c%c%c%c, modifier %llx, field=%s, colorspace=%d, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n",
> >> +		prt_names(pix->type, v4l2_type_names),
> >> +		pix->width, pix->height,
> >> +		(pix->pixelformat & 0xff),
> >> +		(pix->pixelformat >>  8) & 0xff,
> >> +		(pix->pixelformat >> 16) & 0xff,
> >> +		(pix->pixelformat >> 24) & 0xff,
> >> +		pix->modifier, prt_names(pix->field, v4l2_field_names),
> >> +		pix->colorspace, pix->ycbcr_enc,
> >> +		pix->quantization, pix->xfer_func);
> >> +	for (i = 0; i < VIDEO_MAX_PLANES && pix->plane_fmt[i].sizeimage; i++)
> > 
> > This is going to print 8 lines every time. Maybe we could skip 0-sized
> > planes or something?
> 
> I'm already checking pix->plane_fmt[i].sizeimage in the loop, it shouldn't
> print 8 lines every time.
> 

Oops, how could I not notice it. Sorry for the noise.

[snip]
> >> +int v4l2_ext_pix_format_to_format(const struct v4l2_ext_pix_format *e,
> >> +				  struct v4l2_format *f, bool mplane_cap,
> >> +				  bool strict)
> >> +{
> >> +	const struct v4l2_plane_ext_pix_format *pe;
> >> +	struct v4l2_plane_pix_format *p;
> >> +	unsigned int i;
> >> +
> >> +	memset(f, 0, sizeof(*f));
> >> +
> >> +	/*
> >> +	 * Make sure no modifier is required before doing the
> >> +	 * conversion.
> >> +	 */
> >> +	if (e->modifier && strict &&
> > 
> > Do we need the explicit check for e->modifier != 0 if we have to check for
> > the 2 specific values below anyway?
> 
> We don't, since DRM_FORMAT_MOD_LINEAR is zero.
> 
> But I wanted to make it explicit we don't support modifiers in this conversion.
> But I can remove this check, no problem.
> 

Yes, please. I think the double checking is confusing for the reader.

> > 
> >> +	    e->modifier != DRM_FORMAT_MOD_LINEAR &&
> >> +	    e->modifier != DRM_FORMAT_MOD_INVALID)
> >> +		return -EINVAL;
> >> +
> >> +	if (!e->plane_fmt[0].sizeimage && strict)
> >> +		return -EINVAL;
> > 
> > Why is this incorrect?
> 
> !sizeimage for the first plane means that there are no planes in ef.
> strict is true if the result for the conversion should be returned to userspace
> and it is not some internal handling.
> 
> So if there are no planes, we would return an incomplete v4l2_format struct
> to userspace.
> 
> But this is not very clear, I'll improve this for the next version.
> 

So I can see 2 cases here:

1) Userspace gives ext struct and driver accepts legacy.

In this case, the kernel needs to adjust the structure to be correct.
-EINVAL is only valid if

"The struct v4l2_format type field is invalid or the requested buffer type not supported."

as per the current uAPI documentation.

2) Driver gives ext struct and userspace accepts legacy.

If at this point we get a struct with no planes, that sounds like a
driver bug, so rather than signaling -EINVAL to the userspace, we should
probably WARN()?

Or am I getting something wrong? :)

[snip]
> >> +{
> >> +	const struct v4l2_plane_pix_format *p;
> >> +	struct v4l2_plane_ext_pix_format *pe;
> >> +	unsigned int i;
> >> +
> >> +	memset(e, 0, sizeof(*e));
> >> +
> >> +	switch (f->type) {
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >> +		e->width = f->fmt.pix.width;
> >> +		e->height = f->fmt.pix.height;
> >> +		e->pixelformat = f->fmt.pix.pixelformat;
> >> +		e->field = f->fmt.pix.field;
> >> +		e->colorspace = f->fmt.pix.colorspace;
> >> +		if (f->fmt.pix.flags)
> >> +			pr_warn("Ignoring pixelformat flags 0x%x\n",
> >> +				f->fmt.pix.flags);
> > 
> > Would it make sense to print something like video node name and/or function
> > name to explain where this warning comes from?
> 
> I would need to update the function to receive this information, I can try but
> I'm not sure if it is worthy.
> 

I don't have a strong opinion on this, so maybe let's see if others have
any comments.

> > 
> >> +		e->ycbcr_enc = f->fmt.pix.ycbcr_enc;
> >> +		e->quantization = f->fmt.pix.quantization;
> > 
> > Missing xfer_func?
> 
> Yes, thanks for catching this.
> 
> > 
> >> +		e->plane_fmt[0].bytesperline = f->fmt.pix.bytesperline;
> >> +		e->plane_fmt[0].sizeimage = f->fmt.pix.sizeimage;
> > 
> > This doesn't look right. In the ext API we expected the planes to describe
> > color planes, which means that bytesperline needs to be computed for planes
> >> = 1 and sizeimage replaced with per-plane sizes, according to the
> >> pixelformat.
> 
> Ack.
> 
> Just to be clear, even if we are using a planar format that isn't a V4L2_PIX_FMT_*M
> variant, we should describe every plane separatly.
> 
> For instance, if V4L2_PIX_FMT_YVU420 is being used, then f->fmt.pix.bytesperline
> will have data, and we need to calculate bytesperline for all 3 planes, so we'll fill
> out:
> 
> f->plane_fmt[0].bytesperline = f->fmt.pix.bytesperline;
> f->plane_fmt[1].bytesperline = f->fmt.pix.bytesperline / hdiv;
> f->plane_fmt[2].bytesperline = f->fmt.pix.bytesperline / hdiv;
> 
> I'll update this for the next version.
> 

Yes. This basically gives us a unified representation across all
pixelformats and allows userspace to handle them in a uniform way, as
opposed to current uAPI.

[snip]
> >> +		if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> >> +			e->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +		else
> >> +			e->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +
> >> +		for (i = 0; i < VIDEO_MAX_PLANES; i++) {
> >> +			pe = &e->plane_fmt[i];
> >> +			p = &f->fmt.pix_mp.plane_fmt[i];
> >> +			pe->bytesperline = p->bytesperline;
> >> +			pe->sizeimage = p->sizeimage;
> >> +		}
> > 
> > Same here. A blind copy is not enough. For non-M formats, the plane
> > parameters need to be filled according to the pixelformat.
> 
> 
> Right, following the idea above, we need a different handling if we
> aren't using a M-variant of the pixelformat, and we also need to
> convert the pixelformat from the M-variant to non-M-variant.
> 
> I'll also need to save that the original format was a
> M-variant or not, so I can convert it back as expected.

I'm still reading the rest of the series, so it might be answered
already, but did we decide to do anything about the pixelformat codes
themselves? If both M and non-M variants would be allowed with the new
API, then I guess there isn't anything to save, because the original
format would be preserved?

> 
> I'll change this and submit for review.
> 

Cool, thanks.

Best regards,
Tomasz

Powered by blists - more mailing lists