lkml.org 
[lkml]   [2023]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v12 5/7] media: chips-media: wave5: Add the v4l2 layer
    On Wed, Sep 27, 2023 at 09:19:46AM +0200, Hans Verkuil wrote:
    > On 27/09/2023 01:29, Nicolas Dufresne wrote:
    > > Le vendredi 22 septembre 2023 à 09:33 +0200, Hans Verkuil a écrit :
    > >> On 21/09/2023 21:11, Nicolas Dufresne wrote:
    > >>> Le mercredi 20 septembre 2023 à 17:13 +0200, Hans Verkuil a écrit :
    > >>>> On 15/09/2023 23:11, Sebastian Fricke wrote:
    > >>>>> From: Nas Chung <nas.chung@chipsnmedia.com>
    > >>>>>
    > >>>>> Add the decoder and encoder implementing the v4l2
    > >>>>> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
    > >>>>>
    > >>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
    > >>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
    > >>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
    > >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
    > >>>>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
    > >>>>> ---
    > >>>>> drivers/media/platform/chips-media/Kconfig | 1 +
    > >>>>> drivers/media/platform/chips-media/Makefile | 1 +
    > >>>>> drivers/media/platform/chips-media/wave5/Kconfig | 12 +
    > >>>>> drivers/media/platform/chips-media/wave5/Makefile | 10 +
    > >>>>> .../platform/chips-media/wave5/wave5-helper.c | 196 ++
    > >>>>> .../platform/chips-media/wave5/wave5-helper.h | 30 +
    > >>>>> .../platform/chips-media/wave5/wave5-vpu-dec.c | 1965 ++++++++++++++++++++
    > >>>>> .../platform/chips-media/wave5/wave5-vpu-enc.c | 1825 ++++++++++++++++++
    > >>>>> .../media/platform/chips-media/wave5/wave5-vpu.c | 331 ++++
    > >>>>> .../media/platform/chips-media/wave5/wave5-vpu.h | 83 +
    > >>>>> 10 files changed, 4454 insertions(+)
    > >>>>>
    > >>
    > >> <snip>
    > >>
    > >>>>> +static int wave5_vpu_dec_set_eos_on_firmware(struct vpu_instance *inst)
    > >>>>> +{
    > >>>>> + int ret;
    > >>>>> +
    > >>>>> + ret = wave5_vpu_dec_update_bitstream_buffer(inst, 0);
    > >>>>> + if (ret) {
    > >>>>> + dev_err(inst->dev->dev,
    > >>>>> + "Setting EOS for the bitstream, fail: %d\n", ret);
    > >>>>
    > >>>> Is this an error due to a driver problem, or because a bad bitstream is
    > >>>> fed from userspace? In the first case, dev_err would be right, in the
    > >>>> second dev_dbg would be more appropriate. Bad userspace input should not
    > >>>> spam the kernel log in general.
    > >>>
    > >>> Its the first. To set the EOS flag, a command is sent to the firmware. That
    > >>> command may never return (timeout) or may report an error. For this specific
    > >>> command, if that happens we are likely facing firmware of driver problem (or
    > >>> both).
    > >>
    > >> OK, I'd add that as a comment here as this is unexpected behavior.
    > >>
    > >>>
    > >>>>
    > >>>>> + return ret;
    > >>>>> + }
    > >>>>> + return 0;
    > >>>>> +}
    > >>
    > >> <snip>
    > >>
    > >>>>> +static int wave5_vpu_dec_create_bufs(struct file *file, void *priv,
    > >>>>> + struct v4l2_create_buffers *create)
    > >>>>> +{
    > >>>>> + struct v4l2_format *f = &create->format;
    > >>>>> +
    > >>>>> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
    > >>>>> + return -ENOTTY;
    > >>>>
    > >>>> Huh? Why is this needed?
    > >>>
    > >>> Minimally a comment should be added. The why is that we support CREATE_BUF for
    > >>> OUTPUT queue (bitstream) but not for CAPTURE queues. This is simply not
    > >>> supported by Wave5 firmware. Do you have any suggestion how this asymmetry can
    > >>> be implemented better ?
    > >>
    > >> Certainly not with ENOTTY: the ioctl exists, it is just not supported for
    > >> CAPTURE queues.
    > >>
    > >> How about -EPERM? And document this error as well in the VIDIOC_CREATE_BUFS
    > >> documentation. And you want a dev_dbg here too.
    > >
    > > The suggestion cannot be used since there is documentation for that one already,
    > > and it does not match "unsupported".
    > >
    > > "Permission denied. Can be returned if the device needs write permission, or
    > > some special capabilities is needed (e. g. root)"
    > >
    > > What about using the most logical error code, which name is actually obvious,
    > > like ENOTSUP ?
    > >
    > > #define ENOTSUPP 524 /* Operation is not supported */
    > >
    >
    > Let's go with EOPNOTSUPP. That seems to be the more commonly used error
    > code in drivers.

    Hi Hans,

    Sorry to belabour this issue but when I change the return value
    to EOPNOTSUPP, it now causes v4l2-compliance to fail because
    v4l2-test-buffers.cpp expects ENOTTY if CREATE_BUFS is not supported.

    We didn't get this warning before because there was a typo in the
    buffer check and it was only checking for single-planar buffers.

    How would you prefer to handle this? The options seem like
    keep ENOTTY in this driver or
    patch v4l2-compliance to warn if it also receives EOPNOTSUPP?

    >
    > >>
    > >> So I would propose that EPERM is returned if CREATE_BUFS is only supported
    > >> for for one of the two queues of an M2M device.
    > >
    > > Note that userspace does not care of the difference between an ioctl not being
    > > implemented at all or not being implement for one queue. GStreamer have been
    > > testing with both queue type for couple of years now. Adding this distinction is
    > > just leaking an implementation details to userspace. I'm fine to just do what
    > > you'd like, just stating the obvious that while it may look logical inside the
    > > kernel, its a bit of a non-sense for our users.
    >
    > I don't agree with that. If an ioctl returns ENOTTY, then userspace can be certain
    > that that ioctl is not implemented for the given file descriptor. That's not the case
    > here: it is implemented, the operation is just not supported for one of the queues.
    >
    > Regards,
    >
    > Hans

    \
     
     \ /
      Last update: 2023-10-03 01:53    [W:6.403 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site