Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
On 21/07/2020 16:23, Helen Koike wrote: > Hi, > > On 7/21/20 7:24 AM, Hans Verkuil wrote: >> On 17/07/2020 13:54, Helen Koike wrote: >>> Hi, >>> >>> I'm sorry for taking too long to submit v4. >>> >>> It is not perfect, not all v4l2-compliance tests passes, but I'd like a >>> review, >>> specially on the API and potential problems, so I can focus on improving >>> implementation >>> and maybe drop the RFC tag for next version. >>> >>> Follow below what changed in v4 and some items I'd like to discuss: >>> >>> >>> * Ioctl to replace v4l2_pix_format >>> - >>> During last media summit, we agreed to create ioctls that replace the >>> v4l2_pix_format >>> struct and leave the other structs in the v4l2_format union alone. >>> Thus I refactored the code to receive struct v4l2_ext_pix_format, and I >>> renamed the >>> ioctls, so now we have: >>> >>> int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); >>> int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); >>> int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); >>> >>> The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and >>> V4L2_BUF_TYPE_VIDEO_OUTPUT, >>> all the other types are invalid with this API. >>> >>> >>> * Modifiers >>> - >>> I understand that unifying DRM and V4L2 pixel formats is not possible, but >>> I'd like >>> to unify the modifiers [1]. >>> >>> [1] >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290 >>> >>> Should we use the DRM modifiers directly in the V4L2 API? >> >> For now, yes. Most of the modifier work is done in DRM, it is only fairly >> recent >> that the media subsystem starts to have a need for it. So for now just use >> the drm >> header and prefixes. > > ack > >> >>> Or should we move this header to a common place and change the prefix? >>> (which requires >>> us to sync with DRM community). >>> Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping >>> to DRM_ >>> macros? >>> >>> For now, patch 1/6 includes drm/drm_fourcc.h and it is using >>> DRM_FORMAT_MOD_* >>> >>> As discussed before, It would be nice to have documentation describing DRM >>> fourcc >>> equivalents (I'm not sure if someone started this already), listing the >>> number of >>> planes per format. >>> >>> We should also document which pixelformats are valid for the EXT_API, since >>> multiplanar >>> and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to >>> DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) >>> should be >>> replaced by a modifier. >>> >>> Regarding flags [2] field in struct v4l2_pix_format_mplane [3]: >>> The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only >>> used by vsp1 driver. >>> Which I believe could be replaced by a modifier, to avoid another field >>> that changes >>> pixel formats, so I removed it from the EXT API (we can always add it back >>> later with >>> the reserved fields). >> >> The colorspace series that Dafna is working on will add a >> V4L2_PIX_FMT_FLAG_SET_CSC >> flag, so this flags field will be needed. > > This was because the CSC fields were defined in the API as read only (filled > by the driver), > what if those fields in struct v4l2_ext_pix_format allows user to change the > CSC fields, > and it will just fill the right one if it is not supported (similar to how > other fields works > already). > Please, let me know if I'm missing something. Ah, that's true, I forgot about that. > >> >>> >>> [2] >>> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags >>> [3] >>> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane >>> >>> We also discussed to add a new ENUM_FMT_EXT ioctl to return all >>> pixelformats + modifiers >>> combinations. I still didn't add it in this version, but I don't think it >>> affects >>> what is in this RFC and it can be added later. >>> >>> >>> * Buffers/Plane offset >>> - >>> >>> My understanding is that inside a memory buffer we can have multiple planes >>> in random >>> offsets. >>> I was comparing with the DRM API [4], where it can have the same dmabuf for >>> multiple >>> planes in different offsets, and I started to think we could simplify our >>> API, so >>> I took the liberty to do some more changes, please review struct >>> v4l2_ext_plane in >>> this RFC. >>> >>> I removed the data_offset, since it is unused (See Laurent's RFC >>> repurposing this >>> field [5]). And comparing to the DRM API, it seems to me we only need a >>> single offset >>> field. >>> >>> We could also check about
Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
Hi, On 7/21/20 7:24 AM, Hans Verkuil wrote: > On 17/07/2020 13:54, Helen Koike wrote: >> Hi, >> >> I'm sorry for taking too long to submit v4. >> >> It is not perfect, not all v4l2-compliance tests passes, but I'd like a >> review, >> specially on the API and potential problems, so I can focus on improving >> implementation >> and maybe drop the RFC tag for next version. >> >> Follow below what changed in v4 and some items I'd like to discuss: >> >> >> * Ioctl to replace v4l2_pix_format >> - >> During last media summit, we agreed to create ioctls that replace the >> v4l2_pix_format >> struct and leave the other structs in the v4l2_format union alone. >> Thus I refactored the code to receive struct v4l2_ext_pix_format, and I >> renamed the >> ioctls, so now we have: >> >> int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); >> int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); >> int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); >> >> The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and >> V4L2_BUF_TYPE_VIDEO_OUTPUT, >> all the other types are invalid with this API. >> >> >> * Modifiers >> - >> I understand that unifying DRM and V4L2 pixel formats is not possible, but >> I'd like >> to unify the modifiers [1]. >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290 >> >> Should we use the DRM modifiers directly in the V4L2 API? > > For now, yes. Most of the modifier work is done in DRM, it is only fairly > recent > that the media subsystem starts to have a need for it. So for now just use > the drm > header and prefixes. ack > >> Or should we move this header to a common place and change the prefix? >> (which requires >> us to sync with DRM community). >> Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping to >> DRM_ >> macros? >> >> For now, patch 1/6 includes drm/drm_fourcc.h and it is using DRM_FORMAT_MOD_* >> >> As discussed before, It would be nice to have documentation describing DRM >> fourcc >> equivalents (I'm not sure if someone started this already), listing the >> number of >> planes per format. >> >> We should also document which pixelformats are valid for the EXT_API, since >> multiplanar >> and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to >> DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) >> should be >> replaced by a modifier. >> >> Regarding flags [2] field in struct v4l2_pix_format_mplane [3]: >> The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only used >> by vsp1 driver. >> Which I believe could be replaced by a modifier, to avoid another field that >> changes >> pixel formats, so I removed it from the EXT API (we can always add it back >> later with >> the reserved fields). > > The colorspace series that Dafna is working on will add a > V4L2_PIX_FMT_FLAG_SET_CSC > flag, so this flags field will be needed. This was because the CSC fields were defined in the API as read only (filled by the driver), what if those fields in struct v4l2_ext_pix_format allows user to change the CSC fields, and it will just fill the right one if it is not supported (similar to how other fields works already). Please, let me know if I'm missing something. > >> >> [2] >> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags >> [3] >> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane >> >> We also discussed to add a new ENUM_FMT_EXT ioctl to return all pixelformats >> + modifiers >> combinations. I still didn't add it in this version, but I don't think it >> affects >> what is in this RFC and it can be added later. >> >> >> * Buffers/Plane offset >> - >> >> My understanding is that inside a memory buffer we can have multiple planes >> in random >> offsets. >> I was comparing with the DRM API [4], where it can have the same dmabuf for >> multiple >> planes in different offsets, and I started to think we could simplify our >> API, so >> I took the liberty to do some more changes, please review struct >> v4l2_ext_plane in >> this RFC. >> >> I removed the data_offset, since it is unused (See Laurent's RFC repurposing >> this >> field [5]). And comparing to the DRM API, it seems to me we only need a >> single offset >> field. >> >> We could also check about overlapping planes in a memory buffer, but this is >> complicated >> if we use the same memory buffer with different v4l2_ext_buffer objects. We >> can also leave >> to the driver to check situations that may cause HW errors. >> >>
Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
Hi Boris, On 7/21/20 8:15 AM, Boris Brezillon wrote: > Hello Helen, > > Just a few drive-by comments. > > On Fri, 17 Jul 2020 08:54:29 -0300 > Helen Koike wrote: > >> Hi, >> >> I'm sorry for taking too long to submit v4. >> >> It is not perfect, not all v4l2-compliance tests passes, but I'd like a >> review, >> specially on the API and potential problems, so I can focus on improving >> implementation >> and maybe drop the RFC tag for next version. >> >> Follow below what changed in v4 and some items I'd like to discuss: >> >> >> * Ioctl to replace v4l2_pix_format >> - >> During last media summit, we agreed to create ioctls that replace the >> v4l2_pix_format >> struct and leave the other structs in the v4l2_format union alone. >> Thus I refactored the code to receive struct v4l2_ext_pix_format, and I >> renamed the >> ioctls, so now we have: >> >> int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); > > Maybe use the EXT_PIX_FMT suffix here since the struct is really only > about pixel formats. Sorry, this is a copy error, I'm already using this suffix in the code, except for the ioctls that handle buffers (since they get v4l2_ext_buffer struct). Regards, Helen > >> int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); >> int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); >> >> The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and >> V4L2_BUF_TYPE_VIDEO_OUTPUT, >> all the other types are invalid with this API. >> > > [...] > >> >> >> Boris Brezillon (5): >> media: v4l2: Extend pixel formats to unify single/multi-planar >> handling (and more) >> media: videobuf2: Expose helpers to implement the _ext_fmt and >> _ext_buf hooks >> media: mediabus: Add helpers to convert a ext_pix format to/from a >> mbus_fmt >> media: vivid: Convert the capture and output drivers to >> EXT_FMT/EXT_BUF >> media: vimc: Implement the ext_fmt and ext_buf hooks > > I think you should take ownership of these patches. The end result is > likely to be completely different from what I initially posted, and > you're the one doing the hard work here. > >> >> Hans Verkuil (1): >> media: v4l2: Add extended buffer operations >> >> .../media/common/videobuf2/videobuf2-core.c | 2 + >> .../media/common/videobuf2/videobuf2-v4l2.c | 549 +- >> .../media/test-drivers/vimc/vimc-capture.c| 61 +- >> drivers/media/test-drivers/vimc/vimc-common.c | 6 +- >> drivers/media/test-drivers/vimc/vimc-common.h | 2 +- >> drivers/media/test-drivers/vivid/vivid-core.c | 70 +- >> .../test-drivers/vivid/vivid-touch-cap.c | 26 +- >> .../test-drivers/vivid/vivid-touch-cap.h | 3 +- >> .../media/test-drivers/vivid/vivid-vid-cap.c | 169 +--- >> .../media/test-drivers/vivid/vivid-vid-cap.h | 15 +- >> .../media/test-drivers/vivid/vivid-vid-out.c | 193 ++-- >> .../media/test-drivers/vivid/vivid-vid-out.h | 15 +- >> drivers/media/v4l2-core/v4l2-dev.c| 50 +- >> drivers/media/v4l2-core/v4l2-ioctl.c | 934 -- >> include/media/v4l2-ioctl.h| 60 ++ >> include/media/v4l2-mediabus.h | 42 + >> include/media/videobuf2-core.h| 6 +- >> include/media/videobuf2-v4l2.h| 21 +- >> include/uapi/linux/videodev2.h| 144 +++ >> 19 files changed, 1650 insertions(+), 718 deletions(-) >> >
Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
Hello Helen, Just a few drive-by comments. On Fri, 17 Jul 2020 08:54:29 -0300 Helen Koike wrote: > Hi, > > I'm sorry for taking too long to submit v4. > > It is not perfect, not all v4l2-compliance tests passes, but I'd like a > review, > specially on the API and potential problems, so I can focus on improving > implementation > and maybe drop the RFC tag for next version. > > Follow below what changed in v4 and some items I'd like to discuss: > > > * Ioctl to replace v4l2_pix_format > - > During last media summit, we agreed to create ioctls that replace the > v4l2_pix_format > struct and leave the other structs in the v4l2_format union alone. > Thus I refactored the code to receive struct v4l2_ext_pix_format, and I > renamed the > ioctls, so now we have: > > int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); Maybe use the EXT_PIX_FMT suffix here since the struct is really only about pixel formats. > int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); > int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); > > The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and > V4L2_BUF_TYPE_VIDEO_OUTPUT, > all the other types are invalid with this API. > [...] > > > Boris Brezillon (5): > media: v4l2: Extend pixel formats to unify single/multi-planar > handling (and more) > media: videobuf2: Expose helpers to implement the _ext_fmt and > _ext_buf hooks > media: mediabus: Add helpers to convert a ext_pix format to/from a > mbus_fmt > media: vivid: Convert the capture and output drivers to > EXT_FMT/EXT_BUF > media: vimc: Implement the ext_fmt and ext_buf hooks I think you should take ownership of these patches. The end result is likely to be completely different from what I initially posted, and you're the one doing the hard work here. > > Hans Verkuil (1): > media: v4l2: Add extended buffer operations > > .../media/common/videobuf2/videobuf2-core.c | 2 + > .../media/common/videobuf2/videobuf2-v4l2.c | 549 +- > .../media/test-drivers/vimc/vimc-capture.c| 61 +- > drivers/media/test-drivers/vimc/vimc-common.c | 6 +- > drivers/media/test-drivers/vimc/vimc-common.h | 2 +- > drivers/media/test-drivers/vivid/vivid-core.c | 70 +- > .../test-drivers/vivid/vivid-touch-cap.c | 26 +- > .../test-drivers/vivid/vivid-touch-cap.h | 3 +- > .../media/test-drivers/vivid/vivid-vid-cap.c | 169 +--- > .../media/test-drivers/vivid/vivid-vid-cap.h | 15 +- > .../media/test-drivers/vivid/vivid-vid-out.c | 193 ++-- > .../media/test-drivers/vivid/vivid-vid-out.h | 15 +- > drivers/media/v4l2-core/v4l2-dev.c| 50 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 934 -- > include/media/v4l2-ioctl.h| 60 ++ > include/media/v4l2-mediabus.h | 42 + > include/media/videobuf2-core.h| 6 +- > include/media/videobuf2-v4l2.h| 21 +- > include/uapi/linux/videodev2.h| 144 +++ > 19 files changed, 1650 insertions(+), 718 deletions(-) >
Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
On 17/07/2020 13:54, Helen Koike wrote: > Hi, > > I'm sorry for taking too long to submit v4. > > It is not perfect, not all v4l2-compliance tests passes, but I'd like a > review, > specially on the API and potential problems, so I can focus on improving > implementation > and maybe drop the RFC tag for next version. > > Follow below what changed in v4 and some items I'd like to discuss: > > > * Ioctl to replace v4l2_pix_format > - > During last media summit, we agreed to create ioctls that replace the > v4l2_pix_format > struct and leave the other structs in the v4l2_format union alone. > Thus I refactored the code to receive struct v4l2_ext_pix_format, and I > renamed the > ioctls, so now we have: > > int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); > int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); > int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); > > The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and > V4L2_BUF_TYPE_VIDEO_OUTPUT, > all the other types are invalid with this API. > > > * Modifiers > - > I understand that unifying DRM and V4L2 pixel formats is not possible, but > I'd like > to unify the modifiers [1]. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290 > > Should we use the DRM modifiers directly in the V4L2 API? For now, yes. Most of the modifier work is done in DRM, it is only fairly recent that the media subsystem starts to have a need for it. So for now just use the drm header and prefixes. > Or should we move this header to a common place and change the prefix? (which > requires > us to sync with DRM community). > Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping to > DRM_ > macros? > > For now, patch 1/6 includes drm/drm_fourcc.h and it is using DRM_FORMAT_MOD_* > > As discussed before, It would be nice to have documentation describing DRM > fourcc > equivalents (I'm not sure if someone started this already), listing the > number of > planes per format. > > We should also document which pixelformats are valid for the EXT_API, since > multiplanar > and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to > DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) should > be > replaced by a modifier. > > Regarding flags [2] field in struct v4l2_pix_format_mplane [3]: > The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only used > by vsp1 driver. > Which I believe could be replaced by a modifier, to avoid another field that > changes > pixel formats, so I removed it from the EXT API (we can always add it back > later with > the reserved fields). The colorspace series that Dafna is working on will add a V4L2_PIX_FMT_FLAG_SET_CSC flag, so this flags field will be needed. > > [2] > https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags > [3] > https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane > > We also discussed to add a new ENUM_FMT_EXT ioctl to return all pixelformats > + modifiers > combinations. I still didn't add it in this version, but I don't think it > affects > what is in this RFC and it can be added later. > > > * Buffers/Plane offset > - > > My understanding is that inside a memory buffer we can have multiple planes > in random > offsets. > I was comparing with the DRM API [4], where it can have the same dmabuf for > multiple > planes in different offsets, and I started to think we could simplify our > API, so > I took the liberty to do some more changes, please review struct > v4l2_ext_plane in > this RFC. > > I removed the data_offset, since it is unused (See Laurent's RFC repurposing > this > field [5]). And comparing to the DRM API, it seems to me we only need a > single offset > field. > > We could also check about overlapping planes in a memory buffer, but this is > complicated > if we use the same memory buffer with different v4l2_ext_buffer objects. We > can also leave > to the driver to check situations that may cause HW errors. > > [4] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_mode.h#n489 > [5] https://patchwork.linuxtv.org/patch/29177/ > > > * Multistream Channels > - > During last media summit, we discussed about adding a channel number to the > API to > support multistreams. i.e, to have multiple queues through a single video > node. > > Use cases: > > - Blitters: can take multiple
Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
On 7/20/20 8:57 AM, Tomasz Figa wrote: > Hi Helen, > > On Fri, Jul 17, 2020 at 1:55 PM Helen Koike wrote: >> >> Hi, >> >> I'm sorry for taking too long to submit v4. >> >> It is not perfect, not all v4l2-compliance tests passes, but I'd like a >> review, >> specially on the API and potential problems, so I can focus on improving >> implementation >> and maybe drop the RFC tag for next version. >> >> Follow below what changed in v4 and some items I'd like to discuss: >> >> >> * Ioctl to replace v4l2_pix_format >> - >> During last media summit, we agreed to create ioctls that replace the >> v4l2_pix_format >> struct and leave the other structs in the v4l2_format union alone. >> Thus I refactored the code to receive struct v4l2_ext_pix_format, and I >> renamed the >> ioctls, so now we have: >> >> int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); >> int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); >> int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); >> >> The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and >> V4L2_BUF_TYPE_VIDEO_OUTPUT, >> all the other types are invalid with this API. >> >> >> * Modifiers >> - >> I understand that unifying DRM and V4L2 pixel formats is not possible, but >> I'd like >> to unify the modifiers [1]. >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290 >> >> Should we use the DRM modifiers directly in the V4L2 API? >> Or should we move this header to a common place and change the prefix? >> (which requires >> us to sync with DRM community). >> Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping to >> DRM_ >> macros? >> >> For now, patch 1/6 includes drm/drm_fourcc.h and it is using DRM_FORMAT_MOD_* >> >> As discussed before, It would be nice to have documentation describing DRM >> fourcc >> equivalents (I'm not sure if someone started this already), listing the >> number of >> planes per format. >> >> We should also document which pixelformats are valid for the EXT_API, since >> multiplanar >> and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to >> DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) >> should be >> replaced by a modifier. >> >> Regarding flags [2] field in struct v4l2_pix_format_mplane [3]: >> The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only used >> by vsp1 driver. >> Which I believe could be replaced by a modifier, to avoid another field that >> changes >> pixel formats, so I removed it from the EXT API (we can always add it back >> later with >> the reserved fields). >> >> [2] >> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags >> [3] >> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane >> >> We also discussed to add a new ENUM_FMT_EXT ioctl to return all pixelformats >> + modifiers >> combinations. I still didn't add it in this version, but I don't think it >> affects >> what is in this RFC and it can be added later. >> >> >> * Buffers/Plane offset >> - >> >> My understanding is that inside a memory buffer we can have multiple planes >> in random >> offsets. >> I was comparing with the DRM API [4], where it can have the same dmabuf for >> multiple >> planes in different offsets, and I started to think we could simplify our >> API, so >> I took the liberty to do some more changes, please review struct >> v4l2_ext_plane in >> this RFC. >> >> I removed the data_offset, since it is unused (See Laurent's RFC repurposing >> this >> field [5]). And comparing to the DRM API, it seems to me we only need a >> single offset >> field. >> >> We could also check about overlapping planes in a memory buffer, but this is >> complicated >> if we use the same memory buffer with different v4l2_ext_buffer objects. We >> can also leave >> to the driver to check situations that may cause HW errors. >> >> [4] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_mode.h#n489 >> [5] https://patchwork.linuxtv.org/patch/29177/ >> >> >> * Multistream Channels >> - >> During last media summit, we discussed about adding a channel number to the >> API to >> support multistreams. i.e, to have multiple queues through a single video >> node. >> >> Use cases: >> >> - Blitters: can take multiple streams as input, which would require >> multiple OUTPUT queues. >> >> As Nicolas was explaining me: >> "The blitters comes with a lot
Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
Hi Helen, On Fri, Jul 17, 2020 at 1:55 PM Helen Koike wrote: > > Hi, > > I'm sorry for taking too long to submit v4. > > It is not perfect, not all v4l2-compliance tests passes, but I'd like a > review, > specially on the API and potential problems, so I can focus on improving > implementation > and maybe drop the RFC tag for next version. > > Follow below what changed in v4 and some items I'd like to discuss: > > > * Ioctl to replace v4l2_pix_format > - > During last media summit, we agreed to create ioctls that replace the > v4l2_pix_format > struct and leave the other structs in the v4l2_format union alone. > Thus I refactored the code to receive struct v4l2_ext_pix_format, and I > renamed the > ioctls, so now we have: > > int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); > int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); > int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); > > The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and > V4L2_BUF_TYPE_VIDEO_OUTPUT, > all the other types are invalid with this API. > > > * Modifiers > - > I understand that unifying DRM and V4L2 pixel formats is not possible, but > I'd like > to unify the modifiers [1]. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290 > > Should we use the DRM modifiers directly in the V4L2 API? > Or should we move this header to a common place and change the prefix? (which > requires > us to sync with DRM community). > Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping to > DRM_ > macros? > > For now, patch 1/6 includes drm/drm_fourcc.h and it is using DRM_FORMAT_MOD_* > > As discussed before, It would be nice to have documentation describing DRM > fourcc > equivalents (I'm not sure if someone started this already), listing the > number of > planes per format. > > We should also document which pixelformats are valid for the EXT_API, since > multiplanar > and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to > DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) should > be > replaced by a modifier. > > Regarding flags [2] field in struct v4l2_pix_format_mplane [3]: > The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only used > by vsp1 driver. > Which I believe could be replaced by a modifier, to avoid another field that > changes > pixel formats, so I removed it from the EXT API (we can always add it back > later with > the reserved fields). > > [2] > https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags > [3] > https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane > > We also discussed to add a new ENUM_FMT_EXT ioctl to return all pixelformats > + modifiers > combinations. I still didn't add it in this version, but I don't think it > affects > what is in this RFC and it can be added later. > > > * Buffers/Plane offset > - > > My understanding is that inside a memory buffer we can have multiple planes > in random > offsets. > I was comparing with the DRM API [4], where it can have the same dmabuf for > multiple > planes in different offsets, and I started to think we could simplify our > API, so > I took the liberty to do some more changes, please review struct > v4l2_ext_plane in > this RFC. > > I removed the data_offset, since it is unused (See Laurent's RFC repurposing > this > field [5]). And comparing to the DRM API, it seems to me we only need a > single offset > field. > > We could also check about overlapping planes in a memory buffer, but this is > complicated > if we use the same memory buffer with different v4l2_ext_buffer objects. We > can also leave > to the driver to check situations that may cause HW errors. > > [4] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_mode.h#n489 > [5] https://patchwork.linuxtv.org/patch/29177/ > > > * Multistream Channels > - > During last media summit, we discussed about adding a channel number to the > API to > support multistreams. i.e, to have multiple queues through a single video > node. > > Use cases: > > - Blitters: can take multiple streams as input, which would require > multiple OUTPUT queues. > > As Nicolas was explaining me: > "The blitters comes with a lot of variation between hardware. Most > blitters at > least support 3 frames buffer. 2 inputs and one output. The second input > is usually > optional, as the output
[PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls
Hi, I'm sorry for taking too long to submit v4. It is not perfect, not all v4l2-compliance tests passes, but I'd like a review, specially on the API and potential problems, so I can focus on improving implementation and maybe drop the RFC tag for next version. Follow below what changed in v4 and some items I'd like to discuss: * Ioctl to replace v4l2_pix_format - During last media summit, we agreed to create ioctls that replace the v4l2_pix_format struct and leave the other structs in the v4l2_format union alone. Thus I refactored the code to receive struct v4l2_ext_pix_format, and I renamed the ioctls, so now we have: int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp); int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp); int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp); The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and V4L2_BUF_TYPE_VIDEO_OUTPUT, all the other types are invalid with this API. * Modifiers - I understand that unifying DRM and V4L2 pixel formats is not possible, but I'd like to unify the modifiers [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290 Should we use the DRM modifiers directly in the V4L2 API? Or should we move this header to a common place and change the prefix? (which requires us to sync with DRM community). Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping to DRM_ macros? For now, patch 1/6 includes drm/drm_fourcc.h and it is using DRM_FORMAT_MOD_* As discussed before, It would be nice to have documentation describing DRM fourcc equivalents (I'm not sure if someone started this already), listing the number of planes per format. We should also document which pixelformats are valid for the EXT_API, since multiplanar and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) should be replaced by a modifier. Regarding flags [2] field in struct v4l2_pix_format_mplane [3]: The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only used by vsp1 driver. Which I believe could be replaced by a modifier, to avoid another field that changes pixel formats, so I removed it from the EXT API (we can always add it back later with the reserved fields). [2] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags [3] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane We also discussed to add a new ENUM_FMT_EXT ioctl to return all pixelformats + modifiers combinations. I still didn't add it in this version, but I don't think it affects what is in this RFC and it can be added later. * Buffers/Plane offset - My understanding is that inside a memory buffer we can have multiple planes in random offsets. I was comparing with the DRM API [4], where it can have the same dmabuf for multiple planes in different offsets, and I started to think we could simplify our API, so I took the liberty to do some more changes, please review struct v4l2_ext_plane in this RFC. I removed the data_offset, since it is unused (See Laurent's RFC repurposing this field [5]). And comparing to the DRM API, it seems to me we only need a single offset field. We could also check about overlapping planes in a memory buffer, but this is complicated if we use the same memory buffer with different v4l2_ext_buffer objects. We can also leave to the driver to check situations that may cause HW errors. [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_mode.h#n489 [5] https://patchwork.linuxtv.org/patch/29177/ * Multistream Channels - During last media summit, we discussed about adding a channel number to the API to support multistreams. i.e, to have multiple queues through a single video node. Use cases: - Blitters: can take multiple streams as input, which would require multiple OUTPUT queues. As Nicolas was explaining me: "The blitters comes with a lot of variation between hardware. Most blitters at least support 3 frames buffer. 2 inputs and one output. The second input is usually optional, as the output buffer data is not always overwritten (e.g. SRC_OVER blend or 1 input). Some of them have additional solid color or pattern that can be used too. Advanced blitters will have composition feature, and may support more input buffers to reduce the added latency that would be normally done through
