| Message ID | 20221020195533.114049-13-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 795ac295eacb291302b8951605677301c7938bd7 |
| Headers | show |
| Series | media: atomisp: Convert to videobuf2 | expand |
On Thu, Oct 20, 2022 at 09:55:28PM +0200, Hans de Goede wrote: > camorama calls VIDIOC_REQBUFS(V4L2_MEMORY_MMAP) to test if MMAP support > works (this was added specifically because of the previously broken > MMAP support in atomisp). > > Currently this fails because atomisp_get_css_frame_info() fails in this > case. Although it is weird to call VIDIOC_REQBUFS before VIDIOC_S_FMT, > it is allowed to do this. > > Fix this not working by doing a S_FMT to V4L2_PIX_FMT_YUV420 + the highest > supported resolution. > > Note this will cause camorama to use mmap mode, which means it will also > use libv4l2 to do format conversion. libv4l2 will pick V4L2_PIX_FMT_RGB565 > as input format and this will lead to a garbled video display. This is > a libv4lconvert bug, the RGB565 -> RGB24 path in libv4lconvert assumes > that stride == width which is not true on the atomisp. > > I've already send out a libv4lconvert fix for this. Also this can be worked Link: ? > around by passing --dont-use-libv4l2 as argument to camorama. ... > + struct v4l2_format f = { > + .fmt.pix.pixelformat = V4L2_PIX_FMT_YUV420, > + .fmt.pix.width = 10000, > + .fmt.pix.height = 10000, If it's just an arbitrary maximum, I would suggest to use definitions from limits.h, so it will better show the intention. > + };
Em Thu, 20 Oct 2022 21:55:28 +0200 Hans de Goede <hdegoede@redhat.com> escreveu: > camorama calls VIDIOC_REQBUFS(V4L2_MEMORY_MMAP) to test if MMAP support > works (this was added specifically because of the previously broken > MMAP support in atomisp). > > Currently this fails because atomisp_get_css_frame_info() fails in this > case. Although it is weird to call VIDIOC_REQBUFS before VIDIOC_S_FMT, > it is allowed to do this. > > Fix this not working by doing a S_FMT to V4L2_PIX_FMT_YUV420 + the highest > supported resolution. > > Note this will cause camorama to use mmap mode, which means it will also > use libv4l2 to do format conversion. libv4l2 will pick V4L2_PIX_FMT_RGB565 > as input format and this will lead to a garbled video display. This is > a libv4lconvert bug, the RGB565 -> RGB24 path in libv4lconvert assumes > that stride == width which is not true on the atomisp. Hmm... should we add a patch on Camorama for it to not use libv4l2 if format == V4L2_PIX_FMT_RGB565? > > I've already send out a libv4lconvert fix for this. Also this can be worked > around by passing --dont-use-libv4l2 as argument to camorama. Was the patch already applied? The better would be to apply it at libv4l2 upstream, as a fix, porting it to old versions, and mentioning on what versions this got fixed on this changeset. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../media/atomisp/pci/atomisp_compat_css20.c | 2 +- > .../staging/media/atomisp/pci/atomisp_fops.c | 25 +++++++++++++++++-- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c > index ea6114679c53..f282572d69da 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c > @@ -2688,7 +2688,7 @@ int atomisp_get_css_frame_info(struct atomisp_sub_device *asd, > > if (0 != ia_css_pipe_get_info(asd->stream_env[stream_index] > .pipes[pipe_index], &info)) { > - dev_err(isp->dev, "ia_css_pipe_get_info FAILED"); > + dev_dbg(isp->dev, "ia_css_pipe_get_info FAILED"); > return -EINVAL; > } > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c > index bc47a092a8af..f539df09ebd9 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c > @@ -50,15 +50,36 @@ static int atomisp_queue_setup(struct vb2_queue *vq, > u16 source_pad = atomisp_subdev_source_pad(&pipe->vdev); > int ret; > > + mutex_lock(&pipe->asd->isp->mutex); /* for get_css_frame_info() / set_fmt() */ > + > + /* > + * When VIDIOC_S_FMT has not been called before VIDIOC_REQBUFS, then > + * this will fail. Call atomisp_set_fmt() ourselves and try again. > + */ > ret = atomisp_get_css_frame_info(pipe->asd, source_pad, &pipe->frame_info); > - if (ret) > - return ret; > + if (ret) { > + struct v4l2_format f = { > + .fmt.pix.pixelformat = V4L2_PIX_FMT_YUV420, > + .fmt.pix.width = 10000, > + .fmt.pix.height = 10000, > + }; > + > + ret = atomisp_set_fmt(&pipe->vdev, &f); > + if (ret) > + goto out; > + > + ret = atomisp_get_css_frame_info(pipe->asd, source_pad, &pipe->frame_info); > + if (ret) > + goto out; > + } > > atomisp_alloc_css_stat_bufs(pipe->asd, ATOMISP_INPUT_STREAM_GENERAL); > > *nplanes = 1; > sizes[0] = PAGE_ALIGN(pipe->pix.sizeimage); > > +out: > + mutex_unlock(&pipe->asd->isp->mutex); > return 0; > } >
Hi, On 11/14/22 21:38, Mauro Carvalho Chehab wrote: > Em Thu, 20 Oct 2022 21:55:28 +0200 > Hans de Goede <hdegoede@redhat.com> escreveu: > >> camorama calls VIDIOC_REQBUFS(V4L2_MEMORY_MMAP) to test if MMAP support >> works (this was added specifically because of the previously broken >> MMAP support in atomisp). >> >> Currently this fails because atomisp_get_css_frame_info() fails in this >> case. Although it is weird to call VIDIOC_REQBUFS before VIDIOC_S_FMT, >> it is allowed to do this. >> >> Fix this not working by doing a S_FMT to V4L2_PIX_FMT_YUV420 + the highest >> supported resolution. >> >> Note this will cause camorama to use mmap mode, which means it will also >> use libv4l2 to do format conversion. libv4l2 will pick V4L2_PIX_FMT_RGB565 >> as input format and this will lead to a garbled video display. This is >> a libv4lconvert bug, the RGB565 -> RGB24 path in libv4lconvert assumes >> that stride == width which is not true on the atomisp. > > Hmm... should we add a patch on Camorama for it to not use libv4l2 if > format == V4L2_PIX_FMT_RGB565? This is not controlled by Camorama but by libv4lconvert, AFAICT there are no atomisp2 native formats which are supported in Camorama without libv4l ? > >> >> I've already send out a libv4lconvert fix for this. Also this can be worked >> around by passing --dont-use-libv4l2 as argument to camorama. > > Was the patch already applied? The better would be to apply it at > libv4l2 upstream, as a fix, porting it to old versions, and mentioning > on what versions this got fixed on this changeset. I see that you have already found the patches. I will add a comment to the commit msg pointing to the now applied patch. Regards, Hans > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../media/atomisp/pci/atomisp_compat_css20.c | 2 +- >> .../staging/media/atomisp/pci/atomisp_fops.c | 25 +++++++++++++++++-- >> 2 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c >> index ea6114679c53..f282572d69da 100644 >> --- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c >> +++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c >> @@ -2688,7 +2688,7 @@ int atomisp_get_css_frame_info(struct atomisp_sub_device *asd, >> >> if (0 != ia_css_pipe_get_info(asd->stream_env[stream_index] >> .pipes[pipe_index], &info)) { >> - dev_err(isp->dev, "ia_css_pipe_get_info FAILED"); >> + dev_dbg(isp->dev, "ia_css_pipe_get_info FAILED"); >> return -EINVAL; >> } >> >> diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c >> index bc47a092a8af..f539df09ebd9 100644 >> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c >> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c >> @@ -50,15 +50,36 @@ static int atomisp_queue_setup(struct vb2_queue *vq, >> u16 source_pad = atomisp_subdev_source_pad(&pipe->vdev); >> int ret; >> >> + mutex_lock(&pipe->asd->isp->mutex); /* for get_css_frame_info() / set_fmt() */ >> + >> + /* >> + * When VIDIOC_S_FMT has not been called before VIDIOC_REQBUFS, then >> + * this will fail. Call atomisp_set_fmt() ourselves and try again. >> + */ >> ret = atomisp_get_css_frame_info(pipe->asd, source_pad, &pipe->frame_info); >> - if (ret) >> - return ret; >> + if (ret) { >> + struct v4l2_format f = { >> + .fmt.pix.pixelformat = V4L2_PIX_FMT_YUV420, >> + .fmt.pix.width = 10000, >> + .fmt.pix.height = 10000, >> + }; >> + >> + ret = atomisp_set_fmt(&pipe->vdev, &f); >> + if (ret) >> + goto out; >> + >> + ret = atomisp_get_css_frame_info(pipe->asd, source_pad, &pipe->frame_info); >> + if (ret) >> + goto out; >> + } >> >> atomisp_alloc_css_stat_bufs(pipe->asd, ATOMISP_INPUT_STREAM_GENERAL); >> >> *nplanes = 1; >> sizes[0] = PAGE_ALIGN(pipe->pix.sizeimage); >> >> +out: >> + mutex_unlock(&pipe->asd->isp->mutex); >> return 0; >> } >> >
Hi, On 11/14/22 13:17, Andy Shevchenko wrote: > On Thu, Oct 20, 2022 at 09:55:28PM +0200, Hans de Goede wrote: >> camorama calls VIDIOC_REQBUFS(V4L2_MEMORY_MMAP) to test if MMAP support >> works (this was added specifically because of the previously broken >> MMAP support in atomisp). >> >> Currently this fails because atomisp_get_css_frame_info() fails in this >> case. Although it is weird to call VIDIOC_REQBUFS before VIDIOC_S_FMT, >> it is allowed to do this. >> >> Fix this not working by doing a S_FMT to V4L2_PIX_FMT_YUV420 + the highest >> supported resolution. >> >> Note this will cause camorama to use mmap mode, which means it will also >> use libv4l2 to do format conversion. libv4l2 will pick V4L2_PIX_FMT_RGB565 >> as input format and this will lead to a garbled video display. This is >> a libv4lconvert bug, the RGB565 -> RGB24 path in libv4lconvert assumes >> that stride == width which is not true on the atomisp. >> >> I've already send out a libv4lconvert fix for this. Also this can be worked > > Link: ? > Fixed in my media-atomisp branch. >> around by passing --dont-use-libv4l2 as argument to camorama. > > ... > >> + struct v4l2_format f = { >> + .fmt.pix.pixelformat = V4L2_PIX_FMT_YUV420, > >> + .fmt.pix.width = 10000, >> + .fmt.pix.height = 10000, > > If it's just an arbitrary maximum, I would suggest to use definitions from > limits.h, so it will better show the intention. This is duplicating existing code in another path in atomisp2 where it does this when it needs to pick a fmt because it needs one and userspace has not set one yet. I have added an entry to my atomisp TODO list to writer a helper for this so that we only have these magic 10000 values only once. As for if 10000 really is necessary, or if e.g. 65535 might work as well, that is unclear. Regards, Hans > >> + }; >
Em Sun, 20 Nov 2022 23:17:56 +0100 Hans de Goede <hdegoede@redhat.com> escreveu: > Hi, > > On 11/14/22 21:38, Mauro Carvalho Chehab wrote: > > Em Thu, 20 Oct 2022 21:55:28 +0200 > > Hans de Goede <hdegoede@redhat.com> escreveu: > > > >> camorama calls VIDIOC_REQBUFS(V4L2_MEMORY_MMAP) to test if MMAP support > >> works (this was added specifically because of the previously broken > >> MMAP support in atomisp). > >> > >> Currently this fails because atomisp_get_css_frame_info() fails in this > >> case. Although it is weird to call VIDIOC_REQBUFS before VIDIOC_S_FMT, > >> it is allowed to do this. > >> > >> Fix this not working by doing a S_FMT to V4L2_PIX_FMT_YUV420 + the highest > >> supported resolution. > >> > >> Note this will cause camorama to use mmap mode, which means it will also > >> use libv4l2 to do format conversion. libv4l2 will pick V4L2_PIX_FMT_RGB565 > >> as input format and this will lead to a garbled video display. This is > >> a libv4lconvert bug, the RGB565 -> RGB24 path in libv4lconvert assumes > >> that stride == width which is not true on the atomisp. > > > > Hmm... should we add a patch on Camorama for it to not use libv4l2 if > > format == V4L2_PIX_FMT_RGB565? > > This is not controlled by Camorama but by libv4lconvert, AFAICT there are > no atomisp2 native formats which are supported in Camorama without libv4l ? Actually, I added support on Camorama to decode it, plus other formats. It currently supports: V4L2_PIX_FMT_ABGR32 V4L2_PIX_FMT_ARGB32 V4L2_PIX_FMT_BGR24 V4L2_PIX_FMT_BGR32 V4L2_PIX_FMT_NV12 V4L2_PIX_FMT_NV21 V4L2_PIX_FMT_RGB24 V4L2_PIX_FMT_RGB32 V4L2_PIX_FMT_RGB565 V4L2_PIX_FMT_RGB565X V4L2_PIX_FMT_UYVY V4L2_PIX_FMT_VYUY V4L2_PIX_FMT_YUYV V4L2_PIX_FMT_YVYU V4L2_PIX_FMT_XBGR32 V4L2_PIX_FMT_XRGB32 V4L2_PIX_FMT_YUV420 V4L2_PIX_FMT_YVU420 It is just that it defaults to request RGB24 when not in userptr mode or when --dont-use-libv4l2 is not used. I guess it makes sense to make it smarter by using a native format, using its internal logic if the format is directly supported on it. It would also make sense to add Bayer and MPEG formats to it as well. > >> I've already send out a libv4lconvert fix for this. Also this can be worked > >> around by passing --dont-use-libv4l2 as argument to camorama. > > > > Was the patch already applied? The better would be to apply it at > > libv4l2 upstream, as a fix, porting it to old versions, and mentioning > > on what versions this got fixed on this changeset. > > I see that you have already found the patches. I will add a comment > to the commit msg pointing to the now applied patch. Ok. Regards, Mauro
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c index ea6114679c53..f282572d69da 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c +++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c @@ -2688,7 +2688,7 @@ int atomisp_get_css_frame_info(struct atomisp_sub_device *asd, if (0 != ia_css_pipe_get_info(asd->stream_env[stream_index] .pipes[pipe_index], &info)) { - dev_err(isp->dev, "ia_css_pipe_get_info FAILED"); + dev_dbg(isp->dev, "ia_css_pipe_get_info FAILED"); return -EINVAL; } diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c index bc47a092a8af..f539df09ebd9 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c @@ -50,15 +50,36 @@ static int atomisp_queue_setup(struct vb2_queue *vq, u16 source_pad = atomisp_subdev_source_pad(&pipe->vdev); int ret; + mutex_lock(&pipe->asd->isp->mutex); /* for get_css_frame_info() / set_fmt() */ + + /* + * When VIDIOC_S_FMT has not been called before VIDIOC_REQBUFS, then + * this will fail. Call atomisp_set_fmt() ourselves and try again. + */ ret = atomisp_get_css_frame_info(pipe->asd, source_pad, &pipe->frame_info); - if (ret) - return ret; + if (ret) { + struct v4l2_format f = { + .fmt.pix.pixelformat = V4L2_PIX_FMT_YUV420, + .fmt.pix.width = 10000, + .fmt.pix.height = 10000, + }; + + ret = atomisp_set_fmt(&pipe->vdev, &f); + if (ret) + goto out; + + ret = atomisp_get_css_frame_info(pipe->asd, source_pad, &pipe->frame_info); + if (ret) + goto out; + } atomisp_alloc_css_stat_bufs(pipe->asd, ATOMISP_INPUT_STREAM_GENERAL); *nplanes = 1; sizes[0] = PAGE_ALIGN(pipe->pix.sizeimage); +out: + mutex_unlock(&pipe->asd->isp->mutex); return 0; }
camorama calls VIDIOC_REQBUFS(V4L2_MEMORY_MMAP) to test if MMAP support works (this was added specifically because of the previously broken MMAP support in atomisp). Currently this fails because atomisp_get_css_frame_info() fails in this case. Although it is weird to call VIDIOC_REQBUFS before VIDIOC_S_FMT, it is allowed to do this. Fix this not working by doing a S_FMT to V4L2_PIX_FMT_YUV420 + the highest supported resolution. Note this will cause camorama to use mmap mode, which means it will also use libv4l2 to do format conversion. libv4l2 will pick V4L2_PIX_FMT_RGB565 as input format and this will lead to a garbled video display. This is a libv4lconvert bug, the RGB565 -> RGB24 path in libv4lconvert assumes that stride == width which is not true on the atomisp. I've already send out a libv4lconvert fix for this. Also this can be worked around by passing --dont-use-libv4l2 as argument to camorama. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../media/atomisp/pci/atomisp_compat_css20.c | 2 +- .../staging/media/atomisp/pci/atomisp_fops.c | 25 +++++++++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-)