Hi Tomasz,
On 11/19/20 2:45 AM, Tomasz Figa wrote:
> 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.
ok.
>
>>>
>>>> + 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? :)
Make sense, I'll restructure this for the next version.
>
> [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.
Right, I already updated this in my wip branch for next version.
>
> [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 was working with the idea that M-variants wouldn't be allowed.
But then, we have cases where non-M-variant don't exist, such as:
V4L2_PIX_FMT_YVU422M
V4L2_PIX_FMT_YVU444M
(at least, I couldn't find non-M-variant equivalent for those)
But actually, I don't think we formally decided this (and it seems
easier to implement if both are allowed).
Should we allow both variants in the Ext API ?
Thanks
Helen
>
>>
>> I'll change this and submit for review.
>>
>
> Cool, thanks.
>
> Best regards,
> Tomasz
>
