On Mon, Oct 21, 2019 at 7:17 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> On 10/21/19 11:48 AM, Tomasz Figa wrote:
> > On Mon, Oct 21, 2019 at 6:38 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >>
> >> On 10/21/19 11:26 AM, Tomasz Figa wrote:
> >>> On Mon, Oct 21, 2019 at 5:41 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >>>>
> >>>> On 10/8/19 11:11 AM, Boris Brezillon wrote:
> >>>>> This is part of the multiplanar and singleplanar unification process.
> >>>>> v4l2_ext_pix_format is supposed to work for both cases.
> >>>>>
> >>>>> We also add the concept of modifiers already employed in DRM to expose
> >>>>> HW-specific formats (like tiled or compressed formats) and allow
> >>>>> exchanging this information with the DRM subsystem in a consistent way.
> >>>>>
> >>>>> Note that V4L2_BUF_TYPE_VIDEO[_OUTPUT]_OVERLAY and
> >>>>> V4L2_BUF_TYPE_VIDEO_{CAPTURE,OUTPUT}_MPLANE types are no longer accepted
> >>>>> in v4l2_ext_format and will be rejected if you use the {G,S,TRY}EXT_FMT
> >>>>> ioctls. V4L2_BUF_TYPE_VIDEO_{CAPTURE,OUTPUT}_MPLANE is dropped as part
> >>>>> of the multiplanar/singleplanar unification.
> >>>>> V4L2_BUF_TYPE_VIDEO[_OUTPUT]_OVERLAY seems to be used mostly on old
> >>>>> drivers and supporting it would require some extra rework.
> >>>>>
> >>>>> New hooks have been added to v4l2_ioctl_ops to support those new ioctls
> >>>>> in drivers, but, in the meantime, the core takes care of converting
> >>>>> {S,G,TRY}_EXT_FMT requests into {S,G,TRY}_FMT so that old drivers can
> >>>>> still work if the userspace app/lib uses the new ioctls.
> >>>>> The conversion is also done the other around to allow userspace
> >>>>> apps/libs using {S,G,TRY}_FMT to work with drivers implementing the
> >>>>> _ext_ hooks.
> >>>>>
> >>>>> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >>>>> ---
> >>>>
> >>>> <snip>
> >>>>
> >>>>>
> >>>>> +#define VIDIOC_G_EXT_FMT _IOWR('V', 104, struct v4l2_ext_format)
> >>>>> +#define VIDIOC_S_EXT_FMT _IOWR('V', 105, struct v4l2_ext_format)
> >>>>> +#define VIDIOC_TRY_EXT_FMT _IOWR('V', 106, struct v4l2_ext_format)
> >>>>> /* Reminder: when adding new ioctls please add support for them to
> >>>>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> >>>>>
> >>>>>
> >>>>
> >>>> Since we're extending g/s/try_fmt, we should also provide a replacement for
> >>>> enum_fmt, esp. given this thread:
> >>>>
> >>>> https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg150871.html
> >>>
> >>> While that's a completely valid problem that should be addressed, I'm
> >>> not sure if putting all the things in one bag would have a positive
> >>> effect on solving all the problems in a reasonable timeline.
> >>
> >> I'm not sure what you mean with this. We can't ignore this either, and if
> >> we're going to add these new ioctls, then let's try to fix as much as we can.
> >>
> >
> > My point is that this series solves a completely orthogonal problem
> > without a need to touch ENUM_FMT at all. Is there any reason why
> > solving this particular problem separately would make solving the
> > ENUM_FMT problem more difficult in the future?
>
> I do not agree with you that this is an orthogonal problem. If we are creating
> new FMT ioctls to solve various problems, then it makes sense to look at ALL the
> problems, including this. We might decide to postpone creating a EXT_ENUM_FMT
> ioctl, but during the discussion we should look at this issue as well.
>
> To take one suggestion you made: use of the active/try slots for existing non-MC
> drivers. If we decide to go into that direction (and I think it is a very interesting
> idea), then that requires that ENUM_FMT is taken into account anyway. And probably
> other ioctls such as the selection API as well.
>
> I think we need to think big here, and try to at least explore the possibility
> of creating an API that tries to limit the differences between video and subdev
> nodes.
>
Okay, agreed on this.
> >
> >>>
> >>>>
> >>>> So here is a preliminary suggestion:
> >>>>
> >>>> struct v4l2_ext_fmtdesc {
> >>>> __u32 index; /* Format number */
> >>>> __u32 type; /* enum v4l2_buf_type */
> >>>> __u32 which; /* enum v4l2_subdev_format_whence, ignored if mbus_code == 0 */
> >>>> __u32 mbus_code; /* Mediabus Code (set to 0 if n/a) */
> >>>> __u32 flags;
> >>>> __u8 description[32]; /* Description string */
> >>>> __u32 pixelformat; /* Format fourcc */
> >>>> };
> >>>>
> >>>> This would solve (I think) the issue raised in the thread since you can now get
> >>>> just for formats that are valid for the given mediabus code and the which field.
> >>>>
> >>>
> >>> Hmm, why only mbus_code? We have the same problem with mem2mem
> >>> devices, where the format set on one queue affects the formats
> >>> supported on another queue. Perhaps it should be defined to return
> >>> formats valid with the current state of the driver? If we want to
> >>> extend it to return formats for arbitrary states, perhaps we should
> >>> use a mechanism similar to the TRY_FMT slot in subdevices, where we
> >>> can set the configuration we want to test against and then ENUM_FMT
> >>> would return the formats valid for that configuration?
> >>
> >> Good point.
> >>
> >
> > We might want to keep the control'ification of the API in mind, which
> > should simplify dealing with state inter-dependencies, because all the
> > state would be represented in a uniform way.
>
> I still don't know what Laurent wants to do with that.
>
Not sure why Laurent specifically. :)
On the other hand, that's a much more invasive change, so maybe better
not to mix it with the things discussed here.
> >
> >>>
> >>>> Other improvements that could be made is to return more information about the
> >>>> format (similar to struct v4l2_format_info). In particular v4l2_pixel_encoding
> >>>> and mem/comp_planes would be useful for userspace to know.
> >>>
> >>> An alternative would be to go away from mixing mem_planes and
> >>> pixelformats and just having the "planarity" queryable and
> >>> configurable separately. The existing duplicated FourCCs would have to
> >>> remain for compatibility reasons, though.
> >>
> >> Interesting idea, but I don't know if this would make the API more or less confusing.
> >>
> >
> > Yeah, this definitely needs more thought. My experience with working
> > with many people trying to use V4L2 in the userspace tells me that the
> > existing model is confusing, though. DRM and most userspace libraries
> > use FourCCs only to define the pixelformats themselves and planarity
> > is a separate aspect.
> >
> > The target model that I'd see happening would be to have the
> > multiple-memory plane semantics used everywhere, so all color planes
> > are described as separate entities, up to having different DMA-buf
> > FDs. Then the single memory plane case would be just one of the
> > specific cases, where all the DMA-buf FDs point to the same buffer and
> > color plane offsets are laid out contiguously, which could be enforced
> > by generic code when queuing the buffer if that's a hardware
> > requirement.
>
> Do you think you can come up with a rough proposal before the ELCE session?
Sorry, I wasn't able to come up with anything formal, but I believe we
discussed this well during the session and it's written down in the
notes.
Best regards,
Tomasz
>
> Regards,
>
> Hans
>
> >
> >>>
> >>>>
> >>>> Finally, we can also add a new ioctl that combines ENUM_FMT/ENUM_FRAMESIZES/ENUM_FRAMEINTERVALS
> >>>> and returns an array of all valid formats/sizes/intervals, requiring just a single ioctl
> >>>> to obtain all this information.
> >>>
> >>> While it definitely sounds like a useful thing to have, it would be an
> >>> interesting problem to solve given the inter-dependencies between
> >>> pads, queues and other state, as in the mbus example above.
> >>
> >> This is definitely a separate issue for further in the future.
> >
> > Agreed.
> >
> > Best regards,
> > Tomasz
> >
>
