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.
>
>>
>> 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.
>
>> 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.
>
>>
>> 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.
Regards,
Hans
