BUG in tegra_channel_enum_framesizes (l4t 28.2.1)

Hello nvidia,

I think I have discovered a bug in ‘tegra_channel_enum_framesizes’ : at entry, ‘tegra_channel_enum_framesizes’ receives a ‘struct v4l2_frmsizeenum’.

struct v4l2_frmivalenum {
        __u32                   index;          /* Frame format index */
        __u32                   pixel_format;   /* Pixel format */
        __u32                   width;          /* Frame width */
        __u32                   height;         /* Frame height */
        __u32                   type;           /* Frame interval type the device supports. */

        union {                                 /* Frame interval */
                struct v4l2_fract               discrete;
                struct v4l2_frmival_stepwise    stepwise;
        };

        __u32   reserved[2];                    /* Reserved space for future use */
};

Looking at https://www.linuxtv.org/downloads/v4l-dvb-apis-old/vidioc-enum-framesizes.html, where it is said : “The supported pixel formats can be obtained by using the VIDIOC_ENUM_FMT function.”, one can easily see that the ‘pixel_format’ field must contain a value in the V4L2_PIX_FMT_* range (also known as ‘fourcc’).

‘tegra_channel_enum_framesizes’ then calls the subdev implementation, passing a ‘struct v4l2_subdev_frame_size_enum’ parameter, where the ‘code’ field should be in the MEDIA_BUS_FMT_* range :

/**
 * struct v4l2_subdev_frame_size_enum - Media bus format enumeration
 * @pad: pad number, as reported by the media API
 * @index: format index during enumeration
 * @code: format code (MEDIA_BUS_FMT_ definitions)
 * @which: format type (from enum v4l2_subdev_format_whence)
 */
struct v4l2_subdev_frame_size_enum {
        __u32 index;
        __u32 pad;
        __u32 code;
        __u32 min_width;
        __u32 max_width;
        __u32 min_height;
        __u32 max_height;
        __u32 which;
        __u32 reserved[8];
};

but MERELY COPIES THE RECEIVED ‘pixel_format’ INTO THE ‘code’ FIELD. A conversion should happen there !

static int
tegra_channel_enum_framesizes(struct file *file, void *fh,
                              struct v4l2_frmsizeenum *sizes)
{
        struct v4l2_fh *vfh = file->private_data;
        struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
        struct v4l2_subdev *sd = chan->subdev_on_csi;
        struct v4l2_subdev_frame_size_enum fse = {
                .index = sizes->index,
                .code = sizes->pixel_format,  <=====  WRONG !
        };
        int ret = 0;

        ret = v4l2_subdev_call(sd, pad, enum_frame_size, NULL, &fse);

        if (!ret) {
                sizes->type = V4L2_FRMSIZE_TYPE_DISCRETE;
                sizes->discrete.width = fse.max_width;
                sizes->discrete.height = fse.max_height;
        }

        return ret;
}

The result of that is that properly written subdev .enum_frame_size functions will always return -EINVAL !!!

There is also probably the same bug in ‘tegra_channel_enum_frameintervals’ :

static int
tegra_channel_enum_frameintervals(struct file *file, void *fh,
                              struct v4l2_frmivalenum *intervals)
{
        struct v4l2_fh *vfh = file->private_data;
        struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
        struct v4l2_subdev *sd = chan->subdev_on_csi;
        struct v4l2_subdev_frame_interval_enum fie = {
                .index = intervals->index,
                .code = intervals->pixel_format, <====== WRONG
                .width = intervals->width,
                .height = intervals->height,
        };
...
}

hello phdm,

thanks for point out, we will investigate this internally.

When do you plan to provide a patch ? I’d be glad to test it early.

hello phdm,

could you please apply this kernel patch (20180905_Topic1038421_add-format-code-conversions.tar.gz) in the attachment for verification,
thanks
20180905_Topic1038421_add-format-code-conversions.tar.gz (797 Bytes)

Hello JerryChang,

Thank you for the patch.

You may add :
Reported-by: Philippe De Muyter phdm@macqel.be
Tested-by: Philippe De Muyter phdm@macqel.be

But actually v4l2-compliance complains :

    Format ioctls:
            fail: v4l2-test-formats.cpp(315): Accepted framesize for invalid format

Probably because of that part :

if (!vfmt) {
                vfmt = tegra_core_get_format_by_fourcc(chan, chan->format.pixelformat);
        }

If I replace it by

if (!vfmt) {
                return -EINVAL;
        }

then v4l2-compliance does not complain anymore.

hello phdm,

thanks for the review feedback of the patch in comment #4,
we’re now working to integrate the patch into code-line, will get back to you when have some conclusions.
thanks

hello phdm,

please take kernel patch 20181012_Topic1038421_add-format-code-conversions.tar.gz for the improvement.
we also integrate this into code-line, please expect this will be include for next public release.
thanks
20181012_Topic1038421_add-format-code-conversions.tar.gz (2 KB)

hello JerryChang,

does the new patch replace the previous one, or does it come on top of the previous one ?

thanks

hello phdm,

please replace with the previous one,
thanks

Hello JerryChang,

Reported-by: Philippe De Muyter phdm@macqel.be
Tested-by: Philippe De Muyter phdm@macqel.be
Acked-by: Philippe De Muyter phdm@macqel.be