Re: [PATCH v5] media: imx: add mem2mem device
Le mardi 08 janvier 2019 à 16:30 +0100, Philipp Zabel a écrit :
> > # To demonstrate (with patched gst-plugins-bad
> > https://paste.fedoraproject.org/paste/rs-CbEq7coL4XSKrnWpEDw)
> > gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
> > video/x-raw,format=xRGB ! kmssink
>
> Is this with an old kernel? Since c525350f6ed0 ("media: imx: use well
> defined 32-bit RGB pixel format") that command line should make this
> select V4L2_PIX_FMT_XRGB32 ("BX24").
You can now discard this report. Starting from 5.0-rc2, this conversion
works.
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v5] media: imx: add mem2mem device
Le mardi 08 janvier 2019 à 16:30 +0100, Philipp Zabel a écrit :
> Hi Nicolas,
>
> On Mon, 2019-01-07 at 17:36 -0500, Nicolas Dufresne wrote:
> > Le lundi 03 décembre 2018 à 12:48 +0100, Philipp Zabel a écrit :
> > > Add a single imx-media mem2mem video device that uses the IPU IC PP
> > > (image converter post processing) task for scaling and colorspace
> > > conversion.
> > > On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
> > >
> > > The hardware only supports writing to destination buffers up to
> > > 1024x1024 pixels in a single pass, arbitrary sizes can be achieved
> > > by rendering multiple tiles per frame.
> >
> > While testing this driver, I found that the color conversion from YUYV
> > to BGR32 is broken.
>
> Thank you for testing, do you mean V4L2_PIX_FMT_RGB32?
>
> V4L2_PIX_FMT_BGR32 is still contained in the ipu_rgb_formats array in
> imx-media-utils, but happens to be never returned by enum_fmt since that
> already stops at the bayer formats.
>
> > Our test showed that the output of the m2m driver
> > is in fact RGBX/, a format that does not yet exist in V4L2
> > interface but that is supported by the imx-drm driver. This was tested
> > with GStreamer (master of gst-plugins-good), though some changes to
> > gst-plugins-bad is needed to add the missing format to kmssink. Let me
> > know if you need this to produce or not.
> >
> > # To demonstrate (with patched gst-plugins-bad
> > https://paste.fedoraproject.org/paste/rs-CbEq7coL4XSKrnWpEDw)
> > gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
> > video/x-raw,format=xRGB ! kmssink
>
> Is this with an old kernel? Since c525350f6ed0 ("media: imx: use well
> defined 32-bit RGB pixel format") that command line should make this
> select V4L2_PIX_FMT_XRGB32 ("BX24").
My testing is done with a slightly older kernel yes, but my colleagues
do reproduce on something more up to date. I should be updating the
test kernel soon, just need to find the time. As the pixel format
produced does not exist in v4l2 headers, the problem should still be
present for you on the most recent kernels.
Ideally I should produce a manual test of all conversion combination
(like I did for Exynos FIMC). This is fairly easy with GStreamer, I
just need to find the time slot to do so. Though, I thought this was
was rather important as the capture often produce YUYV and RGB can be
useful.
>
> > # Software fix for the color format produced
> > gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
> > video/x-raw,format=xRGB ! capssetter replace=0
> > caps="video/x-raw,format=RGBx" ! kmssink -v
> >
> > Also, BGR32 is deprecated and should not be used, this is mapped by
> > imx_media_enum_format() which I believe is already upstream. If that
> > is, this bug is just inherited from that helper.
>
> regards
> Philipp
Re: [PATCH v5] media: imx: add mem2mem device
Hi Hans,
On Wed, 2018-12-05 at 19:50 +0100, Hans Verkuil wrote:
> On 12/05/2018 02:20 AM, Steve Longerbeam wrote:
> > Hi Hans, Philipp,
> >
> > One comment on my side...
> >
> > On 12/3/18 7:21 AM, Hans Verkuil wrote:
> > >
> > > > +void imx_media_mem2mem_device_unregister(struct imx_media_video_dev
> > > > *vdev)
> > > > +{
> > > > + struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
> > > > + struct video_device *vfd = priv->vdev.vfd;
> > > > +
> > > > + mutex_lock(&priv->mutex);
> > > > +
> > > > + if (video_is_registered(vfd)) {
> > > > + video_unregister_device(vfd);
> > > > + media_entity_cleanup(&vfd->entity);
> > >
> > > Is this needed?
> > >
> > > If this is to be part of the media controller, then I expect to see a call
> > > to v4l2_m2m_register_media_controller() somewhere.
> > >
> >
> > Yes, I agree there should be a call to
> > v4l2_m2m_register_media_controller(). This driver does not connect with
> > any of the imx-media entities, but calling it will at least make the
> > mem2mem output/capture device entities (and processing entity) visible
> > in the media graph.
> >
> > Philipp, can you pick/squash the following from my media-tree github fork?
> >
> > 6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
> > d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
> > device")
> > 6787a50cdc ("media: imx: mem2mem: Register with media control")
> >
> > Steve
> >
>
> Why is this driver part of the imx driver? Since it doesn't connect with
> any of the imx-media entities, doesn't that mean that this is really a
> stand-alone driver?
It is part of the same hardware unit. The mem2mem tasks are scheduled to
the same IC that is also used for scaling in the capture path.
Since the mem2mem driver is at a higher abstraction level, it would be
possible to split it out into a separate platform device, but that felt
a bit artificial, and it would have to be registered from imx-media-dev
anyway.
regards
Philipp
Re: [PATCH v5] media: imx: add mem2mem device
Hi Steve,
On Tue, 2018-12-04 at 17:20 -0800, Steve Longerbeam wrote:
> Hi Hans, Philipp,
>
> One comment on my side...
>
> On 12/3/18 7:21 AM, Hans Verkuil wrote:
> >
> > > +void imx_media_mem2mem_device_unregister(struct imx_media_video_dev
> > > *vdev)
> > > +{
> > > + struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
> > > + struct video_device *vfd = priv->vdev.vfd;
> > > +
> > > + mutex_lock(&priv->mutex);
> > > +
> > > + if (video_is_registered(vfd)) {
> > > + video_unregister_device(vfd);
> > > + media_entity_cleanup(&vfd->entity);
> >
> > Is this needed?
> >
> > If this is to be part of the media controller, then I expect to see a call
> > to v4l2_m2m_register_media_controller() somewhere.
>
> Yes, I agree there should be a call to
> v4l2_m2m_register_media_controller(). This driver does not connect with
> any of the imx-media entities, but calling it will at least make the
> mem2mem output/capture device entities (and processing entity) visible
> in the media graph.
>
> Philipp, can you pick/squash the following from my media-tree github fork?
>
> 6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
> d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
> device")
Thank you. I have squashed those two.
> 6787a50cdc ("media: imx: mem2mem: Register with media control")
I've left this one out for now.
regards
Philipp
Re: [PATCH v5] media: imx: add mem2mem device
Hi Hans,
thank you for your comments!
On Mon, 2018-12-03 at 16:21 +0100, Hans Verkuil wrote:
> On 12/03/2018 12:48 PM, Philipp Zabel wrote:
> > Add a single imx-media mem2mem video device that uses the IPU IC PP
> > (image converter post processing) task for scaling and colorspace
> > conversion.
> > On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
> >
> > The hardware only supports writing to destination buffers up to
> > 1024x1024 pixels in a single pass, arbitrary sizes can be achieved
> > by rendering multiple tiles per frame.
> >
> > Signed-off-by: Philipp Zabel
> > [slongerb...@gmail.com: use ipu_image_convert_adjust(), fix
> > device_run() error handling]
> > Signed-off-by: Steve Longerbeam
> > ---
> > Changes since v4:
> > - No functional changes.
> > - Dropped deprecated TODO comment. This driver has no interaction with
> >the IC task v4l2 subdevices.
> > - Dropped ipu-v3 patches, those are merged independently via imx-drm.
>
> These land in kernel 4.21? Or are they already in 4.20?
These landed in v5.0-rc1.
I've sent a v6 that should already address most of the issues you
mentioned, except those I answer to below:
[...]
> > +static int mem2mem_queue_setup(struct vb2_queue *vq, unsigned int
> > *nbuffers,
> > + unsigned int *nplanes, unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct mem2mem_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct mem2mem_q_data *q_data;
> > + unsigned int count = *nbuffers;
> > + struct v4l2_pix_format *pix;
> > +
> > + q_data = get_q_data(ctx, vq->type);
> > + pix = &q_data->cur_fmt;
> > +
> > + *nplanes = 1;
> > + *nbuffers = count;
> > + sizes[0] = pix->sizeimage;
>
> This needs to be modified slightly to support PREPARE_BUF.
Do you mean CREATE_BUFS? I have fixed queue_setup to do check the size
when called from CREATE_BUFS, but I don't understand what needs to be
changed for PREPARE_BUF.
[...]
> > + ret = ipu_degrees_to_rot_mode(&rot_mode, rotate, hflip, vflip);
> > + if (ret)
> > + return ret;
> > +
> > + if (rot_mode != ctx->rot_mode) {
> > + struct vb2_queue *cap_q;
> > +
> > + cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > + V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > + if (vb2_is_streaming(cap_q))
> > + return -EBUSY;
> > +
> > + ctx->rot_mode = rot_mode;
> > + ctx->rotate = rotate;
> > + ctx->hflip = hflip;
> > + ctx->vflip = vflip;
>
> Changing the orientation should also change the format (swaps width and
> height), but I see no sign of that in the code.
A +/-90° orientation change (in addition to changing rot_mode) should
have the same effect as a S_FMT with swapped width/height?
I was unsure about what the "display window" meant in the
V4L2_CID_ROTATE description [1], but the wording sounded like the user
was expected to call S_FMT manually anyway:
V4L2_CID_ROTATE (integer)
Rotates the image by specified angle.
Common angles are 90, 270 and
180. Rotating the image to 90 and 270
will reverse the height and
width of the display window. It is
necessary to set the new height
and width of the picture using the
VIDIOC_S_FMT ioctl according to
the rotation angle selected.
[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/control.html
> Rotating also changes the buffer size for YUV 4:2:0, so you probably should
> use vb2_is_busy() in the test above.
>
> There is another issue involved. See this old patch (never got merged):
>
> https://patchwork.linuxtv.org/patch/44424/
>
> I think this was discussed as well on irc not too long ago, along there I
> suggested calling the ops queue_prepare and queue_finish.
Do you mean the V4L2_CID_ROTATE should be grabbed while the queue is
busy?
regards
Philipp
Re: [PATCH v5] media: imx: add mem2mem device
Hi Nicolas,
On Mon, 2019-01-07 at 17:36 -0500, Nicolas Dufresne wrote:
> Le lundi 03 décembre 2018 à 12:48 +0100, Philipp Zabel a écrit :
> > Add a single imx-media mem2mem video device that uses the IPU IC PP
> > (image converter post processing) task for scaling and colorspace
> > conversion.
> > On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
> >
> > The hardware only supports writing to destination buffers up to
> > 1024x1024 pixels in a single pass, arbitrary sizes can be achieved
> > by rendering multiple tiles per frame.
>
> While testing this driver, I found that the color conversion from YUYV
> to BGR32 is broken.
Thank you for testing, do you mean V4L2_PIX_FMT_RGB32?
V4L2_PIX_FMT_BGR32 is still contained in the ipu_rgb_formats array in
imx-media-utils, but happens to be never returned by enum_fmt since that
already stops at the bayer formats.
> Our test showed that the output of the m2m driver
> is in fact RGBX/, a format that does not yet exist in V4L2
> interface but that is supported by the imx-drm driver. This was tested
> with GStreamer (master of gst-plugins-good), though some changes to
> gst-plugins-bad is needed to add the missing format to kmssink. Let me
> know if you need this to produce or not.
>
> # To demonstrate (with patched gst-plugins-bad
> https://paste.fedoraproject.org/paste/rs-CbEq7coL4XSKrnWpEDw)
> gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
> video/x-raw,format=xRGB ! kmssink
Is this with an old kernel? Since c525350f6ed0 ("media: imx: use well
defined 32-bit RGB pixel format") that command line should make this
select V4L2_PIX_FMT_XRGB32 ("BX24").
> # Software fix for the color format produced
> gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
> video/x-raw,format=xRGB ! capssetter replace=0 caps="video/x-raw,format=RGBx"
> ! kmssink -v
>
> Also, BGR32 is deprecated and should not be used, this is mapped by
> imx_media_enum_format() which I believe is already upstream. If that
> is, this bug is just inherited from that helper.
regards
Philipp
Re: [PATCH v5] media: imx: add mem2mem device
Le lundi 03 décembre 2018 à 12:48 +0100, Philipp Zabel a écrit :
> Add a single imx-media mem2mem video device that uses the IPU IC PP
> (image converter post processing) task for scaling and colorspace
> conversion.
> On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
>
> The hardware only supports writing to destination buffers up to
> 1024x1024 pixels in a single pass, arbitrary sizes can be achieved
> by rendering multiple tiles per frame.
While testing this driver, I found that the color conversion from YUYV
to BGR32 is broken. Our test showed that the output of the m2m driver
is in fact RGBX/, a format that does not yet exist in V4L2
interface but that is supported by the imx-drm driver. This was tested
with GStreamer (master of gst-plugins-good), though some changes to
gst-plugins-bad is needed to add the missing format to kmssink. Let me
know if you need this to produce or not.
# To demonstrate (with patched gst-plugins-bad
https://paste.fedoraproject.org/paste/rs-CbEq7coL4XSKrnWpEDw)
gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
video/x-raw,format=xRGB ! kmssink
# Software fix for the color format produced
gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! v4l2convert !
video/x-raw,format=xRGB ! capssetter replace=0 caps="video/x-raw,format=RGBx" !
kmssink -v
Also, BGR32 is deprecated and should not be used, this is mapped by
imx_media_enum_format() which I believe is already upstream. If that
is, this bug is just inherited from that helper.
>
> Signed-off-by: Philipp Zabel
> [slongerb...@gmail.com: use ipu_image_convert_adjust(), fix
> device_run() error handling]
> Signed-off-by: Steve Longerbeam
> ---
> Changes since v4:
> - No functional changes.
> - Dropped deprecated TODO comment. This driver has no interaction with
>the IC task v4l2 subdevices.
> - Dropped ipu-v3 patches, those are merged independently via imx-drm.
> ---
> drivers/staging/media/imx/Kconfig | 1 +
> drivers/staging/media/imx/Makefile| 1 +
> drivers/staging/media/imx/imx-media-dev.c | 10 +
> drivers/staging/media/imx/imx-media-mem2mem.c | 873 ++
> drivers/staging/media/imx/imx-media.h | 10 +
> 5 files changed, 895 insertions(+)
> create mode 100644 drivers/staging/media/imx/imx-media-mem2mem.c
>
> diff --git a/drivers/staging/media/imx/Kconfig
> b/drivers/staging/media/imx/Kconfig
> index bfc17de56b17..07013cb3cb66 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -6,6 +6,7 @@ config VIDEO_IMX_MEDIA
> depends on HAS_DMA
> select VIDEOBUF2_DMA_CONTIG
> select V4L2_FWNODE
> + select V4L2_MEM2MEM_DEV
> ---help---
> Say yes here to enable support for video4linux media controller
> driver for the i.MX5/6 SOC.
> diff --git a/drivers/staging/media/imx/Makefile
> b/drivers/staging/media/imx/Makefile
> index 698a4210316e..f2e722d0fa19 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -6,6 +6,7 @@ imx-media-ic-objs := imx-ic-common.o imx-ic-prp.o
> imx-ic-prpencvf.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-capture.o
> +obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-mem2mem.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-vdic.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-ic.o
>
> diff --git a/drivers/staging/media/imx/imx-media-dev.c
> b/drivers/staging/media/imx/imx-media-dev.c
> index 4b344a4a3706..0376b52cb784 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -318,6 +318,16 @@ static int imx_media_probe_complete(struct
> v4l2_async_notifier *notifier)
> goto unlock;
>
> ret = v4l2_device_register_subdev_nodes(&imxmd->v4l2_dev);
> + if (ret)
> + goto unlock;
> +
> + imxmd->m2m_vdev = imx_media_mem2mem_device_init(imxmd);
> + if (IS_ERR(imxmd->m2m_vdev)) {
> + ret = PTR_ERR(imxmd->m2m_vdev);
> + goto unlock;
> + }
> +
> + ret = imx_media_mem2mem_device_register(imxmd->m2m_vdev);
> unlock:
> mutex_unlock(&imxmd->mutex);
> if (ret)
> diff --git a/drivers/staging/media/imx/imx-media-mem2mem.c
> b/drivers/staging/media/imx/imx-media-mem2mem.c
> new file mode 100644
> index ..a2a4dca017ce
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-mem2mem.c
> @@ -0,0 +1,873 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * i.MX IPUv3 mem2mem Scaler/CSC driver
> + *
> + * Copyright (C) 2011 Pengutronix, Sascha Hauer
> + * Copyright (C) 2018 Pengutronix, Philipp Zabel
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "imx-media.h
Re: [PATCH v5] media: imx: add mem2mem device
Hi Hans,
On 12/6/18 4:32 AM, Hans Verkuil wrote:
On 12/06/18 00:13, Steve Longerbeam wrote:
On 12/5/18 10:50 AM, Hans Verkuil wrote:
On 12/05/2018 02:20 AM, Steve Longerbeam wrote:
Hi Hans, Philipp,
One comment on my side...
On 12/3/18 7:21 AM, Hans Verkuil wrote:
+void imx_media_mem2mem_device_unregister(struct imx_media_video_dev *vdev)
+{
+ struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
+ struct video_device *vfd = priv->vdev.vfd;
+
+ mutex_lock(&priv->mutex);
+
+ if (video_is_registered(vfd)) {
+ video_unregister_device(vfd);
+ media_entity_cleanup(&vfd->entity);
Is this needed?
If this is to be part of the media controller, then I expect to see a call
to v4l2_m2m_register_media_controller() somewhere.
Yes, I agree there should be a call to
v4l2_m2m_register_media_controller(). This driver does not connect with
any of the imx-media entities, but calling it will at least make the
mem2mem output/capture device entities (and processing entity) visible
in the media graph.
Philipp, can you pick/squash the following from my media-tree github fork?
6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
device")
6787a50cdc ("media: imx: mem2mem: Register with media control")
Steve
Why is this driver part of the imx driver? Since it doesn't connect with
any of the imx-media entities, doesn't that mean that this is really a
stand-alone driver?
It is basically a stand-alone m2m driver, but it makes use of some
imx-media utility functions like imx_media_enum_format(). Also making it
a true stand-alone driver would require creating a second /dev/mediaN
device.
If it is standalone, is it reused in newer iMX versions? (7 or 8)
No, this driver makes use of the Image Converter in IPUv3, so it will
only run on iMX 5/6. The IPU has been dropped in iMX 7 and 8.
And if it is just a regular m2m device, then it doesn't need to create a
media device either (doesn't hurt, but it is not required).
Ok, I'll leave that up to Philipp. I don't mind either way whether it is
folded into imx-media device, or whether it is made stand-alone with or
without a new media device.
Steve
Re: [PATCH v5] media: imx: add mem2mem device
On 12/06/18 00:13, Steve Longerbeam wrote:
>
>
> On 12/5/18 10:50 AM, Hans Verkuil wrote:
>> On 12/05/2018 02:20 AM, Steve Longerbeam wrote:
>>> Hi Hans, Philipp,
>>>
>>> One comment on my side...
>>>
>>> On 12/3/18 7:21 AM, Hans Verkuil wrote:
> +void imx_media_mem2mem_device_unregister(struct imx_media_video_dev
> *vdev)
> +{
> + struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
> + struct video_device *vfd = priv->vdev.vfd;
> +
> + mutex_lock(&priv->mutex);
> +
> + if (video_is_registered(vfd)) {
> + video_unregister_device(vfd);
> + media_entity_cleanup(&vfd->entity);
Is this needed?
If this is to be part of the media controller, then I expect to see a call
to v4l2_m2m_register_media_controller() somewhere.
>>> Yes, I agree there should be a call to
>>> v4l2_m2m_register_media_controller(). This driver does not connect with
>>> any of the imx-media entities, but calling it will at least make the
>>> mem2mem output/capture device entities (and processing entity) visible
>>> in the media graph.
>>>
>>> Philipp, can you pick/squash the following from my media-tree github fork?
>>>
>>> 6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
>>> d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
>>> device")
>>> 6787a50cdc ("media: imx: mem2mem: Register with media control")
>>>
>>> Steve
>>>
>> Why is this driver part of the imx driver? Since it doesn't connect with
>> any of the imx-media entities, doesn't that mean that this is really a
>> stand-alone driver?
>
> It is basically a stand-alone m2m driver, but it makes use of some
> imx-media utility functions like imx_media_enum_format(). Also making it
> a true stand-alone driver would require creating a second /dev/mediaN
> device.
If it is standalone, is it reused in newer iMX versions? (7 or 8)
And if it is just a regular m2m device, then it doesn't need to create a
media device either (doesn't hurt, but it is not required).
Regards,
Hans
>
> Steve
>
Re: [PATCH v5] media: imx: add mem2mem device
On 12/5/18 10:50 AM, Hans Verkuil wrote:
On 12/05/2018 02:20 AM, Steve Longerbeam wrote:
Hi Hans, Philipp,
One comment on my side...
On 12/3/18 7:21 AM, Hans Verkuil wrote:
+void imx_media_mem2mem_device_unregister(struct imx_media_video_dev *vdev)
+{
+ struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
+ struct video_device *vfd = priv->vdev.vfd;
+
+ mutex_lock(&priv->mutex);
+
+ if (video_is_registered(vfd)) {
+ video_unregister_device(vfd);
+ media_entity_cleanup(&vfd->entity);
Is this needed?
If this is to be part of the media controller, then I expect to see a call
to v4l2_m2m_register_media_controller() somewhere.
Yes, I agree there should be a call to
v4l2_m2m_register_media_controller(). This driver does not connect with
any of the imx-media entities, but calling it will at least make the
mem2mem output/capture device entities (and processing entity) visible
in the media graph.
Philipp, can you pick/squash the following from my media-tree github fork?
6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
device")
6787a50cdc ("media: imx: mem2mem: Register with media control")
Steve
Why is this driver part of the imx driver? Since it doesn't connect with
any of the imx-media entities, doesn't that mean that this is really a
stand-alone driver?
It is basically a stand-alone m2m driver, but it makes use of some
imx-media utility functions like imx_media_enum_format(). Also making it
a true stand-alone driver would require creating a second /dev/mediaN
device.
Steve
Re: [PATCH v5] media: imx: add mem2mem device
On 12/05/2018 02:20 AM, Steve Longerbeam wrote:
> Hi Hans, Philipp,
>
> One comment on my side...
>
> On 12/3/18 7:21 AM, Hans Verkuil wrote:
>>
>>> +void imx_media_mem2mem_device_unregister(struct imx_media_video_dev *vdev)
>>> +{
>>> + struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
>>> + struct video_device *vfd = priv->vdev.vfd;
>>> +
>>> + mutex_lock(&priv->mutex);
>>> +
>>> + if (video_is_registered(vfd)) {
>>> + video_unregister_device(vfd);
>>> + media_entity_cleanup(&vfd->entity);
>> Is this needed?
>>
>> If this is to be part of the media controller, then I expect to see a call
>> to v4l2_m2m_register_media_controller() somewhere.
>>
>
> Yes, I agree there should be a call to
> v4l2_m2m_register_media_controller(). This driver does not connect with
> any of the imx-media entities, but calling it will at least make the
> mem2mem output/capture device entities (and processing entity) visible
> in the media graph.
>
> Philipp, can you pick/squash the following from my media-tree github fork?
>
> 6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
> d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
> device")
> 6787a50cdc ("media: imx: mem2mem: Register with media control")
>
> Steve
>
Why is this driver part of the imx driver? Since it doesn't connect with
any of the imx-media entities, doesn't that mean that this is really a
stand-alone driver?
Regards,
Hans
Re: [PATCH v5] media: imx: add mem2mem device
Hi Hans, Philipp,
One comment on my side...
On 12/3/18 7:21 AM, Hans Verkuil wrote:
+void imx_media_mem2mem_device_unregister(struct imx_media_video_dev *vdev)
+{
+ struct mem2mem_priv *priv = to_mem2mem_priv(vdev);
+ struct video_device *vfd = priv->vdev.vfd;
+
+ mutex_lock(&priv->mutex);
+
+ if (video_is_registered(vfd)) {
+ video_unregister_device(vfd);
+ media_entity_cleanup(&vfd->entity);
Is this needed?
If this is to be part of the media controller, then I expect to see a call
to v4l2_m2m_register_media_controller() somewhere.
Yes, I agree there should be a call to
v4l2_m2m_register_media_controller(). This driver does not connect with
any of the imx-media entities, but calling it will at least make the
mem2mem output/capture device entities (and processing entity) visible
in the media graph.
Philipp, can you pick/squash the following from my media-tree github fork?
6fa05f5170 ("media: imx: mem2mem: Add missing media-device header")
d355bf8b15 ("media: imx: Add missing unregister and remove of mem2mem
device")
6787a50cdc ("media: imx: mem2mem: Register with media control")
Steve
Re: [PATCH v5] media: imx: add mem2mem device
On 12/03/2018 12:48 PM, Philipp Zabel wrote:
> Add a single imx-media mem2mem video device that uses the IPU IC PP
> (image converter post processing) task for scaling and colorspace
> conversion.
> On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
>
> The hardware only supports writing to destination buffers up to
> 1024x1024 pixels in a single pass, arbitrary sizes can be achieved
> by rendering multiple tiles per frame.
>
> Signed-off-by: Philipp Zabel
> [slongerb...@gmail.com: use ipu_image_convert_adjust(), fix
> device_run() error handling]
> Signed-off-by: Steve Longerbeam
> ---
> Changes since v4:
> - No functional changes.
> - Dropped deprecated TODO comment. This driver has no interaction with
>the IC task v4l2 subdevices.
> - Dropped ipu-v3 patches, those are merged independently via imx-drm.
These land in kernel 4.21? Or are they already in 4.20?
> ---
> drivers/staging/media/imx/Kconfig | 1 +
> drivers/staging/media/imx/Makefile| 1 +
> drivers/staging/media/imx/imx-media-dev.c | 10 +
> drivers/staging/media/imx/imx-media-mem2mem.c | 873 ++
Please give this file a better name! imx-media-csc-scaler.c or something is
much more descriptive. Calling it mem2mem doesn't tell me what it actually does!
> drivers/staging/media/imx/imx-media.h | 10 +
> 5 files changed, 895 insertions(+)
> create mode 100644 drivers/staging/media/imx/imx-media-mem2mem.c
>
> diff --git a/drivers/staging/media/imx/Kconfig
> b/drivers/staging/media/imx/Kconfig
> index bfc17de56b17..07013cb3cb66 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -6,6 +6,7 @@ config VIDEO_IMX_MEDIA
> depends on HAS_DMA
> select VIDEOBUF2_DMA_CONTIG
> select V4L2_FWNODE
> + select V4L2_MEM2MEM_DEV
> ---help---
> Say yes here to enable support for video4linux media controller
> driver for the i.MX5/6 SOC.
> diff --git a/drivers/staging/media/imx/Makefile
> b/drivers/staging/media/imx/Makefile
> index 698a4210316e..f2e722d0fa19 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -6,6 +6,7 @@ imx-media-ic-objs := imx-ic-common.o imx-ic-prp.o
> imx-ic-prpencvf.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-capture.o
> +obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-mem2mem.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-vdic.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-ic.o
>
> diff --git a/drivers/staging/media/imx/imx-media-dev.c
> b/drivers/staging/media/imx/imx-media-dev.c
> index 4b344a4a3706..0376b52cb784 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -318,6 +318,16 @@ static int imx_media_probe_complete(struct
> v4l2_async_notifier *notifier)
> goto unlock;
>
> ret = v4l2_device_register_subdev_nodes(&imxmd->v4l2_dev);
> + if (ret)
> + goto unlock;
> +
> + imxmd->m2m_vdev = imx_media_mem2mem_device_init(imxmd);
> + if (IS_ERR(imxmd->m2m_vdev)) {
> + ret = PTR_ERR(imxmd->m2m_vdev);
> + goto unlock;
> + }
> +
> + ret = imx_media_mem2mem_device_register(imxmd->m2m_vdev);
> unlock:
> mutex_unlock(&imxmd->mutex);
> if (ret)
> diff --git a/drivers/staging/media/imx/imx-media-mem2mem.c
> b/drivers/staging/media/imx/imx-media-mem2mem.c
> new file mode 100644
> index ..a2a4dca017ce
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-mem2mem.c
> @@ -0,0 +1,873 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * i.MX IPUv3 mem2mem Scaler/CSC driver
> + *
> + * Copyright (C) 2011 Pengutronix, Sascha Hauer
> + * Copyright (C) 2018 Pengutronix, Philipp Zabel
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "imx-media.h"
> +
> +#define fh_to_ctx(__fh) container_of(__fh, struct mem2mem_ctx, fh)
> +
> +enum {
> + V4L2_M2M_SRC = 0,
> + V4L2_M2M_DST = 1,
> +};
> +
> +struct mem2mem_priv {
> + struct imx_media_video_dev vdev;
> +
> + struct v4l2_m2m_dev *m2m_dev;
> + struct device *dev;
> +
> + struct imx_media_dev *md;
> +
> + struct mutex mutex; /* mem2mem device mutex */
> +
> + atomic_t num_inst;
> +};
> +
> +#define to_mem2mem_priv(v) container_of(v, struct mem2mem_priv, vdev)
> +
> +/* Per-queue, driver-specific private data */
> +struct mem2mem_q_data {
> + struct v4l2_pix_format cur_fmt;
> + struct v4l2_rectrect;
> +};
> +
> +struct mem2mem_ctx {
> + struct mem2mem_priv *priv;
> +
> + struct v4l2_fh fh;
> + struct mem2mem_q_data q_data[2];
> + int
[PATCH v5] media: imx: add mem2mem device
Add a single imx-media mem2mem video device that uses the IPU IC PP
(image converter post processing) task for scaling and colorspace
conversion.
On i.MX6Q/DL SoCs with two IPUs currently only the first IPU is used.
The hardware only supports writing to destination buffers up to
1024x1024 pixels in a single pass, arbitrary sizes can be achieved
by rendering multiple tiles per frame.
Signed-off-by: Philipp Zabel
[slongerb...@gmail.com: use ipu_image_convert_adjust(), fix
device_run() error handling]
Signed-off-by: Steve Longerbeam
---
Changes since v4:
- No functional changes.
- Dropped deprecated TODO comment. This driver has no interaction with
the IC task v4l2 subdevices.
- Dropped ipu-v3 patches, those are merged independently via imx-drm.
---
drivers/staging/media/imx/Kconfig | 1 +
drivers/staging/media/imx/Makefile| 1 +
drivers/staging/media/imx/imx-media-dev.c | 10 +
drivers/staging/media/imx/imx-media-mem2mem.c | 873 ++
drivers/staging/media/imx/imx-media.h | 10 +
5 files changed, 895 insertions(+)
create mode 100644 drivers/staging/media/imx/imx-media-mem2mem.c
diff --git a/drivers/staging/media/imx/Kconfig
b/drivers/staging/media/imx/Kconfig
index bfc17de56b17..07013cb3cb66 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -6,6 +6,7 @@ config VIDEO_IMX_MEDIA
depends on HAS_DMA
select VIDEOBUF2_DMA_CONTIG
select V4L2_FWNODE
+ select V4L2_MEM2MEM_DEV
---help---
Say yes here to enable support for video4linux media controller
driver for the i.MX5/6 SOC.
diff --git a/drivers/staging/media/imx/Makefile
b/drivers/staging/media/imx/Makefile
index 698a4210316e..f2e722d0fa19 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -6,6 +6,7 @@ imx-media-ic-objs := imx-ic-common.o imx-ic-prp.o
imx-ic-prpencvf.o
obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-capture.o
+obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-mem2mem.o
obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-vdic.o
obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-ic.o
diff --git a/drivers/staging/media/imx/imx-media-dev.c
b/drivers/staging/media/imx/imx-media-dev.c
index 4b344a4a3706..0376b52cb784 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -318,6 +318,16 @@ static int imx_media_probe_complete(struct
v4l2_async_notifier *notifier)
goto unlock;
ret = v4l2_device_register_subdev_nodes(&imxmd->v4l2_dev);
+ if (ret)
+ goto unlock;
+
+ imxmd->m2m_vdev = imx_media_mem2mem_device_init(imxmd);
+ if (IS_ERR(imxmd->m2m_vdev)) {
+ ret = PTR_ERR(imxmd->m2m_vdev);
+ goto unlock;
+ }
+
+ ret = imx_media_mem2mem_device_register(imxmd->m2m_vdev);
unlock:
mutex_unlock(&imxmd->mutex);
if (ret)
diff --git a/drivers/staging/media/imx/imx-media-mem2mem.c
b/drivers/staging/media/imx/imx-media-mem2mem.c
new file mode 100644
index ..a2a4dca017ce
--- /dev/null
+++ b/drivers/staging/media/imx/imx-media-mem2mem.c
@@ -0,0 +1,873 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * i.MX IPUv3 mem2mem Scaler/CSC driver
+ *
+ * Copyright (C) 2011 Pengutronix, Sascha Hauer
+ * Copyright (C) 2018 Pengutronix, Philipp Zabel
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "imx-media.h"
+
+#define fh_to_ctx(__fh)container_of(__fh, struct mem2mem_ctx, fh)
+
+enum {
+ V4L2_M2M_SRC = 0,
+ V4L2_M2M_DST = 1,
+};
+
+struct mem2mem_priv {
+ struct imx_media_video_dev vdev;
+
+ struct v4l2_m2m_dev *m2m_dev;
+ struct device *dev;
+
+ struct imx_media_dev *md;
+
+ struct mutex mutex; /* mem2mem device mutex */
+
+ atomic_t num_inst;
+};
+
+#define to_mem2mem_priv(v) container_of(v, struct mem2mem_priv, vdev)
+
+/* Per-queue, driver-specific private data */
+struct mem2mem_q_data {
+ struct v4l2_pix_format cur_fmt;
+ struct v4l2_rectrect;
+};
+
+struct mem2mem_ctx {
+ struct mem2mem_priv *priv;
+
+ struct v4l2_fh fh;
+ struct mem2mem_q_data q_data[2];
+ int error;
+ struct ipu_image_convert_ctx *icc;
+
+ struct v4l2_ctrl_handler ctrl_hdlr;
+ int rotate;
+ bool hflip;
+ bool vflip;
+ enum ipu_rotate_moderot_mode;
+};
+
+static struct mem2mem_q_data *get_q_data(struct mem2mem_ctx *ctx,
+enum v4l2_buf_type type)
+{
+ if (V4L2_TYPE_IS_OUTPUT(type))
+ return &ctx->q_data[V4L2_M2M_SRC];
+ else
+ retur
