cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Sat Oct 6 05:00:13 CEST 2018 media-tree git hash:557c97b5133669297be561e6091da9ab6e488e65 media_build git hash: 44385b9c61ecc27059a651885895c8ea09cd4179 v4l-utils git hash: b5ec216a33438afb6b35189379de24e13113fbb8 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.11-marune linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10-x86_64: ERRORS linux-3.12.74-i686: ERRORS linux-3.12.74-x86_64: ERRORS linux-3.13.11-i686: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.79-i686: ERRORS linux-3.14.79-x86_64: ERRORS linux-3.15.10-i686: ERRORS linux-3.15.10-x86_64: ERRORS linux-3.16.57-i686: ERRORS linux-3.16.57-x86_64: ERRORS linux-3.17.8-i686: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.123-i686: ERRORS linux-3.18.123-x86_64: ERRORS linux-3.19.8-i686: ERRORS linux-3.19.8-x86_64: ERRORS linux-4.0.9-i686: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.52-i686: ERRORS linux-4.1.52-x86_64: ERRORS linux-4.2.8-i686: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-i686: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.158-i686: ERRORS linux-4.4.158-x86_64: ERRORS linux-4.5.7-i686: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-i686: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: ERRORS linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: ERRORS linux-4.8.17-x86_64: ERRORS linux-4.9.129-i686: ERRORS linux-4.9.129-x86_64: ERRORS linux-4.10.17-i686: ERRORS linux-4.10.17-x86_64: ERRORS linux-4.11.12-i686: ERRORS linux-4.11.12-x86_64: ERRORS linux-4.12.14-i686: ERRORS linux-4.12.14-x86_64: ERRORS linux-4.13.16-i686: ERRORS linux-4.13.16-x86_64: ERRORS linux-4.14.72-i686: ERRORS linux-4.14.72-x86_64: ERRORS linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.10-i686: ERRORS linux-4.18.10-x86_64: ERRORS linux-4.19-rc5-i686: ERRORS linux-4.19-rc5-x86_64: ERRORS apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
[PATCH 1/2] dw9714: Remove useless error message
If probe fails, the kernel will print the error code. There's no need to driver to do that. Signed-off-by: Sakari Ailus --- drivers/media/i2c/dw9714.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c index 3dc2100470a1..26d83693a681 100644 --- a/drivers/media/i2c/dw9714.c +++ b/drivers/media/i2c/dw9714.c @@ -171,7 +171,7 @@ static int dw9714_probe(struct i2c_client *client) err_cleanup: v4l2_ctrl_handler_free(_dev->ctrls_vcm); media_entity_cleanup(_dev->sd.entity); - dev_err(>dev, "Probe failed: %d\n", rval); + return rval; } -- 2.11.0
[PATCH 0/2] dw9807 driver fix, dw9714 cleanup
Hi Raj, others, One fix and one cleanup for the dw9807 and dw9714 drivers. The dw9807 driver had the same issue than what you fixed in dw9714. Sakari Ailus (2): dw9714: Remove useless error message dw9807: Fix probe error handling drivers/media/i2c/dw9714.c | 2 +- drivers/media/i2c/dw9807-vcm.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) -- 2.11.0
[PATCH 2/2] dw9807-vcm: Fix probe error handling
v4l2_async_unregister_subdev() may not be called without v4l2_async_register_subdev() being called first. Fix this. Signed-off-by: Sakari Ailus --- drivers/media/i2c/dw9807-vcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/dw9807-vcm.c b/drivers/media/i2c/dw9807-vcm.c index a532c57dc636..73b0e10dc591 100644 --- a/drivers/media/i2c/dw9807-vcm.c +++ b/drivers/media/i2c/dw9807-vcm.c @@ -218,7 +218,8 @@ static int dw9807_probe(struct i2c_client *client) return 0; err_cleanup: - dw9807_subdev_cleanup(dw9807_dev); + v4l2_ctrl_handler_free(_dev->ctrls_vcm); + media_entity_cleanup(_dev->sd.entity); return rval; } -- 2.11.0
Re: [PATCH] MAINTAINERS: Remove stale file entry for the Atmel ISI driver
Em Thu, 04 Oct 2018 23:06:02 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Thursday, 4 October 2018 21:45:05 EEST Mauro Carvalho Chehab wrote:
> > Em Tue, 2 Oct 2018 08:35:47 +0200 Ludovic Desroches escreveu:
> > > On Mon, Oct 01, 2018 at 01:51:01PM -0300, Mauro Carvalho Chehab wrote:
> > > > Em Sun, 30 Sep 2018 02:40:35 -0700 Joe Perches escreveu:
> > > > > On Sun, 2018-09-30 at 06:30 -0300, Mauro Carvalho Chehab wrote:
> > > > > > Em Sun, 30 Sep 2018 09:54:48 +0300 Laurent Pinchart escreveu:
> > > > > > > include/media/atmel-isi got removed three years ago without the
> > > > > > > MAINTAINERS file being updated. Remove the stale entry.
> > > > > > >
> > > > > > > Fixes: 40a78f36fc92 ("[media] v4l: atmel-isi: Remove support for
> > > > > > > platform data") Reported-by: Joe Perches
> > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > MAINTAINERS | 1 -
> > > > > > > 1 file changed, 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > >
> > > > > []
> > > > >
> > > > > > > @@ -2497,7 +2497,6 @@ M: Ludovic Desroches
> > > > > > > > > > > >
> > > > > > > L: linux-media@vger.kernel.org
> > > > > > > S: Supported
> > > > > > > F: drivers/media/platform/atmel/atmel-isi.c
> > > > > > >
> > > > > > > -F: include/media/atmel-isi.h
> > > > > >
> > > > > > I guess the right fix would be to replace it by:
> > > > > >
> > > > > > F: drivers/media/platform/atmel/atmel-isi.h
> > > > >
> > > > > Or replace both F entries with:
> > > > >
> > > > > F:drivers/media/platform/atmel/atmel-isi.*
> > > > >
> > > > > Or combine the 2 MICROCHIP sections into one
> > > > >
> > > > > MICROCHIP ISC DRIVER
> > > > > M:Eugen Hristev
> > > > > L:linux-media@vger.kernel.org
> > > > > S:Supported
> > > > > F:drivers/media/platform/atmel/atmel-isc.c
> > > > > F:drivers/media/platform/atmel/atmel-isc-regs.h
> > > > > F:devicetree/bindings/media/atmel-isc.txt
> > > > >
> > > > > MICROCHIP ISI DRIVER
> > > > > M:Eugen Hristev
> > > > > L:linux-media@vger.kernel.org
> > > > > S:Supported
> > > > > F:drivers/media/platform/atmel/atmel-isi.c
> > > > > F:include/media/atmel-isi.h
> > > > >
> > > > > and maybe use something like:
> > > > >
> > > > > MICROCHIP MEDIA DRIVERS
> > > > > M:Eugen Hristev
> > > > > L:
> > > > > linux-media@vger.kernel.org
> > > > > S:Supported
> > > > > F:drivers/media/platform/atmel/
> > > > > F:devicetree/bindings/media/atmel-isc.txt
> > > >
> > > > Yeah, combining both of them seems a good alternative to me.
> > > >
> > > > Eugen/Ludovic/Josh,
> > > >
> > > > Comments?
> > >
> > > I have no strong opinion about it. The devices are different but usually
> > > there is one person per topic so combining them makes sense.
> >
> > Hmm... At media tree, currently, MAINTAINERS entry is different:
> >
> > MICROCHIP / ATMEL ISC DRIVER
> > M: Songjun Wu
> > L: linux-media@vger.kernel.org
> > S: Supported
> > F: drivers/media/platform/atmel/atmel-isc.c
> > F: drivers/media/platform/atmel/atmel-isc-regs.h
> > F: devicetree/bindings/media/atmel-isc.txt
> >
> > ATMEL ISI DRIVER
> > M: Ludovic Desroches
> > L: linux-media@vger.kernel.org
> > S: Supported
> > F: drivers/media/platform/atmel/atmel-isi.c
> > F: include/media/atmel-isi.h
> >
> > Maybe some patch upstream did some recent changes on it via another
> > tree.
> >
> > So, in order to avoid conflicts upstream, for now I would just correct
> > the location for the ISI file.
> >
> > After the merge window, we may revisit and join both entries,
> > if the maintainers are the same.
>
> Fine with me.
>
> > -
> >
> > MAINTAINERS: fix location for atmel-isi.h file
> >
> > The location of this file got changed by changeset 40a78f36fc92
> > ("[media] v4l: atmel-isi: Remove support for platform data"), but
> > MAINTAINERS was not updated accordingly.
> >
> > Fixes: 40a78f36fc92 ("[media] v4l: atmel-isi: Remove support for platform
> > data") Reported-by: Joe Perches
> > Signed-off-by: Mauro Carvalho Chehab
>
> I don't care about patch count statistics, but the usual practice is to
> retain
> the authorship of the original submitter.
Huh? Frankly, I don't care who will be accredited for this, provided
that this gets fixed. Yet, I'm lost with your comment.
Did I miss some other patch at the ML touching it?
At patchwork, it seems that there are only two patches:
https://patchwork.linuxtv.org/project/linux-media/list/?state=*=MAINTAINERS
the one from you, that assumed that this file got removed
and does:
-F: include/media/atmel-isi.h
And my alternative proposal, that changes it to reflect its
current location:
-F: drivers/media/platform/atmel/atmel-isi.c
-F: include/media/atmel-isi.h
+F: drivers/media/platform/atmel/atmel-isi.*
Re: [PATCH v6 0/6] Add Rockchip VPU JPEG encoder
On Fri, 2018-10-05 at 13:49 -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 05 Oct 2018 12:37:43 -0300
> Ezequiel Garcia escreveu:
>
> > On Fri, 2018-10-05 at 09:10 -0300, Mauro Carvalho Chehab wrote:
> > > Em Thu, 04 Oct 2018 20:39:31 -0300
> > > Ezequiel Garcia escreveu:
> > >
> > > > On Mon, 2018-10-01 at 14:54 -0300, Ezequiel Garcia wrote:
> > > > > On Fri, 2018-09-28 at 14:33 +0200, Hans Verkuil wrote:
> > > > > > On 09/17/2018 07:30 PM, Ezequiel Garcia wrote:
> > > > > > > This series adds support for JPEG encoding via the VPU block
> > > > > > > present in Rockchip platforms. Currently, support for RK3288
> > > > > > > and RK3399 is included.
> > > > > > >
> > > > > > > Please, see the previous versions of this cover letter for
> > > > > > > more information.
> > > > > > >
> > > > > > > Compared to v5, the only change is in the
> > > > > > > V4L2_CID_JPEG_QUANTIZATION
> > > > > > > control. We've decided to support only baseline profile,
> > > > > > > and only add 8-bit luma and chroma tables.
> > > > > > >
> > > > > > > struct v4l2_ctrl_jpeg_quantization {
> > > > > > >__u8luma_quantization_matrix[64];
> > > > > > >__u8chroma_quantization_matrix[64];
> > > > > > > };
> > > > > > >
> > > > > > > By doing this, it's clear that we don't currently support anything
> > > > > > > but baseline.
> > > > > > >
> > > > > > > This series should apply cleanly on top of
> > > > > > >
> > > > > > > git://linuxtv.org/hverkuil/media_tree.git br-cedrus tag.
> > > > > > >
> > > > > > > If everyone is happy with this series, I'd like to route the
> > > > > > > devicetree
> > > > > > > changes through the rockchip tree, and the rest via the media
> > > > > > > subsystem.
> > > > > >
> > > > > > OK, so I have what is no doubt an annoying question: do we really
> > > > > > need
> > > > > > a JPEG_RAW format?
> > > > > >
> > > > >
> > > > > Not annoying, as it helps clarify a few things :-)
> > > > > I think we do need the JPEG_RAW format. The way I see it, using
> > > > > JPEG opens a can of worms...
> > > > >
> > > > > > The JPEG header is really easy to parse for a decoder and really
> > > > > > easy to
> > > > > > prepend to the compressed image for the encoder.
> > > > > >
> > > > > > The only reason I can see for a JPEG_RAW is if the image must start
> > > > > > at
> > > > > > some specific address alignment. Although even then you can just
> > > > > > pad the
> > > > > > JPEG header that you will add according to the alignment
> > > > > > requirements.
> > > > > >
> > > > > > I know I am very late with this question, but I never looked all
> > > > > > that
> > > > > > closely at what a JPEG header looks like. But this helped:
> > > > > >
> > > > > > https://en.wikipedia.org/wiki/JPEG_File_Interchange_Format
> > > > > >
> > > > > > and it doesn't seem difficult at all to parse or create the header.
> > > > > >
> > > > > >
> > > > >
> > > > > ... I think that having JPEG_RAW as the compressed format
> > > > > is much more clear for userspace, as it explicitly specifies
> > > > > what is expected.
> > > > >
> > > > > This way, for a stateless encoder, applications are required
> > > > > to set quantization and/or entropy tables, and are then in
> > > > > charge of using the compressed JPEG_RAW payload in whatever form
> > > > > they want. Stupid simple.
> > > > >
> > > > > On the other side, if the stateless encoder driver supports
> > > > > JPEG (creating headers in-kernel), it means that:
> > > > >
> > > > > *) applications should pass a quality control, if supported,
> > > > > and the driver will use hardcoded tables or...
> > > > >
> > > > > *) applications pass quantization control and, if supported,
> > > > > entropy control. The kernel uses them to create the JPEG frame.
> > > > > But also, some drivers (e.g. Rockchip), use default entropy
> > > > > tables, which should now be in the kernel.
> > > > >
> > > > > So the application would have to query controls to find out
> > > > > what to do. Not exactly hard, but I think having the JPEG_RAW
> > > > > is much simpler and more clear.
> > > > >
> > > > > Now, for stateless decoders, supporting JPEG_RAW means
> > > > > the application has to pass quantization and entropy
> > > > > controls, probably using the Request API.
> > > > > Given the application has parsed the JPEG,
> > > > > it knows the width and height and can request
> > > > > buffers accordingly.
> > > > >
> > > > > The hardware is stateless, and so is the driver.
> > > > >
> > > > > On the other hand, supporting JPEG would mean that
> > > > > drivers will have to parse the image, extracting
> > > > > the quantization and entropy tables.
> > > > >
> > > > > Format negotiation is now more complex,
> > > > > either we follow the stateful spec, introducing a little
> > > > > state machine in the driver... or we use the Request API,
> > > > > but that means parsing on both sides kernel and application.
> > > > >
Re: [PATCH v6 0/6] Add Rockchip VPU JPEG encoder
Em Fri, 05 Oct 2018 12:37:43 -0300
Ezequiel Garcia escreveu:
> On Fri, 2018-10-05 at 09:10 -0300, Mauro Carvalho Chehab wrote:
> > Em Thu, 04 Oct 2018 20:39:31 -0300
> > Ezequiel Garcia escreveu:
> >
> > > On Mon, 2018-10-01 at 14:54 -0300, Ezequiel Garcia wrote:
> > > > On Fri, 2018-09-28 at 14:33 +0200, Hans Verkuil wrote:
> > > > > On 09/17/2018 07:30 PM, Ezequiel Garcia wrote:
> > > > > > This series adds support for JPEG encoding via the VPU block
> > > > > > present in Rockchip platforms. Currently, support for RK3288
> > > > > > and RK3399 is included.
> > > > > >
> > > > > > Please, see the previous versions of this cover letter for
> > > > > > more information.
> > > > > >
> > > > > > Compared to v5, the only change is in the V4L2_CID_JPEG_QUANTIZATION
> > > > > > control. We've decided to support only baseline profile,
> > > > > > and only add 8-bit luma and chroma tables.
> > > > > >
> > > > > > struct v4l2_ctrl_jpeg_quantization {
> > > > > >__u8luma_quantization_matrix[64];
> > > > > >__u8chroma_quantization_matrix[64];
> > > > > > };
> > > > > >
> > > > > > By doing this, it's clear that we don't currently support anything
> > > > > > but baseline.
> > > > > >
> > > > > > This series should apply cleanly on top of
> > > > > >
> > > > > > git://linuxtv.org/hverkuil/media_tree.git br-cedrus tag.
> > > > > >
> > > > > > If everyone is happy with this series, I'd like to route the
> > > > > > devicetree
> > > > > > changes through the rockchip tree, and the rest via the media
> > > > > > subsystem.
> > > > >
> > > > > OK, so I have what is no doubt an annoying question: do we really need
> > > > > a JPEG_RAW format?
> > > > >
> > > >
> > > > Not annoying, as it helps clarify a few things :-)
> > > > I think we do need the JPEG_RAW format. The way I see it, using
> > > > JPEG opens a can of worms...
> > > >
> > > > > The JPEG header is really easy to parse for a decoder and really easy
> > > > > to
> > > > > prepend to the compressed image for the encoder.
> > > > >
> > > > > The only reason I can see for a JPEG_RAW is if the image must start at
> > > > > some specific address alignment. Although even then you can just pad
> > > > > the
> > > > > JPEG header that you will add according to the alignment requirements.
> > > > >
> > > > > I know I am very late with this question, but I never looked all that
> > > > > closely at what a JPEG header looks like. But this helped:
> > > > >
> > > > > https://en.wikipedia.org/wiki/JPEG_File_Interchange_Format
> > > > >
> > > > > and it doesn't seem difficult at all to parse or create the header.
> > > > >
> > > > >
> > > >
> > > > ... I think that having JPEG_RAW as the compressed format
> > > > is much more clear for userspace, as it explicitly specifies
> > > > what is expected.
> > > >
> > > > This way, for a stateless encoder, applications are required
> > > > to set quantization and/or entropy tables, and are then in
> > > > charge of using the compressed JPEG_RAW payload in whatever form
> > > > they want. Stupid simple.
> > > >
> > > > On the other side, if the stateless encoder driver supports
> > > > JPEG (creating headers in-kernel), it means that:
> > > >
> > > > *) applications should pass a quality control, if supported,
> > > > and the driver will use hardcoded tables or...
> > > >
> > > > *) applications pass quantization control and, if supported,
> > > > entropy control. The kernel uses them to create the JPEG frame.
> > > > But also, some drivers (e.g. Rockchip), use default entropy
> > > > tables, which should now be in the kernel.
> > > >
> > > > So the application would have to query controls to find out
> > > > what to do. Not exactly hard, but I think having the JPEG_RAW
> > > > is much simpler and more clear.
> > > >
> > > > Now, for stateless decoders, supporting JPEG_RAW means
> > > > the application has to pass quantization and entropy
> > > > controls, probably using the Request API.
> > > > Given the application has parsed the JPEG,
> > > > it knows the width and height and can request
> > > > buffers accordingly.
> > > >
> > > > The hardware is stateless, and so is the driver.
> > > >
> > > > On the other hand, supporting JPEG would mean that
> > > > drivers will have to parse the image, extracting
> > > > the quantization and entropy tables.
> > > >
> > > > Format negotiation is now more complex,
> > > > either we follow the stateful spec, introducing a little
> > > > state machine in the driver... or we use the Request API,
> > > > but that means parsing on both sides kernel and application.
> > > >
> > > > Either way, using JPEG_RAW is just waaay simpler and puts
> > > > things where they belong.
> > > >
> > >
> > > As discussed on IRC, I'm sending a v7 for this series,
> > > fixing only the checkpatch issues and the extra line in the
> > > binding document.
> > >
> > > We've agreed to move forward with JPEG_RAW, for the
Re: [PATCH v6 0/6] Add Rockchip VPU JPEG encoder
On Fri, 2018-10-05 at 09:10 -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 04 Oct 2018 20:39:31 -0300
> Ezequiel Garcia escreveu:
>
> > On Mon, 2018-10-01 at 14:54 -0300, Ezequiel Garcia wrote:
> > > On Fri, 2018-09-28 at 14:33 +0200, Hans Verkuil wrote:
> > > > On 09/17/2018 07:30 PM, Ezequiel Garcia wrote:
> > > > > This series adds support for JPEG encoding via the VPU block
> > > > > present in Rockchip platforms. Currently, support for RK3288
> > > > > and RK3399 is included.
> > > > >
> > > > > Please, see the previous versions of this cover letter for
> > > > > more information.
> > > > >
> > > > > Compared to v5, the only change is in the V4L2_CID_JPEG_QUANTIZATION
> > > > > control. We've decided to support only baseline profile,
> > > > > and only add 8-bit luma and chroma tables.
> > > > >
> > > > > struct v4l2_ctrl_jpeg_quantization {
> > > > >__u8luma_quantization_matrix[64];
> > > > >__u8chroma_quantization_matrix[64];
> > > > > };
> > > > >
> > > > > By doing this, it's clear that we don't currently support anything
> > > > > but baseline.
> > > > >
> > > > > This series should apply cleanly on top of
> > > > >
> > > > > git://linuxtv.org/hverkuil/media_tree.git br-cedrus tag.
> > > > >
> > > > > If everyone is happy with this series, I'd like to route the
> > > > > devicetree
> > > > > changes through the rockchip tree, and the rest via the media
> > > > > subsystem.
> > > >
> > > > OK, so I have what is no doubt an annoying question: do we really need
> > > > a JPEG_RAW format?
> > > >
> > >
> > > Not annoying, as it helps clarify a few things :-)
> > > I think we do need the JPEG_RAW format. The way I see it, using
> > > JPEG opens a can of worms...
> > >
> > > > The JPEG header is really easy to parse for a decoder and really easy to
> > > > prepend to the compressed image for the encoder.
> > > >
> > > > The only reason I can see for a JPEG_RAW is if the image must start at
> > > > some specific address alignment. Although even then you can just pad the
> > > > JPEG header that you will add according to the alignment requirements.
> > > >
> > > > I know I am very late with this question, but I never looked all that
> > > > closely at what a JPEG header looks like. But this helped:
> > > >
> > > > https://en.wikipedia.org/wiki/JPEG_File_Interchange_Format
> > > >
> > > > and it doesn't seem difficult at all to parse or create the header.
> > > >
> > > >
> > >
> > > ... I think that having JPEG_RAW as the compressed format
> > > is much more clear for userspace, as it explicitly specifies
> > > what is expected.
> > >
> > > This way, for a stateless encoder, applications are required
> > > to set quantization and/or entropy tables, and are then in
> > > charge of using the compressed JPEG_RAW payload in whatever form
> > > they want. Stupid simple.
> > >
> > > On the other side, if the stateless encoder driver supports
> > > JPEG (creating headers in-kernel), it means that:
> > >
> > > *) applications should pass a quality control, if supported,
> > > and the driver will use hardcoded tables or...
> > >
> > > *) applications pass quantization control and, if supported,
> > > entropy control. The kernel uses them to create the JPEG frame.
> > > But also, some drivers (e.g. Rockchip), use default entropy
> > > tables, which should now be in the kernel.
> > >
> > > So the application would have to query controls to find out
> > > what to do. Not exactly hard, but I think having the JPEG_RAW
> > > is much simpler and more clear.
> > >
> > > Now, for stateless decoders, supporting JPEG_RAW means
> > > the application has to pass quantization and entropy
> > > controls, probably using the Request API.
> > > Given the application has parsed the JPEG,
> > > it knows the width and height and can request
> > > buffers accordingly.
> > >
> > > The hardware is stateless, and so is the driver.
> > >
> > > On the other hand, supporting JPEG would mean that
> > > drivers will have to parse the image, extracting
> > > the quantization and entropy tables.
> > >
> > > Format negotiation is now more complex,
> > > either we follow the stateful spec, introducing a little
> > > state machine in the driver... or we use the Request API,
> > > but that means parsing on both sides kernel and application.
> > >
> > > Either way, using JPEG_RAW is just waaay simpler and puts
> > > things where they belong.
> > >
> >
> > As discussed on IRC, I'm sending a v7 for this series,
> > fixing only the checkpatch issues and the extra line in the
> > binding document.
> >
> > We've agreed to move forward with JPEG_RAW, for the reasons
> > stated above.
> >
> > Plus, we've agreed to keep this in staging until the userspace
> > support for JPEG_RAW format is clear.
>
> The problem is that a new format is actually a V4L2 core change, and
> not something for staging.
>
> IMHO, it is prudent to wait for an userspace code before hushing
> merging this
Re: [PATCH v2 0/6] media: video-i2c: support changing frame interval and runtime PM
On Mon, Sep 24, 2018 at 01:34:46AM +0900, Akinobu Mita wrote: > This patchset adds support for changing frame interval and runtime PM for > video-i2c driver. This also adds an helper macro to v4l2 common > internal API that is used to to find a suitable frame interval. > > There are a couple of unrelated changes that are included for simplifying > driver initialization code and register accesses. > > * v2 > - Add Acked-by and Reviewed-by tags > - Update commit log to clarify the use after free > - Add thermistor and termperature register address definisions > - Stop adding v4l2_find_closest_fract() in v4l2 common internal API > - Add V4L2_FRACT_COMPARE() macro in v4l2 common internal API > - Use V4L2_FRACT_COMPARE() to find suitable frame interval instead of > v4l2_find_closest_fract() > - Add comment for register address definisions > > Akinobu Mita (6): > media: video-i2c: avoid accessing released memory area when removing > driver > media: video-i2c: use i2c regmap > media: v4l2-common: add V4L2_FRACT_COMPARE > media: vivid: use V4L2_FRACT_COMPARE > media: video-i2c: support changing frame interval > media: video-i2c: support runtime PM For patches 2--6: Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH RESEND] Revert "media: dvbsky: use just one mutex for serializing device R/W ops"
Em Fri, 5 Oct 2018 16:34:28 +0200 Oliver Freyermuth escreveu: > Dear Mauro, > > thanks! Just to clarify, the issue I described in > https://bugzilla.kernel.org/show_bug.cgi?id=199323 > was on an Intel x86_64 system, with an onboard USB Controller handled by the > standard xhci driver, > so this does not affect RPi alone. That's weird... I tested such patch here before applying (and it was tested by someone else, as far as I know), and it worked fine. Perhaps the x86 bug is related to some recent changes at the USB subsystem. Dunno. Anyway, patch revert applied upstream. Regards, Mauro
[PATCH] mceusb: Include three Hauppauge USB dvb device with IR rx
The three following Hauppauge USB DVB devices have IR receivers, but
lacked the support in mceusb to enable it:
- WinTV-HVR-935C
- WinTV-HVR-955Q
- WinTV-HVR-975
Tested HVR-955Q and HVR-975 plus RC5 remote and irw, works as intended.
Signed-off-by: Brad Love
---
drivers/media/rc/mceusb.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
index 4c0c800..b6db968 100644
--- a/drivers/media/rc/mceusb.c
+++ b/drivers/media/rc/mceusb.c
@@ -432,6 +432,15 @@ static const struct usb_device_id mceusb_dev_table[] = {
.driver_info = HAUPPAUGE_CX_HYBRID_TV },
{ USB_DEVICE(VENDOR_HAUPPAUGE, 0xb139),
.driver_info = HAUPPAUGE_CX_HYBRID_TV },
+ /* Hauppauge WinTV-HVR-935C - based on cx231xx */
+ { USB_DEVICE(VENDOR_HAUPPAUGE, 0xb151),
+ .driver_info = HAUPPAUGE_CX_HYBRID_TV },
+ /* Hauppauge WinTV-HVR-955Q - based on cx231xx */
+ { USB_DEVICE(VENDOR_HAUPPAUGE, 0xb123),
+ .driver_info = HAUPPAUGE_CX_HYBRID_TV },
+ /* Hauppauge WinTV-HVR-975 - based on cx231xx */
+ { USB_DEVICE(VENDOR_HAUPPAUGE, 0xb150),
+ .driver_info = HAUPPAUGE_CX_HYBRID_TV },
{ USB_DEVICE(VENDOR_PCTV, 0x0259),
.driver_info = HAUPPAUGE_CX_HYBRID_TV },
{ USB_DEVICE(VENDOR_PCTV, 0x025e),
--
2.7.4
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
Hi Mita-san,
On Fri, Oct 05, 2018 at 11:59:20PM +0900, Akinobu Mita wrote:
> 2018年10月5日(金) 18:36 Sakari Ailus :
> >
> > Hi Hans,
> >
> > On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> > > On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > > > The video_i2c_data is allocated by kzalloc and released by the video
> > > > device's release callback. The release callback is called when
> > > > video_unregister_device() is called, but it will still be accessed after
> > > > calling video_unregister_device().
> > > >
> > > > Fix the use after free by allocating video_i2c_data by devm_kzalloc()
> > > > with
> > > > i2c_client->dev so that it will automatically be released when the i2c
> > > > driver is removed.
> > >
> > > Hmm. The patch is right, but the explanation isn't. The core problem is
> > > that vdev.release was set to video_i2c_release, but that should only be
> > > used if struct video_device was kzalloc'ed. But in this case it is
> > > embedded
> > > in a larger struct, and then vdev.release should always be set to
> > > video_device_release_empty.
> >
> > When the driver is unbound, what's acquired using the devm_() family of
> > functions is released. At the same time, the user still holds a file
> > handle, and can issue IOCTLs --- but the device's data structures no longer
> > exist.
> >
> > That's not ok, and also the reason why we have the release callback.
> >
> > While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> > fine.
> >
> > Or am I missing something?
>
> How about moving the lines causing use-after-free to release callback
> like below?
>
> static void video_i2c_release(struct video_device *vdev)
> {
> struct video_i2c_data *data = video_get_drvdata(vdev);
>
> v4l2_device_unregister(>v4l2_dev);
> mutex_destroy(>lock);
> mutex_destroy(>queue_lock);
> kfree(data);
> }
So the remove function would only contain video_unregister_device()? That's
the way it should be, as far as I can see.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
2018年10月5日(金) 18:36 Sakari Ailus :
>
> Hi Hans,
>
> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> > On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > > The video_i2c_data is allocated by kzalloc and released by the video
> > > device's release callback. The release callback is called when
> > > video_unregister_device() is called, but it will still be accessed after
> > > calling video_unregister_device().
> > >
> > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> > > i2c_client->dev so that it will automatically be released when the i2c
> > > driver is removed.
> >
> > Hmm. The patch is right, but the explanation isn't. The core problem is
> > that vdev.release was set to video_i2c_release, but that should only be
> > used if struct video_device was kzalloc'ed. But in this case it is embedded
> > in a larger struct, and then vdev.release should always be set to
> > video_device_release_empty.
>
> When the driver is unbound, what's acquired using the devm_() family of
> functions is released. At the same time, the user still holds a file
> handle, and can issue IOCTLs --- but the device's data structures no longer
> exist.
>
> That's not ok, and also the reason why we have the release callback.
>
> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> fine.
>
> Or am I missing something?
How about moving the lines causing use-after-free to release callback
like below?
static void video_i2c_release(struct video_device *vdev)
{
struct video_i2c_data *data = video_get_drvdata(vdev);
v4l2_device_unregister(>v4l2_dev);
mutex_destroy(>lock);
mutex_destroy(>queue_lock);
kfree(data);
}
Re: [PATCH RESEND] Revert "media: dvbsky: use just one mutex for serializing device R/W ops"
Dear Mauro,
thanks! Just to clarify, the issue I described in
https://bugzilla.kernel.org/show_bug.cgi?id=199323
was on an Intel x86_64 system, with an onboard USB Controller handled by the
standard xhci driver,
so this does not affect RPi alone.
Cheers and thanks,
Oliver
Am 05.10.18 um 16:26 schrieb Mauro Carvalho Chehab:
> As pointed at:
> https://bugzilla.kernel.org/show_bug.cgi?id=199323
>
> This patch causes a bad effect on RPi. I suspect that the root
> cause is at the USB RPi driver, with uses high priority
> interrupts instead of normal ones. Anyway, as this patch
> is mostly a cleanup, better to revert it.
>
> This reverts commit 7d95fb746c4eece67308f1642a666ea1ebdbd2cc.
>
> Cc: sta...@vger.kernel.org # For Kernel 4.18
> Signed-off-by: Mauro Carvalho Chehab
> ---
>
> Re-sending it with the right message ID
>
> drivers/media/usb/dvb-usb-v2/dvbsky.c | 16 ++--
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> index 1aa88d94e57f..e28bd8836751 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> @@ -31,6 +31,7 @@ MODULE_PARM_DESC(disable_rc, "Disable inbuilt IR
> receiver.");
> DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>
> struct dvbsky_state {
> + struct mutex stream_mutex;
> u8 ibuf[DVBSKY_BUF_LEN];
> u8 obuf[DVBSKY_BUF_LEN];
> u8 last_lock;
> @@ -67,17 +68,18 @@ static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
>
> static int dvbsky_stream_ctrl(struct dvb_usb_device *d, u8 onoff)
> {
> + struct dvbsky_state *state = d_to_priv(d);
> int ret;
> - static u8 obuf_pre[3] = { 0x37, 0, 0 };
> - static u8 obuf_post[3] = { 0x36, 3, 0 };
> + u8 obuf_pre[3] = { 0x37, 0, 0 };
> + u8 obuf_post[3] = { 0x36, 3, 0 };
>
> - mutex_lock(>usb_mutex);
> - ret = dvb_usbv2_generic_rw_locked(d, obuf_pre, 3, NULL, 0);
> + mutex_lock(>stream_mutex);
> + ret = dvbsky_usb_generic_rw(d, obuf_pre, 3, NULL, 0);
> if (!ret && onoff) {
> msleep(20);
> - ret = dvb_usbv2_generic_rw_locked(d, obuf_post, 3, NULL, 0);
> + ret = dvbsky_usb_generic_rw(d, obuf_post, 3, NULL, 0);
> }
> - mutex_unlock(>usb_mutex);
> + mutex_unlock(>stream_mutex);
> return ret;
> }
>
> @@ -606,6 +608,8 @@ static int dvbsky_init(struct dvb_usb_device *d)
> if (ret)
> return ret;
> */
> + mutex_init(>stream_mutex);
> +
> state->last_lock = 0;
>
> return 0;
>
[PATCH RESEND] Revert "media: dvbsky: use just one mutex for serializing device R/W ops"
As pointed at:
https://bugzilla.kernel.org/show_bug.cgi?id=199323
This patch causes a bad effect on RPi. I suspect that the root
cause is at the USB RPi driver, with uses high priority
interrupts instead of normal ones. Anyway, as this patch
is mostly a cleanup, better to revert it.
This reverts commit 7d95fb746c4eece67308f1642a666ea1ebdbd2cc.
Cc: sta...@vger.kernel.org # For Kernel 4.18
Signed-off-by: Mauro Carvalho Chehab
---
Re-sending it with the right message ID
drivers/media/usb/dvb-usb-v2/dvbsky.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index 1aa88d94e57f..e28bd8836751 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -31,6 +31,7 @@ MODULE_PARM_DESC(disable_rc, "Disable inbuilt IR receiver.");
DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
struct dvbsky_state {
+ struct mutex stream_mutex;
u8 ibuf[DVBSKY_BUF_LEN];
u8 obuf[DVBSKY_BUF_LEN];
u8 last_lock;
@@ -67,17 +68,18 @@ static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
static int dvbsky_stream_ctrl(struct dvb_usb_device *d, u8 onoff)
{
+ struct dvbsky_state *state = d_to_priv(d);
int ret;
- static u8 obuf_pre[3] = { 0x37, 0, 0 };
- static u8 obuf_post[3] = { 0x36, 3, 0 };
+ u8 obuf_pre[3] = { 0x37, 0, 0 };
+ u8 obuf_post[3] = { 0x36, 3, 0 };
- mutex_lock(>usb_mutex);
- ret = dvb_usbv2_generic_rw_locked(d, obuf_pre, 3, NULL, 0);
+ mutex_lock(>stream_mutex);
+ ret = dvbsky_usb_generic_rw(d, obuf_pre, 3, NULL, 0);
if (!ret && onoff) {
msleep(20);
- ret = dvb_usbv2_generic_rw_locked(d, obuf_post, 3, NULL, 0);
+ ret = dvbsky_usb_generic_rw(d, obuf_post, 3, NULL, 0);
}
- mutex_unlock(>usb_mutex);
+ mutex_unlock(>stream_mutex);
return ret;
}
@@ -606,6 +608,8 @@ static int dvbsky_init(struct dvb_usb_device *d)
if (ret)
return ret;
*/
+ mutex_init(>stream_mutex);
+
state->last_lock = 0;
return 0;
--
2.17.1
[PATCH] Revert "media: dvbsky: use just one mutex for serializing device R/W ops"
As pointed at:
https://bugzilla.kernel.org/show_bug.cgi?id=199323
This patch causes a bad effect on RPi. I suspect that the root
cause is at the USB RPi driver, with uses high priority
interrupts instead of normal ones. Anyway, as this patch
is mostly a cleanup, better to revert it.
This reverts commit 7d95fb746c4eece67308f1642a666ea1ebdbd2cc.
Cc: sta...@vger.kernel.org # For Kernel 4.18
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/usb/dvb-usb-v2/dvbsky.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index 1aa88d94e57f..e28bd8836751 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -31,6 +31,7 @@ MODULE_PARM_DESC(disable_rc, "Disable inbuilt IR receiver.");
DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
struct dvbsky_state {
+ struct mutex stream_mutex;
u8 ibuf[DVBSKY_BUF_LEN];
u8 obuf[DVBSKY_BUF_LEN];
u8 last_lock;
@@ -67,17 +68,18 @@ static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
static int dvbsky_stream_ctrl(struct dvb_usb_device *d, u8 onoff)
{
+ struct dvbsky_state *state = d_to_priv(d);
int ret;
- static u8 obuf_pre[3] = { 0x37, 0, 0 };
- static u8 obuf_post[3] = { 0x36, 3, 0 };
+ u8 obuf_pre[3] = { 0x37, 0, 0 };
+ u8 obuf_post[3] = { 0x36, 3, 0 };
- mutex_lock(>usb_mutex);
- ret = dvb_usbv2_generic_rw_locked(d, obuf_pre, 3, NULL, 0);
+ mutex_lock(>stream_mutex);
+ ret = dvbsky_usb_generic_rw(d, obuf_pre, 3, NULL, 0);
if (!ret && onoff) {
msleep(20);
- ret = dvb_usbv2_generic_rw_locked(d, obuf_post, 3, NULL, 0);
+ ret = dvbsky_usb_generic_rw(d, obuf_post, 3, NULL, 0);
}
- mutex_unlock(>usb_mutex);
+ mutex_unlock(>stream_mutex);
return ret;
}
@@ -606,6 +608,8 @@ static int dvbsky_init(struct dvb_usb_device *d)
if (ret)
return ret;
*/
+ mutex_init(>stream_mutex);
+
state->last_lock = 0;
return 0;
--
2.17.1
[PATCH] media: imx355: fix a few coding style issues
Function alignments are off by 1 space, as reported by
checkpatch.pl --strict.
Fix those.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/i2c/imx355.c | 32
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index 803df2a014bb..20c8eea5db4b 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -1136,7 +1136,7 @@ static int imx355_write_reg(struct imx355 *imx355, u16
reg, u32 len, u32 val)
/* Write a list of registers */
static int imx355_write_regs(struct imx355 *imx355,
- const struct imx355_reg *regs, u32 len)
+const struct imx355_reg *regs, u32 len)
{
struct i2c_client *client = v4l2_get_subdevdata(>sd);
int ret;
@@ -1248,8 +1248,8 @@ static const struct v4l2_ctrl_ops imx355_ctrl_ops = {
};
static int imx355_enum_mbus_code(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_mbus_code_enum *code)
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_mbus_code_enum *code)
{
struct imx355 *imx355 = to_imx355(sd);
@@ -1264,8 +1264,8 @@ static int imx355_enum_mbus_code(struct v4l2_subdev *sd,
}
static int imx355_enum_frame_size(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_frame_size_enum *fse)
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
{
struct imx355 *imx355 = to_imx355(sd);
@@ -1298,8 +1298,8 @@ static void imx355_update_pad_format(struct imx355
*imx355,
}
static int imx355_do_get_pad_format(struct imx355 *imx355,
-struct v4l2_subdev_pad_config *cfg,
-struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
{
struct v4l2_mbus_framefmt *framefmt;
struct v4l2_subdev *sd = >sd;
@@ -1315,8 +1315,8 @@ static int imx355_do_get_pad_format(struct imx355 *imx355,
}
static int imx355_get_pad_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_format *fmt)
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *fmt)
{
struct imx355 *imx355 = to_imx355(sd);
int ret;
@@ -1330,8 +1330,8 @@ static int imx355_get_pad_format(struct v4l2_subdev *sd,
static int
imx355_set_pad_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
{
struct imx355 *imx355 = to_imx355(sd);
const struct imx355_mode *mode;
@@ -1680,7 +1680,7 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct
device *dev)
goto out_err;
ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
- >ext_clk);
+ >ext_clk);
if (ret) {
dev_err(dev, "can't get clock frequency");
goto out_err;
@@ -1689,7 +1689,7 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct
device *dev)
dev_dbg(dev, "ext clk: %d", cfg->ext_clk);
if (cfg->ext_clk != IMX355_EXT_CLK) {
dev_err(dev, "external clock %d is not supported",
-cfg->ext_clk);
+ cfg->ext_clk);
goto out_err;
}
@@ -1700,9 +1700,9 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct
device *dev)
}
cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
- cfg->link_freqs = devm_kcalloc(
- dev, bus_cfg.nr_of_link_frequencies + 1,
- sizeof(*cfg->link_freqs), GFP_KERNEL);
+ cfg->link_freqs = devm_kcalloc(dev,
+ bus_cfg.nr_of_link_frequencies + 1,
+ sizeof(*cfg->link_freqs), GFP_KERNEL);
if (!cfg->link_freqs)
goto out_err;
--
2.17.1
[PATCH] media: imx319: fix a few coding style issues
Function alignments are off by 1 space, as reported by
checkpatch.pl --strict.
Fix those.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/i2c/imx319.c | 32
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 329049f7e64d..0d3e27812b93 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1836,7 +1836,7 @@ static int imx319_write_reg(struct imx319 *imx319, u16
reg, u32 len, u32 val)
/* Write a list of registers */
static int imx319_write_regs(struct imx319 *imx319,
- const struct imx319_reg *regs, u32 len)
+const struct imx319_reg *regs, u32 len)
{
struct i2c_client *client = v4l2_get_subdevdata(>sd);
int ret;
@@ -1947,8 +1947,8 @@ static const struct v4l2_ctrl_ops imx319_ctrl_ops = {
};
static int imx319_enum_mbus_code(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_mbus_code_enum *code)
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_mbus_code_enum *code)
{
struct imx319 *imx319 = to_imx319(sd);
@@ -1963,8 +1963,8 @@ static int imx319_enum_mbus_code(struct v4l2_subdev *sd,
}
static int imx319_enum_frame_size(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_frame_size_enum *fse)
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
{
struct imx319 *imx319 = to_imx319(sd);
@@ -1997,8 +1997,8 @@ static void imx319_update_pad_format(struct imx319
*imx319,
}
static int imx319_do_get_pad_format(struct imx319 *imx319,
-struct v4l2_subdev_pad_config *cfg,
-struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
{
struct v4l2_mbus_framefmt *framefmt;
struct v4l2_subdev *sd = >sd;
@@ -2014,8 +2014,8 @@ static int imx319_do_get_pad_format(struct imx319 *imx319,
}
static int imx319_get_pad_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_format *fmt)
+struct v4l2_subdev_pad_config *cfg,
+struct v4l2_subdev_format *fmt)
{
struct imx319 *imx319 = to_imx319(sd);
int ret;
@@ -2029,8 +2029,8 @@ static int imx319_get_pad_format(struct v4l2_subdev *sd,
static int
imx319_set_pad_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
{
struct imx319 *imx319 = to_imx319(sd);
const struct imx319_mode *mode;
@@ -2380,7 +2380,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
device *dev)
goto out_err;
ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
- >ext_clk);
+ >ext_clk);
if (ret) {
dev_err(dev, "can't get clock frequency");
goto out_err;
@@ -2389,7 +2389,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
device *dev)
dev_dbg(dev, "ext clk: %d", cfg->ext_clk);
if (cfg->ext_clk != IMX319_EXT_CLK) {
dev_err(dev, "external clock %d is not supported",
-cfg->ext_clk);
+ cfg->ext_clk);
goto out_err;
}
@@ -2400,9 +2400,9 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
device *dev)
}
cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
- cfg->link_freqs = devm_kcalloc(
- dev, bus_cfg.nr_of_link_frequencies + 1,
- sizeof(*cfg->link_freqs), GFP_KERNEL);
+ cfg->link_freqs = devm_kcalloc(dev,
+ bus_cfg.nr_of_link_frequencies + 1,
+ sizeof(*cfg->link_freqs), GFP_KERNEL);
if (!cfg->link_freqs)
goto out_err;
--
2.17.1
[GIT PULL FOR v4.20] Various cec fixes
This pull request fixes various CEC bugs. The following patches are also CCed to stable for 4.18: cec: add new tx/rx status bits to detect aborts/timeouts adv7604: when the EDID is cleared, unconfigure CEC as well adv7842: when the EDID is cleared, unconfigure CEC as well cec: fix the Signal Free Time calculation The 'add new tx/rx status bits' is strictly speaking not a bug fix, but the absence of these status bits made finding the real bug (https://patchwork.linuxtv.org/patch/52329/) much harder than it should have been. Regards, Hans The following changes since commit f492fb4f5b41e8e62051e710369320e9ffa7a1ea: media: MAINTAINERS: Fix entry for the renamed dw9807 driver (2018-10-05 08:40:00 -0400) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git tags/br-cec-media2 for you to fetch changes up to 75773002b711836690aed39d6702b87165136aa5: media: cec: name for RC passthrough device does not need 'RC for' (2018-10-05 15:54:06 +0200) Tag branch Hans Verkuil (6): cec-core.rst: improve cec_transmit_done documentation cec: add new tx/rx status bits to detect aborts/timeouts adv7604: when the EDID is cleared, unconfigure CEC as well adv7842: when the EDID is cleared, unconfigure CEC as well cec: fix the Signal Free Time calculation cec-gpio: select correct Signal Free Time Sean Young (1): media: cec: name for RC passthrough device does not need 'RC for' Documentation/media/kapi/cec-core.rst| 4 Documentation/media/uapi/cec/cec-ioc-receive.rst | 25 ++-- drivers/media/cec/cec-adap.c | 92 --- drivers/media/cec/cec-core.c | 6 ++--- drivers/media/cec/cec-pin.c | 20 drivers/media/i2c/adv7604.c | 4 +++- drivers/media/i2c/adv7842.c | 4 +++- include/media/cec.h | 4 +--- include/uapi/linux/cec.h | 3 +++ 9 files changed, 84 insertions(+), 78 deletions(-)
[GIT PULL for 4.20] Ov7670 and ov9650 fixes
Hi Mauro, Here are fixes for the Omnivision sensor drivers. Please pull. The following changes since commit f492fb4f5b41e8e62051e710369320e9ffa7a1ea: media: MAINTAINERS: Fix entry for the renamed dw9807 driver (2018-10-05 08:40:00 -0400) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.20-9-sign-2 for you to fetch changes up to f974cffdf34ec08ac0b27ace938c07f4aa078b4c: media: ov5640: fix framerate update (2018-10-05 16:04:12 +0300) ov sensor driver fixes Arnd Bergmann (1): media: ov9650: avoid maybe-uninitialized warnings Hugues Fruchet (1): media: ov5640: fix framerate update Lubomir Rintel (1): ov7670: make "xclk" clock optional drivers/media/i2c/ov5640.c | 7 --- drivers/media/i2c/ov7670.c | 27 +-- drivers/media/i2c/ov9650.c | 2 ++ 3 files changed, 23 insertions(+), 13 deletions(-) -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [RFC] V4L2_PIX_FMT_MJPEG vs V4L2_PIX_FMT_JPEG
Hi, On 05-10-18 13:55, Mauro Carvalho Chehab wrote: Em Mon, 1 Oct 2018 18:19:21 +0100 Dave Stevenson escreveu: Hi All, On Mon, 1 Oct 2018 at 17:32, Ezequiel Garcia wrote: Hi Hans, Thanks for looking into. I remember MJPEG vs. JPEG being a source of confusion for me a few years ago, so clarification is greatly welcome :-) On Mon, 2018-10-01 at 15:03 +0300, Laurent Pinchart wrote: Hi Hans, On Monday, 1 October 2018 14:54:29 EEST Hans Verkuil wrote: On 10/01/2018 01:48 PM, Laurent Pinchart wrote: On Monday, 1 October 2018 11:43:04 EEST Hans Verkuil wrote: It turns out that we have both JPEG and Motion-JPEG pixel formats defined. Furthermore, some drivers support one, some the other and some both. These pixelformats both mean the same. Do they ? I thought MJPEG was JPEG using fixed Huffman tables that were not included in the JPEG headers. I'm not aware of any difference. If there is one, then it is certainly not documented. What I can tell for sure is that many UVC devices don't include Huffman tables in their JPEG headers. Ezequiel, since you've been working with this recently, do you know anything about this? JPEG frames must include huffman and quantization tables, as per the standard. AFAIK, there's no MJPEG specification per-se and vendors specify its own way of conveying a Motion JPEG stream. There is the specfication for MJPEG in Quicktime containers, which defines the MJPEG-A and MJPEG-B variants [1]. MJPEG-B is not a concatenation of JPEG frames as the framing is different, so can't really be combined into V4L2_PIX_FMT_JPEG. Have people encountered devices that produce MJPEG-A or MJPEG-B via V4L2? I haven't, but I have been forced to support both variants on decode. Checking it is not an easy task. I *suspect* that those cameras are all MJPEG-A, as the libv4l decoder uses tinyjpeg library to handle both JPEG and MJPEG. Maybe Hans de Goede knows more about that, and may have actually tested it with different camera models. I've tested the JPG path in libv4l with quite a lot of cameras and sofar it has worked for all of them. There are some non UVC cameras where the hardware produces raw JPG data, but in that case the kernel driver prefixes a JPG header to each frame so that it looks like a regular JPG. Regards, Hans On that thought, whilst capture devices generally don't care, is there a need to differentiate for M2M codec devices which can support encoding the variants? Or likewise on M2M decoders that support only JPEG, how do they tell userspace that they don't support MJPEG-A or MJPEG-B? Dave [1] https://developer.apple.com/standards/qtff-2001.pdf For instance, omiting the huffman table seems to be a vendor thing. Microsoft explicitly omits the huffman tables from each frame: https://www.fileformat.info/format/bmp/spec/b7c72ebab8064da48ae5ed0c053c67a4/view.htm Others could be following the same things. Like I mentioned before, Gstreamer always check for missing huffman table and adds one if missing. Gstreamer has other quirks for missing markers, e.g. deal with a missing EOI: https://github.com/GStreamer/gst-plugins-good/commit/10ff3c8e14e8fba9e0a5d696dce0bea27de644d7 I think Hans suggestion of settling on JPEG makes sense and it would be consistent with Gstreamer. Otherwise, we should specify exactly what we understand by MJPEG, but I don't think it's worth it. Thanks, Ezequiel Thanks, Mauro
[GIT PULL v2 for 4.20] Unlocked V4L2 control grab, imx{319, 355} drivers
Hi Mauro,
Here are drivers for Sony imx319 and imx355 sensors and an unlocked version
of v4l2_ctrl_grab() which is used by the driver.
Since v1, I've rebased this on the current master --- with the fwnode
patches.
Please pull.
The diff is here and effectively the same for both drivers:
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 37c31d17ecf0..329049f7e64d 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2356,7 +2356,9 @@ static int imx319_init_controls(struct imx319 *imx319)
static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
{
struct imx319_hwcfg *cfg;
- struct v4l2_fwnode_endpoint *bus_cfg;
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
struct fwnode_handle *ep;
struct fwnode_handle *fwnode = dev_fwnode(dev);
unsigned int i;
@@ -2369,8 +2371,8 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
device *dev)
if (!ep)
return NULL;
- bus_cfg = v4l2_fwnode_endpoint_alloc_parse(ep);
- if (IS_ERR(bus_cfg))
+ ret = v4l2_fwnode_endpoint_alloc_parse(ep, _cfg);
+ if (ret)
goto out_err;
cfg = devm_kzalloc(dev, sizeof(*cfg), GFP_KERNEL);
@@ -2391,30 +2393,30 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
device *dev)
goto out_err;
}
- dev_dbg(dev, "num of link freqs: %d", bus_cfg->nr_of_link_frequencies);
- if (!bus_cfg->nr_of_link_frequencies) {
+ dev_dbg(dev, "num of link freqs: %d", bus_cfg.nr_of_link_frequencies);
+ if (!bus_cfg.nr_of_link_frequencies) {
dev_warn(dev, "no link frequencies defined");
goto out_err;
}
- cfg->nr_of_link_freqs = bus_cfg->nr_of_link_frequencies;
+ cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
cfg->link_freqs = devm_kcalloc(
- dev, bus_cfg->nr_of_link_frequencies + 1,
+ dev, bus_cfg.nr_of_link_frequencies + 1,
sizeof(*cfg->link_freqs), GFP_KERNEL);
if (!cfg->link_freqs)
goto out_err;
- for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
- cfg->link_freqs[i] = bus_cfg->link_frequencies[i];
+ for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+ cfg->link_freqs[i] = bus_cfg.link_frequencies[i];
dev_dbg(dev, "link_freq[%d] = %lld", i, cfg->link_freqs[i]);
}
- v4l2_fwnode_endpoint_free(bus_cfg);
+ v4l2_fwnode_endpoint_free(_cfg);
fwnode_handle_put(ep);
return cfg;
out_err:
- v4l2_fwnode_endpoint_free(bus_cfg);
+ v4l2_fwnode_endpoint_free(_cfg);
fwnode_handle_put(ep);
return NULL;
}
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index 3baa0edc57a9..803df2a014bb 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -1656,7 +1656,9 @@ static int imx355_init_controls(struct imx355 *imx355)
static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
{
struct imx355_hwcfg *cfg;
- struct v4l2_fwnode_endpoint *bus_cfg;
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
struct fwnode_handle *ep;
struct fwnode_handle *fwnode = dev_fwnode(dev);
unsigned int i;
@@ -1669,8 +1671,8 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct
device *dev)
if (!ep)
return NULL;
- bus_cfg = v4l2_fwnode_endpoint_alloc_parse(ep);
- if (IS_ERR(bus_cfg))
+ ret = v4l2_fwnode_endpoint_alloc_parse(ep, _cfg);
+ if (ret)
goto out_err;
cfg = devm_kzalloc(dev, sizeof(*cfg), GFP_KERNEL);
@@ -1691,30 +1693,30 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct
device *dev)
goto out_err;
}
- dev_dbg(dev, "num of link freqs: %d", bus_cfg->nr_of_link_frequencies);
- if (!bus_cfg->nr_of_link_frequencies) {
+ dev_dbg(dev, "num of link freqs: %d", bus_cfg.nr_of_link_frequencies);
+ if (!bus_cfg.nr_of_link_frequencies) {
dev_warn(dev, "no link frequencies defined");
goto out_err;
}
- cfg->nr_of_link_freqs = bus_cfg->nr_of_link_frequencies;
+ cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
cfg->link_freqs = devm_kcalloc(
- dev, bus_cfg->nr_of_link_frequencies + 1,
+ dev, bus_cfg.nr_of_link_frequencies + 1,
sizeof(*cfg->link_freqs), GFP_KERNEL);
if (!cfg->link_freqs)
goto out_err;
- for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
- cfg->link_freqs[i] = bus_cfg->link_frequencies[i];
+ for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+ cfg->link_freqs[i] = bus_cfg.link_frequencies[i];
Re: [PATCH v7 4/6] media: Add JPEG_RAW format
Em Fri, 05 Oct 2018 08:53:14 -0300 Ezequiel Garcia escreveu: > On Fri, 2018-10-05 at 08:09 -0300, Mauro Carvalho Chehab wrote: > > Em Thu, 4 Oct 2018 21:12:24 -0300 > > Ezequiel Garcia escreveu: > > > > > From: Shunqian Zheng > > > > > > Add V4L2_PIX_FMT_JPEG_RAW format that does not contain > > > JPEG header in the output frame. > > > > > > Signed-off-by: Shunqian Zheng > > > Signed-off-by: Ezequiel Garcia > > > --- > > > Documentation/media/uapi/v4l/pixfmt-compressed.rst | 9 + > > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > > > include/uapi/linux/videodev2.h | 1 + > > > 3 files changed, 11 insertions(+) > > > > > > diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > > b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > > index ba0f6c49d9bf..ad73076276ec 100644 > > > --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > > +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > > @@ -23,6 +23,15 @@ Compressed Formats > > >- 'JPEG' > > >- TBD. See also :ref:`VIDIOC_G_JPEGCOMP `, > > > :ref:`VIDIOC_S_JPEGCOMP `. > > > +* .. _V4L2-PIX-FMT-JPEG-RAW: > > > + > > > + - ``V4L2_PIX_FMT_JPEG_RAW`` > > > + - 'Raw JPEG' > > > + - Raw JPEG bitstream, containing a compressed payload. This format > > > +contains an image scan, i.e. without any metadata or headers. > > > +The user is expected to set the needed metadata such as > > > +quantization and entropy encoding tables, via ``V4L2_CID_JPEG`` > > > +controls, see :ref:`jpeg-control-id`. > > > > IMO, it is not very clear when someone should use V4L2_CID_JPEG or > > V4L2_PIX_FMT_JPEG_RAW. Some drivers do add a JPEG header internally. > > > > For device drivers, if the hardware can parse JPEG frames, then > V4L2_PIX_FMT_JPEG should be used. Otherwise, if the hardware can > only accept a parsed JPEG (i.e. payload and tables), then > only V4L2_PIX_FMT_JPEG_RAW should be supported. It seems you're talking here about V4L2 output devices. I'm more concerned about its usage on V4L2 capture devices. Yet, it should be possible to use it with both, as formats should be generic enough to work on both directions. > Parsing headers in the driver is discouraged by the stateful codec > API specification. True, but we do parsing/adding headers for some formats in Kernel space, as this is a way to abstract the hardware for applications. That's specially true for MPEG and JPEG formats. > With the Request API in place, and the stateful and stateless specs, > device driver writers should be now using the right model for each > type of hardware. > > There are exceptions, though. If the hardware handles full JPEG frames, > but requires some extra parsing on the OS side, then the driver > should be using V4L2_PIX_FMT_JPEG, and doing some extra parsing. > For instance, mtk-jpeg seems to work like this. Actually, all existing drivers do this. I'm not saying that this the right/best model, but it is the way it is. Changing it is possible, but only if it can be done in a driver-independent way and userspace code inside a library exists to support it. > > > Also, if we're now starting to accept headerless JPEG images, you should > > very patch libv4l as well, in order to accept this new format. > > > > Right. > > Thanks, > Eze Thanks, Mauro
Re: [PATCH v6 0/6] Add Rockchip VPU JPEG encoder
Em Thu, 04 Oct 2018 20:39:31 -0300
Ezequiel Garcia escreveu:
> On Mon, 2018-10-01 at 14:54 -0300, Ezequiel Garcia wrote:
> > On Fri, 2018-09-28 at 14:33 +0200, Hans Verkuil wrote:
> > > On 09/17/2018 07:30 PM, Ezequiel Garcia wrote:
> > > > This series adds support for JPEG encoding via the VPU block
> > > > present in Rockchip platforms. Currently, support for RK3288
> > > > and RK3399 is included.
> > > >
> > > > Please, see the previous versions of this cover letter for
> > > > more information.
> > > >
> > > > Compared to v5, the only change is in the V4L2_CID_JPEG_QUANTIZATION
> > > > control. We've decided to support only baseline profile,
> > > > and only add 8-bit luma and chroma tables.
> > > >
> > > > struct v4l2_ctrl_jpeg_quantization {
> > > >__u8luma_quantization_matrix[64];
> > > >__u8chroma_quantization_matrix[64];
> > > > };
> > > >
> > > > By doing this, it's clear that we don't currently support anything
> > > > but baseline.
> > > >
> > > > This series should apply cleanly on top of
> > > >
> > > > git://linuxtv.org/hverkuil/media_tree.git br-cedrus tag.
> > > >
> > > > If everyone is happy with this series, I'd like to route the devicetree
> > > > changes through the rockchip tree, and the rest via the media
> > > > subsystem.
> > >
> > > OK, so I have what is no doubt an annoying question: do we really need
> > > a JPEG_RAW format?
> > >
> >
> > Not annoying, as it helps clarify a few things :-)
> > I think we do need the JPEG_RAW format. The way I see it, using
> > JPEG opens a can of worms...
> >
> > > The JPEG header is really easy to parse for a decoder and really easy to
> > > prepend to the compressed image for the encoder.
> > >
> > > The only reason I can see for a JPEG_RAW is if the image must start at
> > > some specific address alignment. Although even then you can just pad the
> > > JPEG header that you will add according to the alignment requirements.
> > >
> > > I know I am very late with this question, but I never looked all that
> > > closely at what a JPEG header looks like. But this helped:
> > >
> > > https://en.wikipedia.org/wiki/JPEG_File_Interchange_Format
> > >
> > > and it doesn't seem difficult at all to parse or create the header.
> > >
> > >
> >
> > ... I think that having JPEG_RAW as the compressed format
> > is much more clear for userspace, as it explicitly specifies
> > what is expected.
> >
> > This way, for a stateless encoder, applications are required
> > to set quantization and/or entropy tables, and are then in
> > charge of using the compressed JPEG_RAW payload in whatever form
> > they want. Stupid simple.
> >
> > On the other side, if the stateless encoder driver supports
> > JPEG (creating headers in-kernel), it means that:
> >
> > *) applications should pass a quality control, if supported,
> > and the driver will use hardcoded tables or...
> >
> > *) applications pass quantization control and, if supported,
> > entropy control. The kernel uses them to create the JPEG frame.
> > But also, some drivers (e.g. Rockchip), use default entropy
> > tables, which should now be in the kernel.
> >
> > So the application would have to query controls to find out
> > what to do. Not exactly hard, but I think having the JPEG_RAW
> > is much simpler and more clear.
> >
> > Now, for stateless decoders, supporting JPEG_RAW means
> > the application has to pass quantization and entropy
> > controls, probably using the Request API.
> > Given the application has parsed the JPEG,
> > it knows the width and height and can request
> > buffers accordingly.
> >
> > The hardware is stateless, and so is the driver.
> >
> > On the other hand, supporting JPEG would mean that
> > drivers will have to parse the image, extracting
> > the quantization and entropy tables.
> >
> > Format negotiation is now more complex,
> > either we follow the stateful spec, introducing a little
> > state machine in the driver... or we use the Request API,
> > but that means parsing on both sides kernel and application.
> >
> > Either way, using JPEG_RAW is just waaay simpler and puts
> > things where they belong.
> >
>
> As discussed on IRC, I'm sending a v7 for this series,
> fixing only the checkpatch issues and the extra line in the
> binding document.
>
> We've agreed to move forward with JPEG_RAW, for the reasons
> stated above.
>
> Plus, we've agreed to keep this in staging until the userspace
> support for JPEG_RAW format is clear.
The problem is that a new format is actually a V4L2 core change, and
not something for staging.
IMHO, it is prudent to wait for an userspace code before hushing
merging this upstream.
-
I'm concerned about adding a JPEG_RAW format. We had this discussion
in the past. On that time, it was opted to add a code inside the
drivers that will be adding the jpeg headers, on the cases where
the hardware was providing headerless frames.
Among the reasons, different drivers may be
Re: [PATCH v7 2/6] ARM: dts: rockchip: add VPU device node for RK3288
Am Freitag, 5. Oktober 2018, 02:12:22 CEST schrieb Ezequiel Garcia: > Add the Video Processing Unit node for RK3288 SoC. > > Fix the VPU IOMMU node, which was disabled and lacking > its power domain property. > > Signed-off-by: Ezequiel Garcia applied for 4.20 (may possibly move to 4.21 though) after moving power-domain* below (#)iommu* to keep alphabetical sorting. Thanks Heiko
Re: [PATCH v7 3/6] arm64: dts: rockchip: add VPU device node for RK3399
Am Freitag, 5. Oktober 2018, 02:12:23 CEST schrieb Ezequiel Garcia: > Add the Video Processing Unit node for the RK3399 SoC. > > Also, fix the VPU IOMMU node, which was disabled and lacking > its power domain property. > > Signed-off-by: Ezequiel Garcia applied for 4.20 (may possibly move to 4.21 though) after moving power-domain* below (#)iommu* to keep alphabetical sorting. Thanks Heiko
Re: [RFC] V4L2_PIX_FMT_MJPEG vs V4L2_PIX_FMT_JPEG
Em Mon, 1 Oct 2018 18:19:21 +0100 Dave Stevenson escreveu: > Hi All, > > On Mon, 1 Oct 2018 at 17:32, Ezequiel Garcia wrote: > > > > Hi Hans, > > > > Thanks for looking into. I remember MJPEG vs. JPEG being a source > > of confusion for me a few years ago, so clarification is greatly > > welcome :-) > > > > On Mon, 2018-10-01 at 15:03 +0300, Laurent Pinchart wrote: > > > Hi Hans, > > > > > > On Monday, 1 October 2018 14:54:29 EEST Hans Verkuil wrote: > > > > On 10/01/2018 01:48 PM, Laurent Pinchart wrote: > > > > > On Monday, 1 October 2018 11:43:04 EEST Hans Verkuil wrote: > > > > > > It turns out that we have both JPEG and Motion-JPEG pixel formats > > > > > > defined. > > > > > > > > > > > > Furthermore, some drivers support one, some the other and some both. > > > > > > > > > > > > These pixelformats both mean the same. > > > > > > > > > > Do they ? I thought MJPEG was JPEG using fixed Huffman tables that > > > > > were > > > > > not included in the JPEG headers. > > > > > > > > I'm not aware of any difference. If there is one, then it is certainly > > > > not > > > > documented. > > > > > > What I can tell for sure is that many UVC devices don't include Huffman > > > tables > > > in their JPEG headers. > > > > > > > Ezequiel, since you've been working with this recently, do you know > > > > anything > > > > about this? > > > > > > > > > > JPEG frames must include huffman and quantization tables, as per the > > standard. > > > > AFAIK, there's no MJPEG specification per-se and vendors specify its own > > way of conveying a Motion JPEG stream. > > There is the specfication for MJPEG in Quicktime containers, which > defines the MJPEG-A and MJPEG-B variants [1]. > MJPEG-B is not a concatenation of JPEG frames as the framing is > different, so can't really be combined into V4L2_PIX_FMT_JPEG. > Have people encountered devices that produce MJPEG-A or MJPEG-B via > V4L2? I haven't, but I have been forced to support both variants on > decode. Checking it is not an easy task. I *suspect* that those cameras are all MJPEG-A, as the libv4l decoder uses tinyjpeg library to handle both JPEG and MJPEG. Maybe Hans de Goede knows more about that, and may have actually tested it with different camera models. > > On that thought, whilst capture devices generally don't care, is there > a need to differentiate for M2M codec devices which can support > encoding the variants? Or likewise on M2M decoders that support only > JPEG, how do they tell userspace that they don't support MJPEG-A or > MJPEG-B? > > Dave > > [1] https://developer.apple.com/standards/qtff-2001.pdf > > > For instance, omiting the huffman table seems to be a vendor thing. > > Microsoft > > explicitly omits the huffman tables from each frame: > > > > https://www.fileformat.info/format/bmp/spec/b7c72ebab8064da48ae5ed0c053c67a4/view.htm > > > > Others could be following the same things. > > > > Like I mentioned before, Gstreamer always check for missing huffman table > > and adds one if missing. Gstreamer has other quirks for missing markers, > > e.g. deal with a missing EOI: > > > > https://github.com/GStreamer/gst-plugins-good/commit/10ff3c8e14e8fba9e0a5d696dce0bea27de644d7 > > > > I think Hans suggestion of settling on JPEG makes sense and it would > > be consistent with Gstreamer. Otherwise, we should specify exactly what we > > understand by MJPEG, but I don't think it's worth it. > > > > Thanks, > > Ezequiel Thanks, Mauro
Re: [PATCH v7 4/6] media: Add JPEG_RAW format
On Fri, 2018-10-05 at 08:09 -0300, Mauro Carvalho Chehab wrote: > Em Thu, 4 Oct 2018 21:12:24 -0300 > Ezequiel Garcia escreveu: > > > From: Shunqian Zheng > > > > Add V4L2_PIX_FMT_JPEG_RAW format that does not contain > > JPEG header in the output frame. > > > > Signed-off-by: Shunqian Zheng > > Signed-off-by: Ezequiel Garcia > > --- > > Documentation/media/uapi/v4l/pixfmt-compressed.rst | 9 + > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > > include/uapi/linux/videodev2.h | 1 + > > 3 files changed, 11 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > index ba0f6c49d9bf..ad73076276ec 100644 > > --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > > @@ -23,6 +23,15 @@ Compressed Formats > >- 'JPEG' > >- TBD. See also :ref:`VIDIOC_G_JPEGCOMP `, > > :ref:`VIDIOC_S_JPEGCOMP `. > > +* .. _V4L2-PIX-FMT-JPEG-RAW: > > + > > + - ``V4L2_PIX_FMT_JPEG_RAW`` > > + - 'Raw JPEG' > > + - Raw JPEG bitstream, containing a compressed payload. This format > > +contains an image scan, i.e. without any metadata or headers. > > +The user is expected to set the needed metadata such as > > +quantization and entropy encoding tables, via ``V4L2_CID_JPEG`` > > +controls, see :ref:`jpeg-control-id`. > > IMO, it is not very clear when someone should use V4L2_CID_JPEG or > V4L2_PIX_FMT_JPEG_RAW. Some drivers do add a JPEG header internally. > For device drivers, if the hardware can parse JPEG frames, then V4L2_PIX_FMT_JPEG should be used. Otherwise, if the hardware can only accept a parsed JPEG (i.e. payload and tables), then only V4L2_PIX_FMT_JPEG_RAW should be supported. Parsing headers in the driver is discouraged by the stateful codec API specification. With the Request API in place, and the stateful and stateless specs, device driver writers should be now using the right model for each type of hardware. There are exceptions, though. If the hardware handles full JPEG frames, but requires some extra parsing on the OS side, then the driver should be using V4L2_PIX_FMT_JPEG, and doing some extra parsing. For instance, mtk-jpeg seems to work like this. > Also, if we're now starting to accept headerless JPEG images, you should > very patch libv4l as well, in order to accept this new format. > Right. Thanks, Eze
Re: [RFC] V4L2_PIX_FMT_MJPEG vs V4L2_PIX_FMT_JPEG
Em Mon, 01 Oct 2018 08:42:56 -0400 Nicolas Dufresne escreveu: > Hello Hans, > > Le lundi 01 octobre 2018 à 10:43 +0200, Hans Verkuil a écrit : > > It turns out that we have both JPEG and Motion-JPEG pixel formats defined. > > > > Furthermore, some drivers support one, some the other and some both. > > > > These pixelformats both mean the same. > > > > I propose that we settle on JPEG (since it seems to be used most often) and > > add JPEG support to those drivers that currently only use MJPEG. > > Thanks for looking into this. As per GStreamer code, I see 3 alias for > JPEG. V4L2_PIX_FMT_MJPEG/JPEG/PJPG. I don't know the context, this code > was written before I knew GStreamer existed. It's possible there is a > subtle difference, I have never looked at it, but clearly all our JPEG > decoder handle these as being the same. > > https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.c#n956 The code at libv4l handles both MJPEG and JPEG the same way. PJPG is handled somewhat differently (although it uses the same code). There is a code there that cleanups some Pixart-specific headers. Thanks, Mauro
[PATCH -next] [media] media: drop pointless static qualifier in vpfe_ipipeif_init()
There is no need to have the 'resource_size_t res_len' variable static since new value always be assigned before use it. Signed-off-by: YueHaibing --- drivers/staging/media/davinci_vpfe/dm365_ipipeif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c b/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c index a53231b..e191829 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c @@ -998,7 +998,7 @@ int vpfe_ipipeif_init(struct vpfe_ipipeif_device *ipipeif, struct v4l2_subdev *sd = >subdev; struct media_pad *pads = >pads[0]; struct media_entity *me = >entity; - static resource_size_t res_len; + resource_size_t res_len; struct resource *res; int ret;
[PATCH -next] [media] media: drop pointless static qualifier in vpfe_resizer_init()
There is no need to have the 'resource_size_t res_len' variable static since new value always be assigned before use it. Signed-off-by: YueHaibing --- drivers/staging/media/davinci_vpfe/dm365_resizer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c b/drivers/staging/media/davinci_vpfe/dm365_resizer.c index aac6dbf..b2b23a7 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c +++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c @@ -1884,7 +1884,7 @@ int vpfe_resizer_init(struct vpfe_resizer_device *vpfe_rsz, struct v4l2_subdev *sd = _rsz->crop_resizer.subdev; struct media_pad *pads = _rsz->crop_resizer.pads[0]; struct media_entity *me = >entity; - static resource_size_t res_len; + resource_size_t res_len; struct resource *res; int ret;
Re: [PATCH v7 4/6] media: Add JPEG_RAW format
Em Thu, 4 Oct 2018 21:12:24 -0300 Ezequiel Garcia escreveu: > From: Shunqian Zheng > > Add V4L2_PIX_FMT_JPEG_RAW format that does not contain > JPEG header in the output frame. > > Signed-off-by: Shunqian Zheng > Signed-off-by: Ezequiel Garcia > --- > Documentation/media/uapi/v4l/pixfmt-compressed.rst | 9 + > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > include/uapi/linux/videodev2.h | 1 + > 3 files changed, 11 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > index ba0f6c49d9bf..ad73076276ec 100644 > --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > @@ -23,6 +23,15 @@ Compressed Formats >- 'JPEG' >- TBD. See also :ref:`VIDIOC_G_JPEGCOMP `, > :ref:`VIDIOC_S_JPEGCOMP `. > +* .. _V4L2-PIX-FMT-JPEG-RAW: > + > + - ``V4L2_PIX_FMT_JPEG_RAW`` > + - 'Raw JPEG' > + - Raw JPEG bitstream, containing a compressed payload. This format > +contains an image scan, i.e. without any metadata or headers. > +The user is expected to set the needed metadata such as > +quantization and entropy encoding tables, via ``V4L2_CID_JPEG`` > +controls, see :ref:`jpeg-control-id`. IMO, it is not very clear when someone should use V4L2_CID_JPEG or V4L2_PIX_FMT_JPEG_RAW. Some drivers do add a JPEG header internally. Also, if we're now starting to accept headerless JPEG images, you should very patch libv4l as well, in order to accept this new format. Regards, Thanks, Mauro
Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
Hi Hans, I like this series, nice work! On 2018-10-05 09:49:00 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > This patch series converts the last remaining drivers that use g/s_crop and > cropcap to g/s_selection. > > The first two patches do some minor code cleanup. > > The third patch adds a new video_device flag to indicate that the driver > inverts the normal usage of g/s_crop/cropcap. This applies to the old > Samsung drivers that predate the Selection API and that abused the existing > crop API. > > The next three patches do some code cleanup and prepare drivers for the > removal of g/s_crop and ensure that cropcap only returns the pixelaspect. > > The next three patches convert the remaining Samsung drivers and set the > QUIRK flag for all three. > > The final two patches remove vidioc_g/s_crop and rename vidioc_cropcap > to vidioc_g_pixelaspect. > > I would really appreciate it if someone from Samsung can test these > three drivers or at the very least review the code. > > Niklas, this series supersedes your 'v4l2-ioctl: fix CROPCAP type handling' > patch. Sorry about that :-) No worries, I'm happy my tests run without errors again :-) If appropriate fell free to add for the v4l2 and rcar-vin portions: Tested-by: Niklas Söderlund > > Regards, > > Hans > > Hans Verkuil (11): > v4l2-ioctl: don't use CROP/COMPOSE_ACTIVE > v4l2-common.h: put backwards compat defines under #ifndef __KERNEL__ > v4l2-ioctl: add QUIRK_INVERTED_CROP > davinci/vpbe: drop unused g_cropcap > cropcap/g_selection split > exynos-gsc: replace v4l2_crop by v4l2_selection > s5p_mfc_dec.c: convert g_crop to g_selection > exynos4-is: convert g/s_crop to g/s_selection > s5p-g2d: convert g/s_crop to g/s_selection > v4l2-ioctl: remove unused vidioc_g/s_crop > vidioc_cropcap -> vidioc_g_pixelaspect > > drivers/media/pci/bt8xx/bttv-driver.c | 12 +- > drivers/media/pci/cobalt/cobalt-v4l2.c| 48 +-- > drivers/media/pci/cx18/cx18-ioctl.c | 13 +- > drivers/media/pci/cx23885/cx23885-video.c | 40 -- > drivers/media/pci/ivtv/ivtv-ioctl.c | 17 +-- > drivers/media/pci/saa7134/saa7134-video.c | 21 ++- > drivers/media/platform/am437x/am437x-vpfe.c | 31 ++--- > drivers/media/platform/davinci/vpbe.c | 23 > drivers/media/platform/davinci/vpbe_display.c | 10 +- > drivers/media/platform/davinci/vpfe_capture.c | 12 +- > drivers/media/platform/exynos-gsc/gsc-core.c | 57 +++- > drivers/media/platform/exynos-gsc/gsc-core.h | 3 +- > drivers/media/platform/exynos-gsc/gsc-m2m.c | 23 ++-- > drivers/media/platform/exynos4-is/fimc-core.h | 6 +- > drivers/media/platform/exynos4-is/fimc-m2m.c | 130 ++ > drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 +- > drivers/media/platform/s5p-g2d/g2d.c | 102 +- > drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 + > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 49 --- > drivers/media/platform/vivid/vivid-core.c | 9 +- > drivers/media/platform/vivid/vivid-vid-cap.c | 18 ++- > drivers/media/platform/vivid/vivid-vid-cap.h | 2 +- > drivers/media/platform/vivid/vivid-vid-out.c | 18 ++- > drivers/media/platform/vivid/vivid-vid-out.h | 2 +- > drivers/media/usb/au0828/au0828-video.c | 38 +++-- > drivers/media/usb/cpia2/cpia2_v4l.c | 31 +++-- > drivers/media/usb/cx231xx/cx231xx-417.c | 41 -- > drivers/media/usb/cx231xx/cx231xx-video.c | 41 -- > drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 13 +- > drivers/media/v4l2-core/v4l2-dev.c| 8 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 44 -- > include/media/davinci/vpbe.h | 4 - > include/media/v4l2-dev.h | 13 +- > include/media/v4l2-ioctl.h| 16 +-- > include/uapi/linux/v4l2-common.h | 28 ++-- > 35 files changed, 537 insertions(+), 397 deletions(-) > > -- > 2.18.0 > -- Regards, Niklas Söderlund
Re: [RFC PATCH 11/11] vidioc_cropcap -> vidioc_g_pixelaspect
Hi Hans,
Thanks for your patch.
On 2018-10-05 09:49:11 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Now vidioc_cropcap is only used to return the pixelaspect, so
> rename it accordingly.
>
> Signed-off-by: Hans Verkuil
For the v4l2 and rcar-vin changes:
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/pci/bt8xx/bttv-driver.c | 12 +--
> drivers/media/pci/cobalt/cobalt-v4l2.c| 10 +
> drivers/media/pci/cx18/cx18-ioctl.c | 13 ++--
> drivers/media/pci/cx23885/cx23885-video.c | 12 +--
> drivers/media/pci/ivtv/ivtv-ioctl.c | 17 ---
> drivers/media/pci/saa7134/saa7134-video.c | 21 +--
> drivers/media/platform/am437x/am437x-vpfe.c | 13 ++--
> drivers/media/platform/davinci/vpbe_display.c | 10 -
> drivers/media/platform/davinci/vpfe_capture.c | 12 +--
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 -
> drivers/media/platform/vivid/vivid-core.c | 9
> drivers/media/platform/vivid/vivid-vid-cap.c | 18 +++-
> drivers/media/platform/vivid/vivid-vid-cap.h | 2 +-
> drivers/media/platform/vivid/vivid-vid-out.c | 18 +++-
> drivers/media/platform/vivid/vivid-vid-out.h | 2 +-
> drivers/media/usb/au0828/au0828-video.c | 12 +--
> drivers/media/usb/cx231xx/cx231xx-417.c | 12 +--
> drivers/media/usb/cx231xx/cx231xx-video.c | 12 +--
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 13 +++-
> drivers/media/v4l2-core/v4l2-dev.c| 6 +++---
> drivers/media/v4l2-core/v4l2-ioctl.c | 15 +++--
> include/media/v4l2-ioctl.h| 8 +++
> 22 files changed, 131 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c
> b/drivers/media/pci/bt8xx/bttv-driver.c
> index b2cfcbb0008e..52cac1d3f577 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -2792,19 +2792,17 @@ static int bttv_g_tuner(struct file *file, void *priv,
> return 0;
> }
>
> -static int bttv_cropcap(struct file *file, void *priv,
> - struct v4l2_cropcap *cap)
> +static int bttv_g_pixelaspect(struct file *file, void *priv,
> + int type, struct v4l2_fract *f)
> {
> struct bttv_fh *fh = priv;
> struct bttv *btv = fh->btv;
>
> - if (cap->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> - cap->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
> + if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> return -EINVAL;
>
> /* defrect and bounds are set via g_selection */
> - cap->pixelaspect = bttv_tvnorms[btv->tvnorm].cropcap.pixelaspect;
> -
> + *f = bttv_tvnorms[btv->tvnorm].cropcap.pixelaspect;
> return 0;
> }
>
> @@ -3162,7 +3160,7 @@ static const struct v4l2_ioctl_ops bttv_ioctl_ops = {
> .vidioc_g_fmt_vbi_cap = bttv_g_fmt_vbi_cap,
> .vidioc_try_fmt_vbi_cap = bttv_try_fmt_vbi_cap,
> .vidioc_s_fmt_vbi_cap = bttv_s_fmt_vbi_cap,
> - .vidioc_cropcap = bttv_cropcap,
> + .vidioc_g_pixelaspect = bttv_g_pixelaspect,
> .vidioc_reqbufs = bttv_reqbufs,
> .vidioc_querybuf= bttv_querybuf,
> .vidioc_qbuf= bttv_qbuf,
> diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c
> b/drivers/media/pci/cobalt/cobalt-v4l2.c
> index 4a0205aae4b4..c088de551081 100644
> --- a/drivers/media/pci/cobalt/cobalt-v4l2.c
> +++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
> @@ -1077,20 +1077,22 @@ static int cobalt_g_parm(struct file *file, void *fh,
> struct v4l2_streamparm *a)
> return 0;
> }
>
> -static int cobalt_cropcap(struct file *file, void *fh, struct v4l2_cropcap
> *cc)
> +static int cobalt_g_pixelaspect(struct file *file, void *fh,
> + int type, struct v4l2_fract *f)
> {
> struct cobalt_stream *s = video_drvdata(file);
> struct v4l2_dv_timings timings;
> int err = 0;
>
> - if (cc->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> return -EINVAL;
> +
> if (s->input == 1)
> timings = cea1080p60;
> else
> err = v4l2_subdev_call(s->sd, video, g_dv_timings, );
> if (!err)
> - cc->pixelaspect = v4l2_dv_timings_aspect_ratio();
> + *f = v4l2_dv_timings_aspect_ratio();
> return err;
> }
>
> @@ -1132,7 +1134,7 @@ static const struct v4l2_ioctl_ops cobalt_ioctl_ops = {
> .vidioc_log_status = cobalt_log_status,
> .vidioc_streamon= vb2_ioctl_streamon,
> .vidioc_streamoff = vb2_ioctl_streamoff,
> - .vidioc_cropcap = cobalt_cropcap,
> + .vidioc_g_pixelaspect = cobalt_g_pixelaspect,
>
Re: [RFC PATCH 10/11] v4l2-ioctl: remove unused vidioc_g/s_crop
On 10/05/18 12:47, Niklas Söderlund wrote:
> Hi Hans,
>
> Thanks for your work.
>
> On 2018-10-05 09:49:10 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil
>>
>> Now that all drivers have dropped vidioc_g/s_crop we can remove
>> support for them in the V4L2 core.
>>
>> Signed-off-by: Hans Verkuil
>
> If the quirk patch hurt my head this makes me smile!
There you go, they cancel each other out in your head :-)
Regards,
Hans
>
> Reviewed-by: Niklas Söderlund
>
>> ---
>> drivers/media/v4l2-core/v4l2-dev.c | 4 ++--
>> drivers/media/v4l2-core/v4l2-ioctl.c | 4
>> include/media/v4l2-ioctl.h | 8
>> 3 files changed, 2 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
>> b/drivers/media/v4l2-core/v4l2-dev.c
>> index 69e775930fc4..d81141d51faa 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -621,9 +621,9 @@ static void determine_valid_ioctls(struct video_device
>> *vdev)
>> SET_VALID_IOCTL(ops, VIDIOC_TRY_DECODER_CMD,
>> vidioc_try_decoder_cmd);
>> SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMESIZES,
>> vidioc_enum_framesizes);
>> SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMEINTERVALS,
>> vidioc_enum_frameintervals);
>> -if (ops->vidioc_g_crop || ops->vidioc_g_selection)
>> +if (ops->vidioc_g_selection)
>> set_bit(_IOC_NR(VIDIOC_G_CROP), valid_ioctls);
>> -if (ops->vidioc_s_crop || ops->vidioc_s_selection)
>> +if (ops->vidioc_s_selection)
>> set_bit(_IOC_NR(VIDIOC_S_CROP), valid_ioctls);
>> SET_VALID_IOCTL(ops, VIDIOC_G_SELECTION, vidioc_g_selection);
>> SET_VALID_IOCTL(ops, VIDIOC_S_SELECTION, vidioc_s_selection);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 63a92285de39..a59954d351a2 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2207,8 +2207,6 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
>> };
>> int ret;
>>
>> -if (ops->vidioc_g_crop)
>> -return ops->vidioc_g_crop(file, fh, p);
>> /* simulate capture crop using selection api */
>>
>> /* crop means compose for output devices */
>> @@ -2239,8 +2237,6 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
>> .r = p->c,
>> };
>>
>> -if (ops->vidioc_s_crop)
>> -return ops->vidioc_s_crop(file, fh, p);
>> /* simulate capture crop using selection api */
>>
>> /* crop means compose for output devices */
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index 5848d92c30da..85fdd3f4b8ad 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -222,10 +222,6 @@ struct v4l2_fh;
>> * :ref:`VIDIOC_S_MODULATOR ` ioctl
>> * @vidioc_cropcap: pointer to the function that implements
>> * :ref:`VIDIOC_CROPCAP ` ioctl
>> - * @vidioc_g_crop: pointer to the function that implements
>> - * :ref:`VIDIOC_G_CROP ` ioctl
>> - * @vidioc_s_crop: pointer to the function that implements
>> - * :ref:`VIDIOC_S_CROP ` ioctl
>> * @vidioc_g_selection: pointer to the function that implements
>> * :ref:`VIDIOC_G_SELECTION ` ioctl
>> * @vidioc_s_selection: pointer to the function that implements
>> @@ -493,10 +489,6 @@ struct v4l2_ioctl_ops {
>> /* Crop ioctls */
>> int (*vidioc_cropcap)(struct file *file, void *fh,
>>struct v4l2_cropcap *a);
>> -int (*vidioc_g_crop)(struct file *file, void *fh,
>> - struct v4l2_crop *a);
>> -int (*vidioc_s_crop)(struct file *file, void *fh,
>> - const struct v4l2_crop *a);
>> int (*vidioc_g_selection)(struct file *file, void *fh,
>>struct v4l2_selection *s);
>> int (*vidioc_s_selection)(struct file *file, void *fh,
>> --
>> 2.18.0
>>
>
Re: [RFC PATCH 10/11] v4l2-ioctl: remove unused vidioc_g/s_crop
Hi Hans,
Thanks for your work.
On 2018-10-05 09:49:10 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Now that all drivers have dropped vidioc_g/s_crop we can remove
> support for them in the V4L2 core.
>
> Signed-off-by: Hans Verkuil
If the quirk patch hurt my head this makes me smile!
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/v4l2-core/v4l2-dev.c | 4 ++--
> drivers/media/v4l2-core/v4l2-ioctl.c | 4
> include/media/v4l2-ioctl.h | 8
> 3 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c
> index 69e775930fc4..d81141d51faa 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -621,9 +621,9 @@ static void determine_valid_ioctls(struct video_device
> *vdev)
> SET_VALID_IOCTL(ops, VIDIOC_TRY_DECODER_CMD,
> vidioc_try_decoder_cmd);
> SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMESIZES,
> vidioc_enum_framesizes);
> SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMEINTERVALS,
> vidioc_enum_frameintervals);
> - if (ops->vidioc_g_crop || ops->vidioc_g_selection)
> + if (ops->vidioc_g_selection)
> set_bit(_IOC_NR(VIDIOC_G_CROP), valid_ioctls);
> - if (ops->vidioc_s_crop || ops->vidioc_s_selection)
> + if (ops->vidioc_s_selection)
> set_bit(_IOC_NR(VIDIOC_S_CROP), valid_ioctls);
> SET_VALID_IOCTL(ops, VIDIOC_G_SELECTION, vidioc_g_selection);
> SET_VALID_IOCTL(ops, VIDIOC_S_SELECTION, vidioc_s_selection);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 63a92285de39..a59954d351a2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2207,8 +2207,6 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> };
> int ret;
>
> - if (ops->vidioc_g_crop)
> - return ops->vidioc_g_crop(file, fh, p);
> /* simulate capture crop using selection api */
>
> /* crop means compose for output devices */
> @@ -2239,8 +2237,6 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
> .r = p->c,
> };
>
> - if (ops->vidioc_s_crop)
> - return ops->vidioc_s_crop(file, fh, p);
> /* simulate capture crop using selection api */
>
> /* crop means compose for output devices */
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 5848d92c30da..85fdd3f4b8ad 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -222,10 +222,6 @@ struct v4l2_fh;
> * :ref:`VIDIOC_S_MODULATOR ` ioctl
> * @vidioc_cropcap: pointer to the function that implements
> * :ref:`VIDIOC_CROPCAP ` ioctl
> - * @vidioc_g_crop: pointer to the function that implements
> - * :ref:`VIDIOC_G_CROP ` ioctl
> - * @vidioc_s_crop: pointer to the function that implements
> - * :ref:`VIDIOC_S_CROP ` ioctl
> * @vidioc_g_selection: pointer to the function that implements
> * :ref:`VIDIOC_G_SELECTION ` ioctl
> * @vidioc_s_selection: pointer to the function that implements
> @@ -493,10 +489,6 @@ struct v4l2_ioctl_ops {
> /* Crop ioctls */
> int (*vidioc_cropcap)(struct file *file, void *fh,
> struct v4l2_cropcap *a);
> - int (*vidioc_g_crop)(struct file *file, void *fh,
> - struct v4l2_crop *a);
> - int (*vidioc_s_crop)(struct file *file, void *fh,
> - const struct v4l2_crop *a);
> int (*vidioc_g_selection)(struct file *file, void *fh,
> struct v4l2_selection *s);
> int (*vidioc_s_selection)(struct file *file, void *fh,
> --
> 2.18.0
>
--
Regards,
Niklas Söderlund
Re: [RFC PATCH 03/11] v4l2-ioctl: add QUIRK_INVERTED_CROP
Hi Hans,
Thanks for your patch.
On 2018-10-05 09:49:03 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Some old Samsung drivers use the legacy crop API incorrectly:
> the crop and compose targets are swapped. Normally VIDIOC_G_CROP
> will return the CROP rectangle of a CAPTURE stream and the COMPOSE
> rectangle of an OUTPUT stream.
>
> The Samsung drivers do the opposite. Note that these drivers predate
> the selection API.
>
> If this 'QUIRK' flag is set, then the v4l2-ioctl core will swap
> the CROP and COMPOSE targets as well.
>
> That way backwards compatibility is ensured and we can convert the
> Samsung drivers to the selection API.
>
> Signed-off-by: Hans Verkuil
I understand the need for this quirk but it make my head hurt :-)
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 17 -
> include/media/v4l2-dev.h | 13 +++--
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 9c2370e4d05c..63a92285de39 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2200,6 +2200,7 @@ static int v4l_s_selection(const struct v4l2_ioctl_ops
> *ops,
> static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> + struct video_device *vfd = video_devdata(file);
> struct v4l2_crop *p = arg;
> struct v4l2_selection s = {
> .type = p->type,
> @@ -2216,6 +2217,10 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> else
> s.target = V4L2_SEL_TGT_CROP;
>
> + if (test_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags))
> + s.target = s.target == V4L2_SEL_TGT_COMPOSE ?
> + V4L2_SEL_TGT_CROP : V4L2_SEL_TGT_COMPOSE;
> +
> ret = v4l_g_selection(ops, file, fh, );
>
> /* copying results to old structure on success */
> @@ -2227,6 +2232,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> + struct video_device *vfd = video_devdata(file);
> struct v4l2_crop *p = arg;
> struct v4l2_selection s = {
> .type = p->type,
> @@ -2243,12 +2249,17 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops
> *ops,
> else
> s.target = V4L2_SEL_TGT_CROP;
>
> + if (test_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags))
> + s.target = s.target == V4L2_SEL_TGT_COMPOSE ?
> + V4L2_SEL_TGT_CROP : V4L2_SEL_TGT_COMPOSE;
> +
> return v4l_s_selection(ops, file, fh, );
> }
>
> static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> + struct video_device *vfd = video_devdata(file);
> struct v4l2_cropcap *p = arg;
> struct v4l2_selection s = { .type = p->type };
> int ret = 0;
> @@ -2285,13 +2296,17 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops
> *ops,
> else
> s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>
> + if (test_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags))
> + s.target = s.target == V4L2_SEL_TGT_COMPOSE_BOUNDS ?
> + V4L2_SEL_TGT_CROP_BOUNDS : V4L2_SEL_TGT_COMPOSE_BOUNDS;
> +
> ret = v4l_g_selection(ops, file, fh, );
> if (ret)
> return ret;
> p->bounds = s.r;
>
> /* obtaining defrect */
> - if (V4L2_TYPE_IS_OUTPUT(p->type))
> + if (s.target == V4L2_SEL_TGT_COMPOSE_BOUNDS)
> s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
> else
> s.target = V4L2_SEL_TGT_CROP_DEFAULT;
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 456ac13eca1d..48531e57cc5a 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -74,10 +74,19 @@ struct v4l2_ctrl_handler;
> * indicates that file->private_data points to v4l2_fh.
> * This flag is set by the core when v4l2_fh_init() is called.
> * All new drivers should use it.
> + * @V4L2_FL_QUIRK_INVERTED_CROP:
> + * some old M2M drivers use g/s_crop/cropcap incorrectly: crop and
> + * compose are swapped. If this flag is set, then the selection
> + * targets are swapped in the g/s_crop/cropcap functions in v4l2-ioctl.c.
> + * This allows those drivers to correctly implement the selection API,
> + * but the old crop API will still work as expected in order to preserve
> + * backwards compatibility.
> + * Never set this flag for new drivers.
> */
> enum v4l2_video_device_flags {
> - V4L2_FL_REGISTERED = 0,
> - V4L2_FL_USES_V4L2_FH= 1,
> + V4L2_FL_REGISTERED = 0,
> + V4L2_FL_USES_V4L2_FH= 1,
> + V4L2_FL_QUIRK_INVERTED_CROP = 2,
> };
>
> /*
Re: [PATCH v2 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Hi Mauro, On Fri, Oct 05, 2018 at 06:29:36AM -0400, Mauro Carvalho Chehab wrote: > There are several coding style issues at those definitions, > and the previous patchset added even more. > > Address the trivial ones by first calling: > > ./scripts/checkpatch.pl --strict --fix-inline > include/media/v4l2-async.h include/media/v4l2-fwnode.h > include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c > drivers/media/v4l2-core/v4l2-fwnode.c > > and then manually adjusting the style where needed. > > Signed-off-by: Mauro Carvalho Chehab For patches 1 and 2: Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 2/3] media: v4l2-fwnode: cleanup functions that parse endpoints
Em Fri, 5 Oct 2018 13:08:25 +0300 Sakari Ailus escreveu: > > > This is still over 80 here. I think we could think of abbreviating what's > > > in the function name, not limiting to the endpoint. I think I'd prefer to > > > leave that for 4.21 as there's not much time anymore. > > > > Yes, I know. Renaming the function is the only way to get rid of > > those remaining warnings. If you're ok with renaming, IMHO it is best > > do do it right now, as we are already churning a lot of fwnode-related > > code, avoiding the need of touching it again for 4.21. > > This will presumably continue in v4.21 (or later). As noted in the cover > page of the fwnode patchset: > > This patchset does not address remaining issues such as supporting > setting defaults for e.g. bridge drivers with multiple ports, but > with Steve Longerbeam's patchset we're much closer to that goal. OK! Feel free to rename them when you feel ready. My suggestion is to do it at the end of a media merging cycle, as makes easier to avoid conflicts. I don't care that much about 80 cols. Yet, here it makes a point: we should be more spartan when naming functions :-) Thanks, Mauro
Re: [PATCH 3/3] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() call
Em Fri, 5 Oct 2018 13:06:04 +0300 Sakari Ailus escreveu: > > > > - unsigned int i; > > > > > > I'd like to keep this here. > > > > Why? IMHO, it makes harder to read (yet, if you insist, I'm ok with > > both ways). > > Generally loop, temporary, return etc. variables are nice to declare as > last. That is the practice in this file and generally in kernel code, > albeit with variable degree of application. I've seen more than one practice of ordering arguments at the Kernel :-) Anyway, I kept it there on the version I just sent. Thanks, Mauro
Re: [RFC PATCH 02/11] v4l2-common.h: put backwards compat defines under #ifndef __KERNEL__
Hi Hans,
Thanks for your patch.
On 2018-10-05 09:49:02 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> This ensures that they won't be used in kernel code.
>
> Signed-off-by: Hans Verkuil
Reviewed-by: Niklas Söderlund
> ---
> include/uapi/linux/v4l2-common.h | 28 +++-
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/include/uapi/linux/v4l2-common.h
> b/include/uapi/linux/v4l2-common.h
> index 4f7b892377cd..7d21c1634b4d 100644
> --- a/include/uapi/linux/v4l2-common.h
> +++ b/include/uapi/linux/v4l2-common.h
> @@ -79,24 +79,11 @@
> /* Current composing area plus all padding pixels */
> #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103
>
> -/* Backward compatibility target definitions --- to be removed. */
> -#define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
> -#define V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SEL_TGT_COMPOSE
> -#define V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL V4L2_SEL_TGT_CROP
> -#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL V4L2_SEL_TGT_COMPOSE
> -#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS V4L2_SEL_TGT_CROP_BOUNDS
> -#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS V4L2_SEL_TGT_COMPOSE_BOUNDS
> -
> /* Selection flags */
> #define V4L2_SEL_FLAG_GE (1 << 0)
> #define V4L2_SEL_FLAG_LE (1 << 1)
> #define V4L2_SEL_FLAG_KEEP_CONFIG(1 << 2)
>
> -/* Backward compatibility flag definitions --- to be removed. */
> -#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE V4L2_SEL_FLAG_GE
> -#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE V4L2_SEL_FLAG_LE
> -#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG V4L2_SEL_FLAG_KEEP_CONFIG
> -
> struct v4l2_edid {
> __u32 pad;
> __u32 start_block;
> @@ -105,4 +92,19 @@ struct v4l2_edid {
> __u8 *edid;
> };
>
> +#ifndef __KERNEL__
> +/* Backward compatibility target definitions --- to be removed. */
> +#define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
> +#define V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SEL_TGT_COMPOSE
> +#define V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL V4L2_SEL_TGT_CROP
> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL V4L2_SEL_TGT_COMPOSE
> +#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS V4L2_SEL_TGT_CROP_BOUNDS
> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS V4L2_SEL_TGT_COMPOSE_BOUNDS
> +
> +/* Backward compatibility flag definitions --- to be removed. */
> +#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE V4L2_SEL_FLAG_GE
> +#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE V4L2_SEL_FLAG_LE
> +#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG V4L2_SEL_FLAG_KEEP_CONFIG
> +#endif
> +
> #endif /* __V4L2_COMMON__ */
> --
> 2.18.0
>
--
Regards,
Niklas Söderlund
[PATCH v2 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
There are several coding style issues at those definitions,
and the previous patchset added even more.
Address the trivial ones by first calling:
./scripts/checkpatch.pl --strict --fix-inline
include/media/v4l2-async.h include/media/v4l2-fwnode.h
include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c
drivers/media/v4l2-core/v4l2-fwnode.c
and then manually adjusting the style where needed.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-async.c | 45 ---
drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++---
include/media/v4l2-async.h| 12 +-
include/media/v4l2-fwnode.h | 46 ---
include/media/v4l2-mediabus.h | 32 +++--
5 files changed, 179 insertions(+), 141 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c
b/drivers/media/v4l2-core/v4l2-async.c
index 70adbd9a01a2..a6d91370838d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -57,6 +57,7 @@ static bool match_i2c(struct v4l2_subdev *sd, struct
v4l2_async_subdev *asd)
{
#if IS_ENABLED(CONFIG_I2C)
struct i2c_client *client = i2c_verify_client(sd->dev);
+
return client &&
asd->match.i2c.adapter_id == client->adapter->nr &&
asd->match.i2c.address == client->addr;
@@ -89,10 +90,11 @@ static LIST_HEAD(subdev_list);
static LIST_HEAD(notifier_list);
static DEFINE_MUTEX(list_lock);
-static struct v4l2_async_subdev *v4l2_async_find_match(
- struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
+static struct v4l2_async_subdev *
+v4l2_async_find_match(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *sd)
{
- bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
+ bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
struct v4l2_async_subdev *asd;
list_for_each_entry(asd, >waiting, list) {
@@ -150,8 +152,8 @@ static bool asd_equal(struct v4l2_async_subdev *asd_x,
}
/* Find the sub-device notifier registered by a sub-device driver. */
-static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
- struct v4l2_subdev *sd)
+static struct v4l2_async_notifier *
+v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd)
{
struct v4l2_async_notifier *n;
@@ -163,8 +165,8 @@ static struct v4l2_async_notifier
*v4l2_async_find_subdev_notifier(
}
/* Get v4l2_device related to the notifier if one can be found. */
-static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
- struct v4l2_async_notifier *notifier)
+static struct v4l2_device *
+v4l2_async_notifier_find_v4l2_dev(struct v4l2_async_notifier *notifier)
{
while (notifier->parent)
notifier = notifier->parent;
@@ -175,8 +177,8 @@ static struct v4l2_device
*v4l2_async_notifier_find_v4l2_dev(
/*
* Return true if all child sub-device notifiers are complete, false otherwise.
*/
-static bool v4l2_async_notifier_can_complete(
- struct v4l2_async_notifier *notifier)
+static bool
+v4l2_async_notifier_can_complete(struct v4l2_async_notifier *notifier)
{
struct v4l2_subdev *sd;
@@ -199,8 +201,8 @@ static bool v4l2_async_notifier_can_complete(
* Complete the master notifier if possible. This is done when all async
* sub-devices have been bound; v4l2_device is also available then.
*/
-static int v4l2_async_notifier_try_complete(
- struct v4l2_async_notifier *notifier)
+static int
+v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
{
/* Quick check whether there are still more sub-devices here. */
if (!list_empty(>waiting))
@@ -221,8 +223,8 @@ static int v4l2_async_notifier_try_complete(
return v4l2_async_notifier_call_complete(notifier);
}
-static int v4l2_async_notifier_try_all_subdevs(
- struct v4l2_async_notifier *notifier);
+static int
+v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier);
static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
struct v4l2_device *v4l2_dev,
@@ -268,8 +270,8 @@ static int v4l2_async_match_notify(struct
v4l2_async_notifier *notifier,
}
/* Test all async sub-devices in a notifier for a match. */
-static int v4l2_async_notifier_try_all_subdevs(
- struct v4l2_async_notifier *notifier)
+static int
+v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier)
{
struct v4l2_device *v4l2_dev =
v4l2_async_notifier_find_v4l2_dev(notifier);
@@ -306,14 +308,17 @@ static int v4l2_async_notifier_try_all_subdevs(
static void v4l2_async_cleanup(struct v4l2_subdev *sd)
{
v4l2_device_unregister_subdev(sd);
- /* Subdevice driver will reprobe and put the subdev back onto the list
*/
+ /*
+* Subdevice driver will reprobe and put the subdev back
+* onto the list
+*/
[PATCH v2 0/3] Coding style cleanups after the fwnode patchset
The fwnode patchset added a several new warnings, as identified by checkpatch.pl --strict. Those are at the core stuff, and makes harder to review patches there. In order to fully address checkpatch.pl, we still need to: - Add SPDX headers; - Rename functions. Let's do those on some future change. - v2: - fixed a weird line break at patch 1/3; - kept the order of arguments inside a function at patch 3/3. Mauro Carvalho Chehab (3): media: v4l2-core: cleanup coding style at V4L2 async/fwnode media: v4l2-fwnode: cleanup functions that parse endpoints media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() call drivers/media/v4l2-core/v4l2-async.c | 45 +++--- drivers/media/v4l2-core/v4l2-fwnode.c | 188 ++ include/media/v4l2-async.h| 12 +- include/media/v4l2-fwnode.h | 45 +++--- include/media/v4l2-mediabus.h | 32 +++-- 5 files changed, 176 insertions(+), 146 deletions(-) -- 2.17.1
[PATCH v2 3/3] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() call
The v4l2_fwnode_reference_parse_int_props() has a big name, causing
it to cause coding style warnings. Also, it depends on a const
struct embedded indide a function.
Rearrange the logic in order to move the struct declaration out
of such function and use it inside this function.
That cleans up some coding style issues.
Acked-by: Sakari Ailus
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-fwnode.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
b/drivers/media/v4l2-core/v4l2-fwnode.c
index a7c2487154a4..218f0da0ce76 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -1006,6 +1006,12 @@ v4l2_fwnode_reference_get_int_prop(struct fwnode_handle
*fwnode,
return fwnode;
}
+struct v4l2_fwnode_int_props {
+ const char *name;
+ const char * const *props;
+ unsigned int nprops;
+};
+
/*
* v4l2_fwnode_reference_parse_int_props - parse references for async
*sub-devices
@@ -1032,13 +1038,14 @@ v4l2_fwnode_reference_get_int_prop(struct fwnode_handle
*fwnode,
static int
v4l2_fwnode_reference_parse_int_props(struct device *dev,
struct v4l2_async_notifier *notifier,
- const char *prop,
- const char * const *props,
- unsigned int nprops)
+ const struct v4l2_fwnode_int_props *p)
{
struct fwnode_handle *fwnode;
unsigned int index;
int ret;
+ const char *prop = p->name;
+ const char * const *props = p->props;
+ unsigned int nprops = p->nprops;
index = 0;
do {
@@ -1093,11 +1100,7 @@ int
v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
struct v4l2_async_notifier
*notifier)
{
static const char * const led_props[] = { "led" };
- static const struct {
- const char *name;
- const char * const *props;
- unsigned int nprops;
- } props[] = {
+ static const struct v4l2_fwnode_int_props props[] = {
{ "flash-leds", led_props, ARRAY_SIZE(led_props) },
{ "lens-focus", NULL, 0 },
};
@@ -1109,9 +1112,7 @@ int v4l2_async_notifier_parse_fwnode_sensor_common(struct
device *dev,
if (props[i].props && is_acpi_node(dev_fwnode(dev)))
ret = v4l2_fwnode_reference_parse_int_props(dev,
notifier,
-
props[i].name,
-
props[i].props,
-
props[i].nprops);
+ [i]);
else
ret = v4l2_fwnode_reference_parse(dev, notifier,
props[i].name);
--
2.17.1
[PATCH v2 2/3] media: v4l2-fwnode: cleanup functions that parse endpoints
There is already a typedef for the parse endpoint function.
However, instead of using it, it is redefined at the C file
(and on one of the function headers).
Replace them by the function typedef, in order to cleanup
several related coding style warnings.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-fwnode.c | 64 ---
include/media/v4l2-fwnode.h | 19
2 files changed, 37 insertions(+), 46 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
b/drivers/media/v4l2-core/v4l2-fwnode.c
index 4e518d5fddd8..a7c2487154a4 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -596,12 +596,10 @@ EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
static int
v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
- struct v4l2_async_notifier *notifier,
- struct fwnode_handle *endpoint,
- unsigned int asd_struct_size,
- int (*parse_endpoint)(struct device *dev,
- struct v4l2_fwnode_endpoint *vep,
- struct v4l2_async_subdev *asd))
+ struct v4l2_async_notifier *notifier,
+ struct fwnode_handle *endpoint,
+ unsigned int asd_struct_size,
+ parse_endpoint_func parse_endpoint)
{
struct v4l2_fwnode_endpoint vep = { .bus_type = 0 };
struct v4l2_async_subdev *asd;
@@ -657,13 +655,12 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device
*dev,
}
static int
-__v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
- struct v4l2_async_notifier *notifier,
- size_t asd_struct_size,
- unsigned int port, bool has_port,
- int (*parse_endpoint)(struct device *dev,
- struct v4l2_fwnode_endpoint *vep,
- struct v4l2_async_subdev *asd))
+__v4l2_async_notifier_parse_fwnode_ep(struct device *dev,
+ struct v4l2_async_notifier *notifier,
+ size_t asd_struct_size,
+ unsigned int port,
+ bool has_port,
+ parse_endpoint_func parse_endpoint)
{
struct fwnode_handle *fwnode;
int ret = 0;
@@ -708,31 +705,27 @@ __v4l2_async_notifier_parse_fwnode_endpoints(struct
device *dev,
int
v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
- struct v4l2_async_notifier *notifier,
- size_t asd_struct_size,
- int (*parse_endpoint)(struct device *dev,
- struct v4l2_fwnode_endpoint *vep,
- struct v4l2_async_subdev *asd))
+ struct v4l2_async_notifier *notifier,
+ size_t asd_struct_size,
+ parse_endpoint_func parse_endpoint)
{
- return __v4l2_async_notifier_parse_fwnode_endpoints(dev, notifier,
- asd_struct_size, 0,
- false,
- parse_endpoint);
+ return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
+asd_struct_size, 0,
+false, parse_endpoint);
}
EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
int
v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
- struct v4l2_async_notifier *notifier,
- size_t asd_struct_size, unsigned int port,
- int (*parse_endpoint)(struct device *dev,
- struct v4l2_fwnode_endpoint *vep,
- struct v4l2_async_subdev *asd))
+ struct v4l2_async_notifier
*notifier,
+ size_t asd_struct_size,
+ unsigned int port,
+ parse_endpoint_func
parse_endpoint)
{
- return __v4l2_async_notifier_parse_fwnode_endpoints(dev, notifier,
- asd_struct_size,
- port, true,
- parse_endpoint);
+ return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
+
Re: [PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Hi Mauro,
On Fri, Oct 05, 2018 at 07:12:02AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 5 Oct 2018 10:55:58 +0300
> Sakari Ailus escreveu:
...
> > > @@ -436,8 +437,7 @@ static int __v4l2_fwnode_endpoint_parse(struct
> > > fwnode_handle *fwnode,
> > > if (mbus_type != V4L2_MBUS_UNKNOWN &&
> > > vep->bus_type != mbus_type) {
> > > pr_debug("expecting bus type %s\n",
> > > - v4l2_fwnode_mbus_type_to_string(
> > > - vep->bus_type));
> > > +
> > > v4l2_fwnode_mbus_type_to_string(vep->bus_type));
> >
> > This one's over 80. I preferred what it was. But I have no strong
> > preference here.
> >
> > > return -ENXIO;
> > > }
> > > } else {
> > > @@ -452,8 +452,8 @@ static int __v4l2_fwnode_endpoint_parse(struct
> > > fwnode_handle *fwnode,
> > > return rval;
> > >
> > > if (vep->bus_type == V4L2_MBUS_UNKNOWN)
> > > - v4l2_fwnode_endpoint_parse_parallel_bus(
> > > - fwnode, vep, V4L2_MBUS_UNKNOWN);
> >
> > This is not uncommon way of aligning function arguments when short of
> > space. It is also not exceeding 80 characters, as the replacement below.
>
> Well, Lindent used to align like that. That's why we see it on lots of
> code inside media: in the past, people tend to use it to get rid of
> some checkpatch warnings. Lindent script has long gone (still people
> sometimes call indent), and now checkpatch evolved, and has a
> --fix-inplace, with is usually enough to pinpoint where to change
> (although it does a crap job for more multi-line function args).
>
> As a reviewer, this hurts my eyes. It took me more time to review
> something like
> v4l2_fwnode_endpoint_parse_parallel_bus(
> fwnode, vep, V4L2_MBUS_UNKNOWN);
>
> than something like:
> v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep,
>
> V4L2_MBUS_UNKNOWN);
I think this is somewhat a matter of taste and I prefer it different. :-)
>
> The parenthesis alignment really helps to identify that the second
> line are arguments.
>
> Btw, if you use checkpatch with --strict, you'll see that this is
> not the right Kernel coding style. It will complain for both ending a
> line with an open parenthesis and that the second line is not aligned.
Right; V4L2 has a lot of that pattern (also elsewhere) but you'd get told
to fix that if it were in another tree (non-media). I think we agree on
renaming the very long function names; it'll get rid of probably much of
that pattern. I'll submit patches for that later on, possibly including
other improvements to the API. But that'll be after 4.20.
>
> >
> > > + v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep,
> > > +
> > > V4L2_MBUS_UNKNOWN);
> > >
> > > pr_debug("assuming media bus type %s (%u)\n",
> > >v4l2_fwnode_mbus_type_to_string(vep->bus_type),
> > > @@ -511,8 +511,8 @@ void v4l2_fwnode_endpoint_free(struct
> > > v4l2_fwnode_endpoint *vep)
> > > }
> > > EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_free);
> > >
> > > -int v4l2_fwnode_endpoint_alloc_parse(
> > > - struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep)
> > > +int v4l2_fwnode_endpoint_alloc_parse(struct fwnode_handle *fwnode,
> > > + struct v4l2_fwnode_endpoint *vep)
> > > {
> > > int rval;
> > >
> > > @@ -533,9 +533,10 @@ int v4l2_fwnode_endpoint_alloc_parse(
> > >
> > > vep->nr_of_link_frequencies = rval;
> > >
> > > - rval = fwnode_property_read_u64_array(
> > > - fwnode, "link-frequencies", vep->link_frequencies,
> > > - vep->nr_of_link_frequencies);
> > > + rval = fwnode_property_read_u64_array(fwnode,
> > > + "link-frequencies",
> > > + vep->link_frequencies,
> > > +
> > > vep->nr_of_link_frequencies);
> >
> > Over 80 characters.
>
> True, but it is better to violate 80-cols (those days, I guess everybody
> uses a graphical environment), than to not align the arguments.
>
> The 80-cols is there nowadays mostly to warn about code complexity, where
> multiple indentations are present.
I also review the patches using Mutt and my terminal window width is set at
80 characters. That's not uncommon I believe.
>
> For a reviewer, the parenthesis alignment is a way more relevant, as
> it allows to immediately notice that the two following lines are
> arguments of the function, and not a new indentation level.
That's a valid point, yet more important is that it's not at the same level
than the first line of the statement
Re: [PATCH] MAINTAINERS: update videobuf2 entry
Hi Marek,
On Fri, Oct 05, 2018 at 10:01:41AM +0200, Marek Szyprowski wrote:
> Commits 03fbdb2fc2b8 ("media: move videobuf2 to drivers/media/common")
> and 7952be9b6ece ("media: drivers/media/common/videobuf2: rename from
> videobuf") moved videobuf2 framework source code finally to
> drivers/media/common/videobuf2 directory, so update relevant paths in
> MAINTAINERS file.
>
> Reported-by: Joe Perches
> Signed-off-by: Marek Szyprowski
Thanks!
Acked-by: Sakari Ailus
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [RFC PATCH 01/11] v4l2-ioctl: don't use CROP/COMPOSE_ACTIVE
Hi Hans, Thanks for your work. On 2018-10-05 09:49:01 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Drop the deprecated _ACTIVE part. > > Signed-off-by: Hans Verkuil Reviewed-by: Niklas Söderlund > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c > b/drivers/media/v4l2-core/v4l2-ioctl.c > index 7de041bae84f..9c2370e4d05c 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -2212,9 +2212,9 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops, > > /* crop means compose for output devices */ > if (V4L2_TYPE_IS_OUTPUT(p->type)) > - s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE; > + s.target = V4L2_SEL_TGT_COMPOSE; > else > - s.target = V4L2_SEL_TGT_CROP_ACTIVE; > + s.target = V4L2_SEL_TGT_CROP; > > ret = v4l_g_selection(ops, file, fh, ); > > @@ -2239,9 +2239,9 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops, > > /* crop means compose for output devices */ > if (V4L2_TYPE_IS_OUTPUT(p->type)) > - s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE; > + s.target = V4L2_SEL_TGT_COMPOSE; > else > - s.target = V4L2_SEL_TGT_CROP_ACTIVE; > + s.target = V4L2_SEL_TGT_CROP; > > return v4l_s_selection(ops, file, fh, ); > } > -- > 2.18.0 > -- Regards, Niklas Söderlund
Re: [PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Em Fri, 5 Oct 2018 10:55:58 +0300
Sakari Ailus escreveu:
> Hi Mauro,
>
> On Thu, Oct 04, 2018 at 06:13:46PM -0400, Mauro Carvalho Chehab wrote:
> > There are several coding style issues at those definitions,
> > and the previous patchset added even more.
> >
> > Address the trivial ones by first calling:
> >
> > ./scripts/checkpatch.pl --strict --fix-inline
> > include/media/v4l2-async.h include/media/v4l2-fwnode.h
> > include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c
> > drivers/media/v4l2-core/v4l2-fwnode.c
> >
> > and then manually adjusting the style where needed.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 45 ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++---
> > include/media/v4l2-async.h| 12 +-
> > include/media/v4l2-fwnode.h | 46 ---
> > include/media/v4l2-mediabus.h | 32 +++--
> > 5 files changed, 179 insertions(+), 141 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 70adbd9a01a2..6fdda745a1da 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -57,6 +57,7 @@ static bool match_i2c(struct v4l2_subdev *sd, struct
> > v4l2_async_subdev *asd)
> > {
> > #if IS_ENABLED(CONFIG_I2C)
> > struct i2c_client *client = i2c_verify_client(sd->dev);
> > +
> > return client &&
> > asd->match.i2c.adapter_id == client->adapter->nr &&
> > asd->match.i2c.address == client->addr;
> > @@ -89,10 +90,11 @@ static LIST_HEAD(subdev_list);
> > static LIST_HEAD(notifier_list);
> > static DEFINE_MUTEX(list_lock);
> >
> > -static struct v4l2_async_subdev *v4l2_async_find_match(
> > - struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> > +static struct
> > +v4l2_async_subdev *v4l2_async_find_match(struct v4l2_async_notifier
> > *notifier,
>
> This looks odd. If you want to rewrap it, then I'd put the stuff before and
> including the first asterisk on the first line.
Yeah, makes sense.
>
> > +struct v4l2_subdev *sd)
> > {
> > - bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
> > + bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
> > struct v4l2_async_subdev *asd;
> >
> > list_for_each_entry(asd, >waiting, list) {
> > @@ -150,8 +152,8 @@ static bool asd_equal(struct v4l2_async_subdev *asd_x,
> > }
> >
> > /* Find the sub-device notifier registered by a sub-device driver. */
> > -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> > - struct v4l2_subdev *sd)
> > +static struct v4l2_async_notifier *
> > +v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd)
>
> It's better here. Below, too.
>
> > {
> > struct v4l2_async_notifier *n;
> >
> > @@ -163,8 +165,8 @@ static struct v4l2_async_notifier
> > *v4l2_async_find_subdev_notifier(
> > }
> >
> > /* Get v4l2_device related to the notifier if one can be found. */
> > -static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> > - struct v4l2_async_notifier *notifier)
> > +static struct v4l2_device *
> > +v4l2_async_notifier_find_v4l2_dev(struct v4l2_async_notifier *notifier)
> > {
> > while (notifier->parent)
> > notifier = notifier->parent;
> > @@ -175,8 +177,8 @@ static struct v4l2_device
> > *v4l2_async_notifier_find_v4l2_dev(
> > /*
> > * Return true if all child sub-device notifiers are complete, false
> > otherwise.
> > */
> > -static bool v4l2_async_notifier_can_complete(
> > - struct v4l2_async_notifier *notifier)
> > +static bool
> > +v4l2_async_notifier_can_complete(struct v4l2_async_notifier *notifier)
> > {
> > struct v4l2_subdev *sd;
> >
> > @@ -199,8 +201,8 @@ static bool v4l2_async_notifier_can_complete(
> > * Complete the master notifier if possible. This is done when all async
> > * sub-devices have been bound; v4l2_device is also available then.
> > */
> > -static int v4l2_async_notifier_try_complete(
> > - struct v4l2_async_notifier *notifier)
> > +static int
> > +v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
> > {
> > /* Quick check whether there are still more sub-devices here. */
> > if (!list_empty(>waiting))
> > @@ -221,8 +223,8 @@ static int v4l2_async_notifier_try_complete(
> > return v4l2_async_notifier_call_complete(notifier);
> > }
> >
> > -static int v4l2_async_notifier_try_all_subdevs(
> > - struct v4l2_async_notifier *notifier);
> > +static int
> > +v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier);
> >
> > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> >struct v4l2_device *v4l2_dev,
> > @@ -268,8 +270,8 @@ static int v4l2_async_match_notify(struct
> > v4l2_async_notifier *notifier,
> > }
> >
> >
Re: [PATCH 2/3] media: v4l2-fwnode: cleanup functions that parse endpoints
On Fri, Oct 05, 2018 at 06:52:20AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 5 Oct 2018 11:01:18 +0300
> Sakari Ailus escreveu:
>
> > Hi Mauro,
> >
> > Feel free to ignore the comments on the first patch regarding the functions
> > below. There are other issues there though.
> >
> > On Thu, Oct 04, 2018 at 06:13:47PM -0400, Mauro Carvalho Chehab wrote:
> > > There is already a typedef for the parse endpoint function.
> > > However, instead of using it, it is redefined at the C file
> > > (and on one of the function headers).
> > >
> > > Replace them by the function typedef, in order to cleanup
> > > several related coding style warnings.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 64 ---
> > > include/media/v4l2-fwnode.h | 19
> > > 2 files changed, 37 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index 4e518d5fddd8..a7c2487154a4 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -596,12 +596,10 @@ EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > >
> > > static int
> > > v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> > > - struct v4l2_async_notifier *notifier,
> > > - struct fwnode_handle *endpoint,
> > > - unsigned int asd_struct_size,
> > > - int (*parse_endpoint)(struct device *dev,
> > > - struct v4l2_fwnode_endpoint *vep,
> > > - struct v4l2_async_subdev *asd))
> > > + struct v4l2_async_notifier *notifier,
> > > + struct fwnode_handle *endpoint,
> > > + unsigned int asd_struct_size,
> > > + parse_endpoint_func parse_endpoint)
> > > {
> > > struct v4l2_fwnode_endpoint vep = { .bus_type = 0 };
> > > struct v4l2_async_subdev *asd;
> > > @@ -657,13 +655,12 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct
> > > device *dev,
> > > }
> > >
> > > static int
> > > -__v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
> > > - struct v4l2_async_notifier *notifier,
> > > - size_t asd_struct_size,
> > > - unsigned int port, bool has_port,
> > > - int (*parse_endpoint)(struct device *dev,
> > > - struct v4l2_fwnode_endpoint *vep,
> > > - struct v4l2_async_subdev *asd))
> > > +__v4l2_async_notifier_parse_fwnode_ep(struct device *dev,
> > > + struct v4l2_async_notifier *notifier,
> > > + size_t asd_struct_size,
> > > + unsigned int port,
> > > + bool has_port,
> > > + parse_endpoint_func parse_endpoint)
> > > {
> > > struct fwnode_handle *fwnode;
> > > int ret = 0;
> > > @@ -708,31 +705,27 @@ __v4l2_async_notifier_parse_fwnode_endpoints(struct
> > > device *dev,
> > >
> > > int
> > > v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
> > > - struct v4l2_async_notifier *notifier,
> > > - size_t asd_struct_size,
> > > - int (*parse_endpoint)(struct device *dev,
> > > - struct v4l2_fwnode_endpoint *vep,
> > > - struct v4l2_async_subdev *asd))
> > > +struct v4l2_async_notifier *notifier,
> > > +size_t asd_struct_size,
> > > +parse_endpoint_func parse_endpoint)
> > > {
> > > - return __v4l2_async_notifier_parse_fwnode_endpoints(dev, notifier,
> > > - asd_struct_size, 0,
> > > - false,
> > > - parse_endpoint);
> > > + return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
> > > + asd_struct_size, 0,
> > > + false, parse_endpoint);
> > > }
> > > EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> > >
> > > int
> > > v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
> > > - struct v4l2_async_notifier *notifier,
> > > - size_t asd_struct_size, unsigned int port,
> > > - int (*parse_endpoint)(struct device *dev,
> > > - struct v4l2_fwnode_endpoint *vep,
> > > - struct v4l2_async_subdev *asd))
> > > +struct v4l2_async_notifier
> > >
Re: [PATCH 3/3] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() call
On Fri, Oct 05, 2018 at 06:54:49AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 5 Oct 2018 11:03:10 +0300
> Sakari Ailus escreveu:
>
> > Hi Mauro,
> >
> > On Thu, Oct 04, 2018 at 06:13:48PM -0400, Mauro Carvalho Chehab wrote:
> > > The v4l2_fwnode_reference_parse_int_props() has a big name, causing
> > > it to cause coding style warnings. Also, it depends on a const
> > > struct embedded indide a function.
> > >
> > > Rearrange the logic in order to move the struct declaration out
> > > of such function and use it inside this function.
> > >
> > > That cleans up some coding style issues.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 25 +
> > > 1 file changed, 13 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index a7c2487154a4..e0cd119d6f5c 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -1006,6 +1006,12 @@ v4l2_fwnode_reference_get_int_prop(struct
> > > fwnode_handle *fwnode,
> > > return fwnode;
> > > }
> > >
> > > +struct v4l2_fwnode_int_props {
> > > + const char *name;
> > > + const char * const *props;
> > > + unsigned int nprops;
> > > +};
> > > +
> > > /*
> > > * v4l2_fwnode_reference_parse_int_props - parse references for async
> > > * sub-devices
> > > @@ -1032,13 +1038,14 @@ v4l2_fwnode_reference_get_int_prop(struct
> > > fwnode_handle *fwnode,
> > > static int
> > > v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > > struct v4l2_async_notifier *notifier,
> > > - const char *prop,
> > > - const char * const *props,
> > > - unsigned int nprops)
> > > + const struct v4l2_fwnode_int_props *p)
> > > {
> > > struct fwnode_handle *fwnode;
> > > unsigned int index;
> > > int ret;
> > > + const char *prop = p->name;
> > > + const char * const *props = p->props;
> > > + unsigned int nprops = p->nprops;
> > >
> > > index = 0;
> > > do {
> > > @@ -1092,16 +1099,12 @@ v4l2_fwnode_reference_parse_int_props(struct
> > > device *dev,
> > > int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > > struct v4l2_async_notifier
> > > *notifier)
> > > {
> > > + unsigned int i;
> > > static const char * const led_props[] = { "led" };
> > > - static const struct {
> > > - const char *name;
> > > - const char * const *props;
> > > - unsigned int nprops;
> > > - } props[] = {
> > > + static const struct v4l2_fwnode_int_props props[] = {
> > > { "flash-leds", led_props, ARRAY_SIZE(led_props) },
> > > { "lens-focus", NULL, 0 },
> > > };
> > > - unsigned int i;
> >
> > I'd like to keep this here.
>
> Why? IMHO, it makes harder to read (yet, if you insist, I'm ok with
> both ways).
Generally loop, temporary, return etc. variables are nice to declare as
last. That is the practice in this file and generally in kernel code,
albeit with variable degree of application.
See e.g. drivers/media/v4l2-core/v4l2-ctrls.c .
>
> > Apart from that,
> >
> > Acked-by: Sakari Ailus
> >
> > >
> > > for (i = 0; i < ARRAY_SIZE(props); i++) {
> > > int ret;
> > > @@ -1109,9 +1112,7 @@ int
> > > v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > > if (props[i].props && is_acpi_node(dev_fwnode(dev)))
> > > ret = v4l2_fwnode_reference_parse_int_props(dev,
> > > notifier,
> > > -
> > > props[i].name,
> > > -
> > > props[i].props,
> > > -
> > > props[i].nprops);
> > > + [i]);
> > > else
> > > ret = v4l2_fwnode_reference_parse(dev, notifier,
> > > props[i].name);
> >
>
>
>
> Thanks,
> Mauro
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH 3/3] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() call
Em Fri, 5 Oct 2018 11:03:10 +0300
Sakari Ailus escreveu:
> Hi Mauro,
>
> On Thu, Oct 04, 2018 at 06:13:48PM -0400, Mauro Carvalho Chehab wrote:
> > The v4l2_fwnode_reference_parse_int_props() has a big name, causing
> > it to cause coding style warnings. Also, it depends on a const
> > struct embedded indide a function.
> >
> > Rearrange the logic in order to move the struct declaration out
> > of such function and use it inside this function.
> >
> > That cleans up some coding style issues.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 25 +
> > 1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index a7c2487154a4..e0cd119d6f5c 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -1006,6 +1006,12 @@ v4l2_fwnode_reference_get_int_prop(struct
> > fwnode_handle *fwnode,
> > return fwnode;
> > }
> >
> > +struct v4l2_fwnode_int_props {
> > + const char *name;
> > + const char * const *props;
> > + unsigned int nprops;
> > +};
> > +
> > /*
> > * v4l2_fwnode_reference_parse_int_props - parse references for async
> > *sub-devices
> > @@ -1032,13 +1038,14 @@ v4l2_fwnode_reference_get_int_prop(struct
> > fwnode_handle *fwnode,
> > static int
> > v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > struct v4l2_async_notifier *notifier,
> > - const char *prop,
> > - const char * const *props,
> > - unsigned int nprops)
> > + const struct v4l2_fwnode_int_props *p)
> > {
> > struct fwnode_handle *fwnode;
> > unsigned int index;
> > int ret;
> > + const char *prop = p->name;
> > + const char * const *props = p->props;
> > + unsigned int nprops = p->nprops;
> >
> > index = 0;
> > do {
> > @@ -1092,16 +1099,12 @@ v4l2_fwnode_reference_parse_int_props(struct device
> > *dev,
> > int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> >struct v4l2_async_notifier
> > *notifier)
> > {
> > + unsigned int i;
> > static const char * const led_props[] = { "led" };
> > - static const struct {
> > - const char *name;
> > - const char * const *props;
> > - unsigned int nprops;
> > - } props[] = {
> > + static const struct v4l2_fwnode_int_props props[] = {
> > { "flash-leds", led_props, ARRAY_SIZE(led_props) },
> > { "lens-focus", NULL, 0 },
> > };
> > - unsigned int i;
>
> I'd like to keep this here.
Why? IMHO, it makes harder to read (yet, if you insist, I'm ok with
both ways).
> Apart from that,
>
> Acked-by: Sakari Ailus
>
> >
> > for (i = 0; i < ARRAY_SIZE(props); i++) {
> > int ret;
> > @@ -1109,9 +1112,7 @@ int
> > v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > if (props[i].props && is_acpi_node(dev_fwnode(dev)))
> > ret = v4l2_fwnode_reference_parse_int_props(dev,
> > notifier,
> > -
> > props[i].name,
> > -
> > props[i].props,
> > -
> > props[i].nprops);
> > + [i]);
> > else
> > ret = v4l2_fwnode_reference_parse(dev, notifier,
> > props[i].name);
>
Thanks,
Mauro
Re: [PATCH 2/3] media: v4l2-fwnode: cleanup functions that parse endpoints
Em Fri, 5 Oct 2018 11:01:18 +0300
Sakari Ailus escreveu:
> Hi Mauro,
>
> Feel free to ignore the comments on the first patch regarding the functions
> below. There are other issues there though.
>
> On Thu, Oct 04, 2018 at 06:13:47PM -0400, Mauro Carvalho Chehab wrote:
> > There is already a typedef for the parse endpoint function.
> > However, instead of using it, it is redefined at the C file
> > (and on one of the function headers).
> >
> > Replace them by the function typedef, in order to cleanup
> > several related coding style warnings.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 64 ---
> > include/media/v4l2-fwnode.h | 19
> > 2 files changed, 37 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 4e518d5fddd8..a7c2487154a4 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -596,12 +596,10 @@ EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > static int
> > v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> > - struct v4l2_async_notifier *notifier,
> > - struct fwnode_handle *endpoint,
> > - unsigned int asd_struct_size,
> > - int (*parse_endpoint)(struct device *dev,
> > - struct v4l2_fwnode_endpoint *vep,
> > - struct v4l2_async_subdev *asd))
> > + struct v4l2_async_notifier *notifier,
> > + struct fwnode_handle *endpoint,
> > + unsigned int asd_struct_size,
> > + parse_endpoint_func parse_endpoint)
> > {
> > struct v4l2_fwnode_endpoint vep = { .bus_type = 0 };
> > struct v4l2_async_subdev *asd;
> > @@ -657,13 +655,12 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct
> > device *dev,
> > }
> >
> > static int
> > -__v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
> > - struct v4l2_async_notifier *notifier,
> > - size_t asd_struct_size,
> > - unsigned int port, bool has_port,
> > - int (*parse_endpoint)(struct device *dev,
> > - struct v4l2_fwnode_endpoint *vep,
> > - struct v4l2_async_subdev *asd))
> > +__v4l2_async_notifier_parse_fwnode_ep(struct device *dev,
> > + struct v4l2_async_notifier *notifier,
> > + size_t asd_struct_size,
> > + unsigned int port,
> > + bool has_port,
> > + parse_endpoint_func parse_endpoint)
> > {
> > struct fwnode_handle *fwnode;
> > int ret = 0;
> > @@ -708,31 +705,27 @@ __v4l2_async_notifier_parse_fwnode_endpoints(struct
> > device *dev,
> >
> > int
> > v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
> > - struct v4l2_async_notifier *notifier,
> > - size_t asd_struct_size,
> > - int (*parse_endpoint)(struct device *dev,
> > - struct v4l2_fwnode_endpoint *vep,
> > - struct v4l2_async_subdev *asd))
> > + struct v4l2_async_notifier *notifier,
> > + size_t asd_struct_size,
> > + parse_endpoint_func parse_endpoint)
> > {
> > - return __v4l2_async_notifier_parse_fwnode_endpoints(dev, notifier,
> > - asd_struct_size, 0,
> > - false,
> > - parse_endpoint);
> > + return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
> > +asd_struct_size, 0,
> > +false, parse_endpoint);
> > }
> > EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> >
> > int
> > v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
> > - struct v4l2_async_notifier *notifier,
> > - size_t asd_struct_size, unsigned int port,
> > - int (*parse_endpoint)(struct device *dev,
> > - struct v4l2_fwnode_endpoint *vep,
> > - struct v4l2_async_subdev *asd))
> > + struct v4l2_async_notifier
> > *notifier,
> > + size_t asd_struct_size,
> > + unsigned int port,
> > +
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
Hi Hans, On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote: > On 09/23/2018 06:34 PM, Akinobu Mita wrote: > > The video_i2c_data is allocated by kzalloc and released by the video > > device's release callback. The release callback is called when > > video_unregister_device() is called, but it will still be accessed after > > calling video_unregister_device(). > > > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with > > i2c_client->dev so that it will automatically be released when the i2c > > driver is removed. > > Hmm. The patch is right, but the explanation isn't. The core problem is > that vdev.release was set to video_i2c_release, but that should only be > used if struct video_device was kzalloc'ed. But in this case it is embedded > in a larger struct, and then vdev.release should always be set to > video_device_release_empty. When the driver is unbound, what's acquired using the devm_() family of functions is released. At the same time, the user still holds a file handle, and can issue IOCTLs --- but the device's data structures no longer exist. That's not ok, and also the reason why we have the release callback. While there are issues elsewhere, this bit of the V4L2 / MC frameworks is fine. Or am I missing something? -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] media: cec: name for RC passthrough device does not need 'RC for'
On Fri, Oct 05, 2018 at 09:37:11AM +0200, Hans Verkuil wrote: > On 10/05/2018 12:21 AM, Sean Young wrote: > > An RC device is does not need to be called 'RC for'. Simply the name > > will suffice. > > > > Signed-off-by: Sean Young > > Reviewed-by: Hans Verkuil > > OK if I take this patch? I have a cec pull request upcoming anyway. Please do, thank you. Sean
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
On 10/02/18 18:17, Akinobu Mita wrote: > 2018年10月1日(月) 18:40 Hans Verkuil : >> >> On 09/23/2018 06:34 PM, Akinobu Mita wrote: >>> The video_i2c_data is allocated by kzalloc and released by the video >>> device's release callback. The release callback is called when >>> video_unregister_device() is called, but it will still be accessed after >>> calling video_unregister_device(). >>> >>> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with >>> i2c_client->dev so that it will automatically be released when the i2c >>> driver is removed. >> >> Hmm. The patch is right, but the explanation isn't. The core problem is >> that vdev.release was set to video_i2c_release, but that should only be >> used if struct video_device was kzalloc'ed. But in this case it is embedded >> in a larger struct, and then vdev.release should always be set to >> video_device_release_empty. >> >> That was the real reason for the invalid access. > > How about the commit log below? > > struct video_device for video-i2c is embedded in a struct video_i2c_data, > and then vdev.release should always be set to video_device_release_empty. > > However, the vdev.release is currently set to video_i2c_release which > frees the whole struct video_i2c_data. In video_i2c_remove(), some fields > (v4l2_dev, lock, and queue_lock) in struct video_i2c_data are still > accessed after video_unregister_device(). > > This fixes the use after free by setting the vdev.release to > video_device_release_empty. Also, convert to use devm_kzalloc() for > allocating a struct video_i2c_data in order to simplify the code. > Looks good. Regards, Hans
Re: s5p_mfc and H.264 frame cropping question
On Fri, Oct 5, 2018 at 5:38 PM Hans Verkuil wrote: > > On 10/05/2018 10:16 AM, Tomasz Figa wrote: > > On Fri, Oct 5, 2018 at 3:58 PM Hans Verkuil wrote: > >> > >> On 10/05/2018 05:12 AM, Tomasz Figa wrote: > >>> Hi Hans, > >>> > >>> On Fri, Oct 5, 2018 at 5:02 AM Hans Verkuil wrote: > > Hi all, > > I'm looking at removing the last users of vidioc_g/s_crop from the > driver and > I came across vidioc_g_crop in > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c. > > What this really does AFAICS is return the H.264 frame crop as read from > the > bitstream. This has nothing to do with regular cropping/composing but it > might be > something that could be implemented as a new selection target. > >>> > >>> It has a lot to do, because the output frame buffer may contain (and > >>> on the hardware I worked with, s5p-mfc and mtk-vcodec, indeed does) > >>> the whole encoded stream and the frame crop from the bitstream > >>> specifies the rectangle within it that corresponds to the part that > >>> should be displayed. > >> > >> Yes, but is that part actually cropped? Or is the full uncropped image > >> DMAed > >> to the capture buffer? > > > > The latter. > > > >> > >> To take a practical example: a H.264 stream with a 1920x1088 image and a > >> frame > >> crop rectangle of 1920x1080. What is the G_FMT width/height for the decoder > >> capture stream: 1920x1088 or 1920x1080? > >> > >> If it is 1920x1088, then you have a compose rectangle. If it is 1920x1080 > >> then > >> you have a crop rectangle. > > > > 1920x1088 > > > >> > >> As far as I can tell from this driver it actually has a compose rectangle > >> and the use of g_crop is wrong and is there due to historical reasons (the > >> driver predates the selection API). > > > > Yes, it is there due to historical reasons. > > > >> > >>> > > I'm not really sure what to do with the existing code since it is an > abuse of > the crop API, but I guess the first step is to decide how this should be > handled > properly. > > Are there other decoders that can retrieve this information? Should this > be > mentioned in the stateful codec API? > >>> > >>> coda [1], mtk-vcodec [2] and venus [3] expose this using the > >>> V4L2_SEL_TGT_COMPOSE selection target. v1 of the specification defines > >>> the selection targets in a way, which is compatible with that: > >>> V4L2_SEL_TGT_COMPOSE defaults to V4L2_SEL_TGT_COMPOSE_DEFAULT, which > >>> equals to V4L2_SEL_TGT_CROP, which defaults to > >>> V4L2_SEL_TGT_CROP_DEFAULT, which is defined as follows: > >>> > >>> + ``V4L2_SEL_TGT_CROP_DEFAULT`` > >>> + a rectangle covering the part of the frame buffer that contains > >>> + meaningful picture data (visible area); width and height will > >>> be > >>> + equal to visible resolution of the stream > >> > >> Where do you get that from? That's the crop definition for an output > >> stream, > >> not a capture stream (assuming we have a codec). > > > > We're talking about a decoder here, right? > > > > In this case OUTPUT stream is just a sequence of bytes, not video > > frames, so there is no selection defined for OUTPUT queue. > > > > CAPTURE stream should be seen as a video grabber, so CROP targets > > relate to the encoded rectangle (1920x1088) and COMPOSE targets to the > > CAPTURE buffer. V4L2_SEL_TGT_COMPOSE would be the part of the CAPTURE > > buffer that is written with the image selected by V4L2_SEL_TGT_CROP. > > > > On a decoder that cannot do arbitrary crop and compose, like s5p-mfc, > > both targets would have identical rectangles, equal to the visible > > region (1920x1080). On hardware which can actually do fancier things, > > userspace could freely configure CAPTURE buffer width and height and > > V4L2_SEL_TGT_COMPOSE rectangle, for example to downscale the decoded > > video on the fly. > > > > Please check how I specified all the targets in last version of the > > specification (https://lore.kernel.org/patchwork/patch/966933/) and > > comment there, if there is anything that goes against the > > specification of the selection API. > > I think we all mean the same thing, but just got confused :-) > > > > >> > >> I kind of lost you with "which equals to V4L2_SEL_TGT_CROP". > >> > >> In any case, this particular driver should implement g_selection for > >> CAPTURE and implement the COMPOSE targets. That makes sense. > > Right. > > Please check my RFC series I just posted that hopefully fixes this. > > Specifically https://patchwork.linuxtv.org/patch/52393/ and > https://patchwork.linuxtv.org/patch/52388/ Yeah, I've noticed it, thanks! Added to my list. Best regards, Tomasz
Re: s5p_mfc and H.264 frame cropping question
On 10/05/2018 10:16 AM, Tomasz Figa wrote: > On Fri, Oct 5, 2018 at 3:58 PM Hans Verkuil wrote: >> >> On 10/05/2018 05:12 AM, Tomasz Figa wrote: >>> Hi Hans, >>> >>> On Fri, Oct 5, 2018 at 5:02 AM Hans Verkuil wrote: Hi all, I'm looking at removing the last users of vidioc_g/s_crop from the driver and I came across vidioc_g_crop in drivers/media/platform/s5p-mfc/s5p_mfc_dec.c. What this really does AFAICS is return the H.264 frame crop as read from the bitstream. This has nothing to do with regular cropping/composing but it might be something that could be implemented as a new selection target. >>> >>> It has a lot to do, because the output frame buffer may contain (and >>> on the hardware I worked with, s5p-mfc and mtk-vcodec, indeed does) >>> the whole encoded stream and the frame crop from the bitstream >>> specifies the rectangle within it that corresponds to the part that >>> should be displayed. >> >> Yes, but is that part actually cropped? Or is the full uncropped image DMAed >> to the capture buffer? > > The latter. > >> >> To take a practical example: a H.264 stream with a 1920x1088 image and a >> frame >> crop rectangle of 1920x1080. What is the G_FMT width/height for the decoder >> capture stream: 1920x1088 or 1920x1080? >> >> If it is 1920x1088, then you have a compose rectangle. If it is 1920x1080 >> then >> you have a crop rectangle. > > 1920x1088 > >> >> As far as I can tell from this driver it actually has a compose rectangle >> and the use of g_crop is wrong and is there due to historical reasons (the >> driver predates the selection API). > > Yes, it is there due to historical reasons. > >> >>> I'm not really sure what to do with the existing code since it is an abuse of the crop API, but I guess the first step is to decide how this should be handled properly. Are there other decoders that can retrieve this information? Should this be mentioned in the stateful codec API? >>> >>> coda [1], mtk-vcodec [2] and venus [3] expose this using the >>> V4L2_SEL_TGT_COMPOSE selection target. v1 of the specification defines >>> the selection targets in a way, which is compatible with that: >>> V4L2_SEL_TGT_COMPOSE defaults to V4L2_SEL_TGT_COMPOSE_DEFAULT, which >>> equals to V4L2_SEL_TGT_CROP, which defaults to >>> V4L2_SEL_TGT_CROP_DEFAULT, which is defined as follows: >>> >>> + ``V4L2_SEL_TGT_CROP_DEFAULT`` >>> + a rectangle covering the part of the frame buffer that contains >>> + meaningful picture data (visible area); width and height will be >>> + equal to visible resolution of the stream >> >> Where do you get that from? That's the crop definition for an output stream, >> not a capture stream (assuming we have a codec). > > We're talking about a decoder here, right? > > In this case OUTPUT stream is just a sequence of bytes, not video > frames, so there is no selection defined for OUTPUT queue. > > CAPTURE stream should be seen as a video grabber, so CROP targets > relate to the encoded rectangle (1920x1088) and COMPOSE targets to the > CAPTURE buffer. V4L2_SEL_TGT_COMPOSE would be the part of the CAPTURE > buffer that is written with the image selected by V4L2_SEL_TGT_CROP. > > On a decoder that cannot do arbitrary crop and compose, like s5p-mfc, > both targets would have identical rectangles, equal to the visible > region (1920x1080). On hardware which can actually do fancier things, > userspace could freely configure CAPTURE buffer width and height and > V4L2_SEL_TGT_COMPOSE rectangle, for example to downscale the decoded > video on the fly. > > Please check how I specified all the targets in last version of the > specification (https://lore.kernel.org/patchwork/patch/966933/) and > comment there, if there is anything that goes against the > specification of the selection API. I think we all mean the same thing, but just got confused :-) > >> >> I kind of lost you with "which equals to V4L2_SEL_TGT_CROP". >> >> In any case, this particular driver should implement g_selection for >> CAPTURE and implement the COMPOSE targets. That makes sense. Right. Please check my RFC series I just posted that hopefully fixes this. Specifically https://patchwork.linuxtv.org/patch/52393/ and https://patchwork.linuxtv.org/patch/52388/ Regards, Hans
[GIT PULL FOR v4.20] Add Rockchip VPU JPEG encoder
This adds the Rockchip VPU JPEG encoder staging driver. Note: this goes on top of the request_api branch! It does not use the request API as such, but it would otherwise conflict with that series, so it is easier to just base it on top of the request_api branch. You will get a linker warning about a missing sunxi_sram_release symbol. That is resolved by this patch: https://lkml.org/lkml/2018/9/9/113 Which will go into 4.20 through another subsystem. Regards, Hans The following changes since commit 50e761516f2b8c0cdeb31a8c6ca1b4ef98cd13f1: media: platform: Add Cedrus VPU decoder driver (2018-09-24 10:47:10 -0400) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git tags/br-rkjpeg for you to fetch changes up to 8241137a4f3fb2c83a212887862c38f309736e82: media: add Rockchip VPU JPEG encoder driver (2018-10-05 10:28:55 +0200) Tag branch Ezequiel Garcia (2): dt-bindings: Document the Rockchip VPU bindings media: add Rockchip VPU JPEG encoder driver Shunqian Zheng (2): media: Add JPEG_RAW format media: Add controls for JPEG quantization tables Documentation/devicetree/bindings/media/rockchip-vpu.txt| 29 +++ Documentation/media/uapi/v4l/extended-controls.rst | 25 ++ Documentation/media/uapi/v4l/pixfmt-compressed.rst | 9 + Documentation/media/videodev2.h.rst.exceptions | 1 + MAINTAINERS | 7 + drivers/media/v4l2-core/v4l2-ctrls.c| 8 + drivers/media/v4l2-core/v4l2-ioctl.c| 1 + drivers/staging/media/Kconfig | 2 + drivers/staging/media/Makefile | 1 + drivers/staging/media/rockchip/vpu/Kconfig | 12 + drivers/staging/media/rockchip/vpu/Makefile | 9 + drivers/staging/media/rockchip/vpu/TODO | 9 + drivers/staging/media/rockchip/vpu/rk3288_vpu_hw.c | 125 ++ drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c | 127 +++ drivers/staging/media/rockchip/vpu/rk3288_vpu_regs.h| 442 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c | 125 ++ drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c | 155 + drivers/staging/media/rockchip/vpu/rk3399_vpu_regs.h| 600 drivers/staging/media/rockchip/vpu/rockchip_vpu.h | 278 +++ drivers/staging/media/rockchip/vpu/rockchip_vpu_common.h| 31 +++ drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c | 527 ++ drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c | 606 + drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h| 65 ++ include/uapi/linux/v4l2-controls.h | 10 + include/uapi/linux/videodev2.h | 2 + 25 files changed, 3206 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/rockchip-vpu.txt create mode 100644 drivers/staging/media/rockchip/vpu/Kconfig create mode 100644 drivers/staging/media/rockchip/vpu/Makefile create mode 100644 drivers/staging/media/rockchip/vpu/TODO create mode 100644 drivers/staging/media/rockchip/vpu/rk3288_vpu_hw.c create mode 100644 drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c create mode 100644 drivers/staging/media/rockchip/vpu/rk3288_vpu_regs.h create mode 100644 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c create mode 100644 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c create mode 100644 drivers/staging/media/rockchip/vpu/rk3399_vpu_regs.h create mode 100644 drivers/staging/media/rockchip/vpu/rockchip_vpu.h create mode 100644 drivers/staging/media/rockchip/vpu/rockchip_vpu_common.h create mode 100644 drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c create mode 100644 drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c create mode 100644 drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h
Re: s5p_mfc and H.264 frame cropping question
On Fri, Oct 5, 2018 at 3:58 PM Hans Verkuil wrote: > > On 10/05/2018 05:12 AM, Tomasz Figa wrote: > > Hi Hans, > > > > On Fri, Oct 5, 2018 at 5:02 AM Hans Verkuil wrote: > >> > >> Hi all, > >> > >> I'm looking at removing the last users of vidioc_g/s_crop from the driver > >> and > >> I came across vidioc_g_crop in > >> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c. > >> > >> What this really does AFAICS is return the H.264 frame crop as read from > >> the > >> bitstream. This has nothing to do with regular cropping/composing but it > >> might be > >> something that could be implemented as a new selection target. > > > > It has a lot to do, because the output frame buffer may contain (and > > on the hardware I worked with, s5p-mfc and mtk-vcodec, indeed does) > > the whole encoded stream and the frame crop from the bitstream > > specifies the rectangle within it that corresponds to the part that > > should be displayed. > > Yes, but is that part actually cropped? Or is the full uncropped image DMAed > to the capture buffer? The latter. > > To take a practical example: a H.264 stream with a 1920x1088 image and a frame > crop rectangle of 1920x1080. What is the G_FMT width/height for the decoder > capture stream: 1920x1088 or 1920x1080? > > If it is 1920x1088, then you have a compose rectangle. If it is 1920x1080 then > you have a crop rectangle. 1920x1088 > > As far as I can tell from this driver it actually has a compose rectangle > and the use of g_crop is wrong and is there due to historical reasons (the > driver predates the selection API). Yes, it is there due to historical reasons. > > > > >> > >> I'm not really sure what to do with the existing code since it is an abuse > >> of > >> the crop API, but I guess the first step is to decide how this should be > >> handled > >> properly. > >> > >> Are there other decoders that can retrieve this information? Should this be > >> mentioned in the stateful codec API? > > > > coda [1], mtk-vcodec [2] and venus [3] expose this using the > > V4L2_SEL_TGT_COMPOSE selection target. v1 of the specification defines > > the selection targets in a way, which is compatible with that: > > V4L2_SEL_TGT_COMPOSE defaults to V4L2_SEL_TGT_COMPOSE_DEFAULT, which > > equals to V4L2_SEL_TGT_CROP, which defaults to > > V4L2_SEL_TGT_CROP_DEFAULT, which is defined as follows: > > > > + ``V4L2_SEL_TGT_CROP_DEFAULT`` > > + a rectangle covering the part of the frame buffer that contains > > + meaningful picture data (visible area); width and height will be > > + equal to visible resolution of the stream > > Where do you get that from? That's the crop definition for an output stream, > not a capture stream (assuming we have a codec). We're talking about a decoder here, right? In this case OUTPUT stream is just a sequence of bytes, not video frames, so there is no selection defined for OUTPUT queue. CAPTURE stream should be seen as a video grabber, so CROP targets relate to the encoded rectangle (1920x1088) and COMPOSE targets to the CAPTURE buffer. V4L2_SEL_TGT_COMPOSE would be the part of the CAPTURE buffer that is written with the image selected by V4L2_SEL_TGT_CROP. On a decoder that cannot do arbitrary crop and compose, like s5p-mfc, both targets would have identical rectangles, equal to the visible region (1920x1080). On hardware which can actually do fancier things, userspace could freely configure CAPTURE buffer width and height and V4L2_SEL_TGT_COMPOSE rectangle, for example to downscale the decoded video on the fly. Please check how I specified all the targets in last version of the specification (https://lore.kernel.org/patchwork/patch/966933/) and comment there, if there is anything that goes against the specification of the selection API. > > I kind of lost you with "which equals to V4L2_SEL_TGT_CROP". > > In any case, this particular driver should implement g_selection for > CAPTURE and implement the COMPOSE targets. That makes sense. Right. Best regards, Tomasz
Re: [PATCH 2/3] media: v4l2-fwnode: cleanup functions that parse endpoints
Hi Mauro,
Feel free to ignore the comments on the first patch regarding the functions
below. There are other issues there though.
On Thu, Oct 04, 2018 at 06:13:47PM -0400, Mauro Carvalho Chehab wrote:
> There is already a typedef for the parse endpoint function.
> However, instead of using it, it is redefined at the C file
> (and on one of the function headers).
>
> Replace them by the function typedef, in order to cleanup
> several related coding style warnings.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 64 ---
> include/media/v4l2-fwnode.h | 19
> 2 files changed, 37 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 4e518d5fddd8..a7c2487154a4 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -596,12 +596,10 @@ EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>
> static int
> v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> - struct v4l2_async_notifier *notifier,
> - struct fwnode_handle *endpoint,
> - unsigned int asd_struct_size,
> - int (*parse_endpoint)(struct device *dev,
> - struct v4l2_fwnode_endpoint *vep,
> - struct v4l2_async_subdev *asd))
> + struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *endpoint,
> + unsigned int asd_struct_size,
> + parse_endpoint_func parse_endpoint)
> {
> struct v4l2_fwnode_endpoint vep = { .bus_type = 0 };
> struct v4l2_async_subdev *asd;
> @@ -657,13 +655,12 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device
> *dev,
> }
>
> static int
> -__v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
> - struct v4l2_async_notifier *notifier,
> - size_t asd_struct_size,
> - unsigned int port, bool has_port,
> - int (*parse_endpoint)(struct device *dev,
> - struct v4l2_fwnode_endpoint *vep,
> - struct v4l2_async_subdev *asd))
> +__v4l2_async_notifier_parse_fwnode_ep(struct device *dev,
> + struct v4l2_async_notifier *notifier,
> + size_t asd_struct_size,
> + unsigned int port,
> + bool has_port,
> + parse_endpoint_func parse_endpoint)
> {
> struct fwnode_handle *fwnode;
> int ret = 0;
> @@ -708,31 +705,27 @@ __v4l2_async_notifier_parse_fwnode_endpoints(struct
> device *dev,
>
> int
> v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
> - struct v4l2_async_notifier *notifier,
> - size_t asd_struct_size,
> - int (*parse_endpoint)(struct device *dev,
> - struct v4l2_fwnode_endpoint *vep,
> - struct v4l2_async_subdev *asd))
> +struct v4l2_async_notifier *notifier,
> +size_t asd_struct_size,
> +parse_endpoint_func parse_endpoint)
> {
> - return __v4l2_async_notifier_parse_fwnode_endpoints(dev, notifier,
> - asd_struct_size, 0,
> - false,
> - parse_endpoint);
> + return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
> + asd_struct_size, 0,
> + false, parse_endpoint);
> }
> EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
>
> int
> v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
> - struct v4l2_async_notifier *notifier,
> - size_t asd_struct_size, unsigned int port,
> - int (*parse_endpoint)(struct device *dev,
> - struct v4l2_fwnode_endpoint *vep,
> - struct v4l2_async_subdev *asd))
> +struct v4l2_async_notifier
> *notifier,
> +size_t asd_struct_size,
> +unsigned int port,
> +parse_endpoint_func
> parse_endpoint)
This is still over 80 here. I think we could think of abbreviating what's
in the function
Re: [PATCH 3/3] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() call
Hi Mauro,
On Thu, Oct 04, 2018 at 06:13:48PM -0400, Mauro Carvalho Chehab wrote:
> The v4l2_fwnode_reference_parse_int_props() has a big name, causing
> it to cause coding style warnings. Also, it depends on a const
> struct embedded indide a function.
>
> Rearrange the logic in order to move the struct declaration out
> of such function and use it inside this function.
>
> That cleans up some coding style issues.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index a7c2487154a4..e0cd119d6f5c 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -1006,6 +1006,12 @@ v4l2_fwnode_reference_get_int_prop(struct
> fwnode_handle *fwnode,
> return fwnode;
> }
>
> +struct v4l2_fwnode_int_props {
> + const char *name;
> + const char * const *props;
> + unsigned int nprops;
> +};
> +
> /*
> * v4l2_fwnode_reference_parse_int_props - parse references for async
> * sub-devices
> @@ -1032,13 +1038,14 @@ v4l2_fwnode_reference_get_int_prop(struct
> fwnode_handle *fwnode,
> static int
> v4l2_fwnode_reference_parse_int_props(struct device *dev,
> struct v4l2_async_notifier *notifier,
> - const char *prop,
> - const char * const *props,
> - unsigned int nprops)
> + const struct v4l2_fwnode_int_props *p)
> {
> struct fwnode_handle *fwnode;
> unsigned int index;
> int ret;
> + const char *prop = p->name;
> + const char * const *props = p->props;
> + unsigned int nprops = p->nprops;
>
> index = 0;
> do {
> @@ -1092,16 +1099,12 @@ v4l2_fwnode_reference_parse_int_props(struct device
> *dev,
> int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> struct v4l2_async_notifier
> *notifier)
> {
> + unsigned int i;
> static const char * const led_props[] = { "led" };
> - static const struct {
> - const char *name;
> - const char * const *props;
> - unsigned int nprops;
> - } props[] = {
> + static const struct v4l2_fwnode_int_props props[] = {
> { "flash-leds", led_props, ARRAY_SIZE(led_props) },
> { "lens-focus", NULL, 0 },
> };
> - unsigned int i;
I'd like to keep this here.
Apart from that,
Acked-by: Sakari Ailus
>
> for (i = 0; i < ARRAY_SIZE(props); i++) {
> int ret;
> @@ -1109,9 +1112,7 @@ int
> v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> if (props[i].props && is_acpi_node(dev_fwnode(dev)))
> ret = v4l2_fwnode_reference_parse_int_props(dev,
> notifier,
> -
> props[i].name,
> -
> props[i].props,
> -
> props[i].nprops);
> + [i]);
> else
> ret = v4l2_fwnode_reference_parse(dev, notifier,
> props[i].name);
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[PATCH] MAINTAINERS: update videobuf2 entry
Commits 03fbdb2fc2b8 ("media: move videobuf2 to drivers/media/common")
and 7952be9b6ece ("media: drivers/media/common/videobuf2: rename from
videobuf") moved videobuf2 framework source code finally to
drivers/media/common/videobuf2 directory, so update relevant paths in
MAINTAINERS file.
Reported-by: Joe Perches
Signed-off-by: Marek Szyprowski
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 29c08106bd22..4455fe05d3a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15720,7 +15720,7 @@ M: Marek Szyprowski
M: Kyungmin Park
L: linux-media@vger.kernel.org
S: Maintained
-F: drivers/media/v4l2-core/videobuf2-*
+F: drivers/media/common/videobuf2/*
F: include/media/videobuf2-*
VIMC VIRTUAL MEDIA CONTROLLER DRIVER
--
2.17.1
Re: [PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Hi Mauro,
On Thu, Oct 04, 2018 at 06:13:46PM -0400, Mauro Carvalho Chehab wrote:
> There are several coding style issues at those definitions,
> and the previous patchset added even more.
>
> Address the trivial ones by first calling:
>
> ./scripts/checkpatch.pl --strict --fix-inline
> include/media/v4l2-async.h include/media/v4l2-fwnode.h
> include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c
> drivers/media/v4l2-core/v4l2-fwnode.c
>
> and then manually adjusting the style where needed.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/v4l2-core/v4l2-async.c | 45 ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++---
> include/media/v4l2-async.h| 12 +-
> include/media/v4l2-fwnode.h | 46 ---
> include/media/v4l2-mediabus.h | 32 +++--
> 5 files changed, 179 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c
> index 70adbd9a01a2..6fdda745a1da 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -57,6 +57,7 @@ static bool match_i2c(struct v4l2_subdev *sd, struct
> v4l2_async_subdev *asd)
> {
> #if IS_ENABLED(CONFIG_I2C)
> struct i2c_client *client = i2c_verify_client(sd->dev);
> +
> return client &&
> asd->match.i2c.adapter_id == client->adapter->nr &&
> asd->match.i2c.address == client->addr;
> @@ -89,10 +90,11 @@ static LIST_HEAD(subdev_list);
> static LIST_HEAD(notifier_list);
> static DEFINE_MUTEX(list_lock);
>
> -static struct v4l2_async_subdev *v4l2_async_find_match(
> - struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> +static struct
> +v4l2_async_subdev *v4l2_async_find_match(struct v4l2_async_notifier
> *notifier,
This looks odd. If you want to rewrap it, then I'd put the stuff before and
including the first asterisk on the first line.
> + struct v4l2_subdev *sd)
> {
> - bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
> + bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
> struct v4l2_async_subdev *asd;
>
> list_for_each_entry(asd, >waiting, list) {
> @@ -150,8 +152,8 @@ static bool asd_equal(struct v4l2_async_subdev *asd_x,
> }
>
> /* Find the sub-device notifier registered by a sub-device driver. */
> -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> - struct v4l2_subdev *sd)
> +static struct v4l2_async_notifier *
> +v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd)
It's better here. Below, too.
> {
> struct v4l2_async_notifier *n;
>
> @@ -163,8 +165,8 @@ static struct v4l2_async_notifier
> *v4l2_async_find_subdev_notifier(
> }
>
> /* Get v4l2_device related to the notifier if one can be found. */
> -static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> - struct v4l2_async_notifier *notifier)
> +static struct v4l2_device *
> +v4l2_async_notifier_find_v4l2_dev(struct v4l2_async_notifier *notifier)
> {
> while (notifier->parent)
> notifier = notifier->parent;
> @@ -175,8 +177,8 @@ static struct v4l2_device
> *v4l2_async_notifier_find_v4l2_dev(
> /*
> * Return true if all child sub-device notifiers are complete, false
> otherwise.
> */
> -static bool v4l2_async_notifier_can_complete(
> - struct v4l2_async_notifier *notifier)
> +static bool
> +v4l2_async_notifier_can_complete(struct v4l2_async_notifier *notifier)
> {
> struct v4l2_subdev *sd;
>
> @@ -199,8 +201,8 @@ static bool v4l2_async_notifier_can_complete(
> * Complete the master notifier if possible. This is done when all async
> * sub-devices have been bound; v4l2_device is also available then.
> */
> -static int v4l2_async_notifier_try_complete(
> - struct v4l2_async_notifier *notifier)
> +static int
> +v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
> {
> /* Quick check whether there are still more sub-devices here. */
> if (!list_empty(>waiting))
> @@ -221,8 +223,8 @@ static int v4l2_async_notifier_try_complete(
> return v4l2_async_notifier_call_complete(notifier);
> }
>
> -static int v4l2_async_notifier_try_all_subdevs(
> - struct v4l2_async_notifier *notifier);
> +static int
> +v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier);
>
> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> struct v4l2_device *v4l2_dev,
> @@ -268,8 +270,8 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier,
> }
>
> /* Test all async sub-devices in a notifier for a match. */
> -static int v4l2_async_notifier_try_all_subdevs(
> - struct v4l2_async_notifier *notifier)
> +static int
> +v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier)
> {
> struct v4l2_device
[RFC PATCH 05/11] cropcap/g_selection split
From: Hans Verkuil
If g_selection is implemented, then the v4l2-ioctl cropcap code assumes
that cropcap just implements the pixelaspect part and that g_selection
provides the crop bounds and default rectangles.
There are still some drivers that only implement cropcap and not
g_selection. Split up cropcap into a cropcap and g_selection for those
drivers.
Signed-off-by: Hans Verkuil
---
drivers/media/pci/cobalt/cobalt-v4l2.c | 38 ++---
drivers/media/pci/cx23885/cx23885-video.c | 28 ---
drivers/media/platform/am437x/am437x-vpfe.c | 18 +-
drivers/media/usb/au0828/au0828-video.c | 30
drivers/media/usb/cpia2/cpia2_v4l.c | 31 +
drivers/media/usb/cx231xx/cx231xx-417.c | 29 +---
drivers/media/usb/cx231xx/cx231xx-video.c | 29 +---
7 files changed, 152 insertions(+), 51 deletions(-)
diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c
b/drivers/media/pci/cobalt/cobalt-v4l2.c
index 0525f5e1565b..4a0205aae4b4 100644
--- a/drivers/media/pci/cobalt/cobalt-v4l2.c
+++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
@@ -1089,14 +1089,43 @@ static int cobalt_cropcap(struct file *file, void *fh,
struct v4l2_cropcap *cc)
timings = cea1080p60;
else
err = v4l2_subdev_call(s->sd, video, g_dv_timings, );
- if (!err) {
- cc->bounds.width = cc->defrect.width = timings.bt.width;
- cc->bounds.height = cc->defrect.height = timings.bt.height;
+ if (!err)
cc->pixelaspect = v4l2_dv_timings_aspect_ratio();
- }
return err;
}
+static int cobalt_g_selection(struct file *file, void *fh,
+ struct v4l2_selection *sel)
+{
+ struct cobalt_stream *s = video_drvdata(file);
+ struct v4l2_dv_timings timings;
+ int err = 0;
+
+ if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ if (s->input == 1)
+ timings = cea1080p60;
+ else
+ err = v4l2_subdev_call(s->sd, video, g_dv_timings, );
+
+ if (err)
+ return err;
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ sel->r.top = 0;
+ sel->r.left = 0;
+ sel->r.width = timings.bt.width;
+ sel->r.height = timings.bt.height;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
static const struct v4l2_ioctl_ops cobalt_ioctl_ops = {
.vidioc_querycap= cobalt_querycap,
.vidioc_g_parm = cobalt_g_parm,
@@ -1104,6 +1133,7 @@ static const struct v4l2_ioctl_ops cobalt_ioctl_ops = {
.vidioc_streamon= vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,
.vidioc_cropcap = cobalt_cropcap,
+ .vidioc_g_selection = cobalt_g_selection,
.vidioc_enum_input = cobalt_enum_input,
.vidioc_g_input = cobalt_g_input,
.vidioc_s_input = cobalt_s_input,
diff --git a/drivers/media/pci/cx23885/cx23885-video.c
b/drivers/media/pci/cx23885/cx23885-video.c
index 92d32a733f1b..a9844c4020ff 100644
--- a/drivers/media/pci/cx23885/cx23885-video.c
+++ b/drivers/media/pci/cx23885/cx23885-video.c
@@ -677,17 +677,34 @@ static int vidioc_cropcap(struct file *file, void *priv,
if (cc->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
- cc->bounds.left = 0;
- cc->bounds.top = 0;
- cc->bounds.width = 720;
- cc->bounds.height = norm_maxh(dev->tvnorm);
- cc->defrect = cc->bounds;
cc->pixelaspect.numerator = is_50hz ? 54 : 11;
cc->pixelaspect.denominator = is_50hz ? 59 : 10;
return 0;
}
+static int vidioc_g_selection(struct file *file, void *fh,
+ struct v4l2_selection *sel)
+{
+ struct cx23885_dev *dev = video_drvdata(file);
+
+ if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ sel->r.top = 0;
+ sel->r.left = 0;
+ sel->r.width = 720;
+ sel->r.height = norm_maxh(dev->tvnorm);
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int vidioc_g_std(struct file *file, void *priv, v4l2_std_id *id)
{
struct cx23885_dev *dev = video_drvdata(file);
@@ -1123,6 +1140,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
.vidioc_streamon = vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,
.vidioc_cropcap = vidioc_cropcap,
+ .vidioc_g_selection = vidioc_g_selection,
[RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
From: Hans Verkuil This patch series converts the last remaining drivers that use g/s_crop and cropcap to g/s_selection. The first two patches do some minor code cleanup. The third patch adds a new video_device flag to indicate that the driver inverts the normal usage of g/s_crop/cropcap. This applies to the old Samsung drivers that predate the Selection API and that abused the existing crop API. The next three patches do some code cleanup and prepare drivers for the removal of g/s_crop and ensure that cropcap only returns the pixelaspect. The next three patches convert the remaining Samsung drivers and set the QUIRK flag for all three. The final two patches remove vidioc_g/s_crop and rename vidioc_cropcap to vidioc_g_pixelaspect. I would really appreciate it if someone from Samsung can test these three drivers or at the very least review the code. Niklas, this series supersedes your 'v4l2-ioctl: fix CROPCAP type handling' patch. Sorry about that :-) Regards, Hans Hans Verkuil (11): v4l2-ioctl: don't use CROP/COMPOSE_ACTIVE v4l2-common.h: put backwards compat defines under #ifndef __KERNEL__ v4l2-ioctl: add QUIRK_INVERTED_CROP davinci/vpbe: drop unused g_cropcap cropcap/g_selection split exynos-gsc: replace v4l2_crop by v4l2_selection s5p_mfc_dec.c: convert g_crop to g_selection exynos4-is: convert g/s_crop to g/s_selection s5p-g2d: convert g/s_crop to g/s_selection v4l2-ioctl: remove unused vidioc_g/s_crop vidioc_cropcap -> vidioc_g_pixelaspect drivers/media/pci/bt8xx/bttv-driver.c | 12 +- drivers/media/pci/cobalt/cobalt-v4l2.c| 48 +-- drivers/media/pci/cx18/cx18-ioctl.c | 13 +- drivers/media/pci/cx23885/cx23885-video.c | 40 -- drivers/media/pci/ivtv/ivtv-ioctl.c | 17 +-- drivers/media/pci/saa7134/saa7134-video.c | 21 ++- drivers/media/platform/am437x/am437x-vpfe.c | 31 ++--- drivers/media/platform/davinci/vpbe.c | 23 drivers/media/platform/davinci/vpbe_display.c | 10 +- drivers/media/platform/davinci/vpfe_capture.c | 12 +- drivers/media/platform/exynos-gsc/gsc-core.c | 57 +++- drivers/media/platform/exynos-gsc/gsc-core.h | 3 +- drivers/media/platform/exynos-gsc/gsc-m2m.c | 23 ++-- drivers/media/platform/exynos4-is/fimc-core.h | 6 +- drivers/media/platform/exynos4-is/fimc-m2m.c | 130 ++ drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 +- drivers/media/platform/s5p-g2d/g2d.c | 102 +- drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 + drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 49 --- drivers/media/platform/vivid/vivid-core.c | 9 +- drivers/media/platform/vivid/vivid-vid-cap.c | 18 ++- drivers/media/platform/vivid/vivid-vid-cap.h | 2 +- drivers/media/platform/vivid/vivid-vid-out.c | 18 ++- drivers/media/platform/vivid/vivid-vid-out.h | 2 +- drivers/media/usb/au0828/au0828-video.c | 38 +++-- drivers/media/usb/cpia2/cpia2_v4l.c | 31 +++-- drivers/media/usb/cx231xx/cx231xx-417.c | 41 -- drivers/media/usb/cx231xx/cx231xx-video.c | 41 -- drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 13 +- drivers/media/v4l2-core/v4l2-dev.c| 8 +- drivers/media/v4l2-core/v4l2-ioctl.c | 44 -- include/media/davinci/vpbe.h | 4 - include/media/v4l2-dev.h | 13 +- include/media/v4l2-ioctl.h| 16 +-- include/uapi/linux/v4l2-common.h | 28 ++-- 35 files changed, 537 insertions(+), 397 deletions(-) -- 2.18.0
[RFC PATCH 06/11] exynos-gsc: replace v4l2_crop by v4l2_selection
From: Hans Verkuil
Replace the use of struct v4l2_crop by struct v4l2_selection.
Also drop the unused gsc_g_crop function.
Signed-off-by: Hans Verkuil
---
drivers/media/platform/exynos-gsc/gsc-core.c | 57
drivers/media/platform/exynos-gsc/gsc-core.h | 3 +-
drivers/media/platform/exynos-gsc/gsc-m2m.c | 23
3 files changed, 33 insertions(+), 50 deletions(-)
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 838c5c53de37..0fa3ec04ab7b 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -541,20 +541,7 @@ void gsc_check_crop_change(u32 tmp_w, u32 tmp_h, u32 *w,
u32 *h)
}
}
-int gsc_g_crop(struct gsc_ctx *ctx, struct v4l2_crop *cr)
-{
- struct gsc_frame *frame;
-
- frame = ctx_get_frame(ctx, cr->type);
- if (IS_ERR(frame))
- return PTR_ERR(frame);
-
- cr->c = frame->crop;
-
- return 0;
-}
-
-int gsc_try_crop(struct gsc_ctx *ctx, struct v4l2_crop *cr)
+int gsc_try_selection(struct gsc_ctx *ctx, struct v4l2_selection *s)
{
struct gsc_frame *f;
struct gsc_dev *gsc = ctx->gsc_dev;
@@ -562,25 +549,25 @@ int gsc_try_crop(struct gsc_ctx *ctx, struct v4l2_crop
*cr)
u32 mod_x = 0, mod_y = 0, tmp_w, tmp_h;
u32 min_w, min_h, max_w, max_h;
- if (cr->c.top < 0 || cr->c.left < 0) {
+ if (s->r.top < 0 || s->r.left < 0) {
pr_err("doesn't support negative values for top & left\n");
return -EINVAL;
}
- pr_debug("user put w: %d, h: %d", cr->c.width, cr->c.height);
+ pr_debug("user put w: %d, h: %d", s->r.width, s->r.height);
- if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
f = >d_frame;
- else if (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
f = >s_frame;
else
return -EINVAL;
max_w = f->f_width;
max_h = f->f_height;
- tmp_w = cr->c.width;
- tmp_h = cr->c.height;
+ tmp_w = s->r.width;
+ tmp_h = s->r.height;
- if (V4L2_TYPE_IS_OUTPUT(cr->type)) {
+ if (V4L2_TYPE_IS_OUTPUT(s->type)) {
if ((is_yuv422(f->fmt->color) && f->fmt->num_comp == 1) ||
is_rgb(f->fmt->color))
min_w = 32;
@@ -602,8 +589,8 @@ int gsc_try_crop(struct gsc_ctx *ctx, struct v4l2_crop *cr)
max_h = f->f_width;
min_w = variant->pix_min->target_rot_en_w;
min_h = variant->pix_min->target_rot_en_h;
- tmp_w = cr->c.height;
- tmp_h = cr->c.width;
+ tmp_w = s->r.height;
+ tmp_h = s->r.width;
} else {
min_w = variant->pix_min->target_rot_dis_w;
min_h = variant->pix_min->target_rot_dis_h;
@@ -616,29 +603,29 @@ int gsc_try_crop(struct gsc_ctx *ctx, struct v4l2_crop
*cr)
v4l_bound_align_image(_w, min_w, max_w, mod_x,
_h, min_h, max_h, mod_y, 0);
- if (!V4L2_TYPE_IS_OUTPUT(cr->type) &&
- (ctx->gsc_ctrls.rotate->val == 90 ||
- ctx->gsc_ctrls.rotate->val == 270))
+ if (!V4L2_TYPE_IS_OUTPUT(s->type) &&
+ (ctx->gsc_ctrls.rotate->val == 90 ||
+ctx->gsc_ctrls.rotate->val == 270))
gsc_check_crop_change(tmp_h, tmp_w,
- >c.width, >c.height);
+ >r.width, >r.height);
else
gsc_check_crop_change(tmp_w, tmp_h,
- >c.width, >c.height);
+ >r.width, >r.height);
/* adjust left/top if cropping rectangle is out of bounds */
/* Need to add code to algin left value with 2's multiple */
- if (cr->c.left + tmp_w > max_w)
- cr->c.left = max_w - tmp_w;
- if (cr->c.top + tmp_h > max_h)
- cr->c.top = max_h - tmp_h;
+ if (s->r.left + tmp_w > max_w)
+ s->r.left = max_w - tmp_w;
+ if (s->r.top + tmp_h > max_h)
+ s->r.top = max_h - tmp_h;
if ((is_yuv420(f->fmt->color) || is_yuv422(f->fmt->color)) &&
- cr->c.left & 1)
- cr->c.left -= 1;
+ s->r.left & 1)
+ s->r.left -= 1;
pr_debug("Aligned l:%d, t:%d, w:%d, h:%d, f_w: %d, f_h: %d",
- cr->c.left, cr->c.top, cr->c.width, cr->c.height, max_w, max_h);
+s->r.left, s->r.top, s->r.width, s->r.height, max_w, max_h);
return 0;
}
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h
b/drivers/media/platform/exynos-gsc/gsc-core.h
index
[RFC PATCH 04/11] davinci/vpbe: drop unused g_cropcap
From: Hans Verkuil
This function/callback is never used. Drop it.
Signed-off-by: Hans Verkuil
---
drivers/media/platform/davinci/vpbe.c | 23 ---
include/media/davinci/vpbe.h | 4
2 files changed, 27 deletions(-)
diff --git a/drivers/media/platform/davinci/vpbe.c
b/drivers/media/platform/davinci/vpbe.c
index 18c035ef84cf..e80d7806cc45 100644
--- a/drivers/media/platform/davinci/vpbe.c
+++ b/drivers/media/platform/davinci/vpbe.c
@@ -92,28 +92,6 @@ static int vpbe_find_encoder_sd_index(struct vpbe_config
*cfg,
return -EINVAL;
}
-/**
- * vpbe_g_cropcap - Get crop capabilities of the display
- * @vpbe_dev: vpbe device ptr
- * @cropcap: cropcap is a ptr to struct v4l2_cropcap
- *
- * Update the crop capabilities in crop cap for current
- * mode
- */
-static int vpbe_g_cropcap(struct vpbe_device *vpbe_dev,
- struct v4l2_cropcap *cropcap)
-{
- if (!cropcap)
- return -EINVAL;
- cropcap->bounds.left = 0;
- cropcap->bounds.top = 0;
- cropcap->bounds.width = vpbe_dev->current_timings.xres;
- cropcap->bounds.height = vpbe_dev->current_timings.yres;
- cropcap->defrect = cropcap->bounds;
-
- return 0;
-}
-
/**
* vpbe_enum_outputs - enumerate outputs
* @vpbe_dev: vpbe device ptr
@@ -793,7 +771,6 @@ static void vpbe_deinitialize(struct device *dev, struct
vpbe_device *vpbe_dev)
}
static const struct vpbe_device_ops vpbe_dev_ops = {
- .g_cropcap = vpbe_g_cropcap,
.enum_outputs = vpbe_enum_outputs,
.set_output = vpbe_set_output,
.get_output = vpbe_get_output,
diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 79a566d7defd..5c31a7682492 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -100,10 +100,6 @@ struct vpbe_config {
struct vpbe_device;
struct vpbe_device_ops {
- /* crop cap for the display */
- int (*g_cropcap)(struct vpbe_device *vpbe_dev,
-struct v4l2_cropcap *cropcap);
-
/* Enumerate the outputs */
int (*enum_outputs)(struct vpbe_device *vpbe_dev,
struct v4l2_output *output);
--
2.18.0
[RFC PATCH 08/11] exynos4-is: convert g/s_crop to g/s_selection
From: Hans Verkuil
Replace g/s_crop by g/s_selection and set the V4L2_FL_QUIRK_INVERTED_CROP
flag since this is one of the old drivers that predates the selection
API. Those old drivers allowed g_crop when it really shouldn't have since
g_crop returns a compose rectangle instead of a crop rectangle for the
CAPTURE stream, and vice versa for the OUTPUT stream.
Also drop the now unused vidioc_cropcap.
Signed-off-by: Hans Verkuil
---
drivers/media/platform/exynos4-is/fimc-core.h | 6 +-
drivers/media/platform/exynos4-is/fimc-m2m.c | 130 ++
2 files changed, 79 insertions(+), 57 deletions(-)
diff --git a/drivers/media/platform/exynos4-is/fimc-core.h
b/drivers/media/platform/exynos4-is/fimc-core.h
index 82d514df97f0..9f751a5efd64 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.h
+++ b/drivers/media/platform/exynos4-is/fimc-core.h
@@ -596,12 +596,14 @@ static inline struct fimc_frame *ctx_get_frame(struct
fimc_ctx *ctx,
{
struct fimc_frame *frame;
- if (V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE == type) {
+ if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ||
+ type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
if (fimc_ctx_state_is_set(FIMC_CTX_M2M, ctx))
frame = >s_frame;
else
return ERR_PTR(-EINVAL);
- } else if (V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE == type) {
+ } else if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ||
+ type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
frame = >d_frame;
} else {
v4l2_err(ctx->fimc_dev->v4l2_dev,
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c
b/drivers/media/platform/exynos4-is/fimc-m2m.c
index a19f8b164a47..61c8177409cf 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -383,60 +383,80 @@ static int fimc_m2m_s_fmt_mplane(struct file *file, void
*fh,
return 0;
}
-static int fimc_m2m_cropcap(struct file *file, void *fh,
- struct v4l2_cropcap *cr)
+static int fimc_m2m_g_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
{
struct fimc_ctx *ctx = fh_to_ctx(fh);
struct fimc_frame *frame;
- frame = ctx_get_frame(ctx, cr->type);
+ frame = ctx_get_frame(ctx, s->type);
if (IS_ERR(frame))
return PTR_ERR(frame);
- cr->bounds.left = 0;
- cr->bounds.top = 0;
- cr->bounds.width = frame->o_width;
- cr->bounds.height = frame->o_height;
- cr->defrect = cr->bounds;
-
- return 0;
-}
-
-static int fimc_m2m_g_crop(struct file *file, void *fh, struct v4l2_crop *cr)
-{
- struct fimc_ctx *ctx = fh_to_ctx(fh);
- struct fimc_frame *frame;
-
- frame = ctx_get_frame(ctx, cr->type);
- if (IS_ERR(frame))
- return PTR_ERR(frame);
-
- cr->c.left = frame->offs_h;
- cr->c.top = frame->offs_v;
- cr->c.width = frame->width;
- cr->c.height = frame->height;
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ return -EINVAL;
+ break;
+ case V4L2_SEL_TGT_COMPOSE:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ case V4L2_SEL_TGT_COMPOSE:
+ s->r.left = frame->offs_h;
+ s->r.top = frame->offs_v;
+ s->r.width = frame->width;
+ s->r.height = frame->height;
+ break;
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = frame->o_width;
+ s->r.height = frame->o_height;
+ break;
+ default:
+ return -EINVAL;
+ }
return 0;
}
-static int fimc_m2m_try_crop(struct fimc_ctx *ctx, struct v4l2_crop *cr)
+static int fimc_m2m_try_selection(struct fimc_ctx *ctx,
+ struct v4l2_selection *s)
{
struct fimc_dev *fimc = ctx->fimc_dev;
struct fimc_frame *f;
u32 min_size, halign, depth = 0;
int i;
- if (cr->c.top < 0 || cr->c.left < 0) {
+ if (s->r.top < 0 || s->r.left < 0) {
v4l2_err(>m2m.vfd,
"doesn't support negative values for top & left\n");
return -EINVAL;
}
- if (cr->type ==
[RFC PATCH 02/11] v4l2-common.h: put backwards compat defines under #ifndef __KERNEL__
From: Hans Verkuil
This ensures that they won't be used in kernel code.
Signed-off-by: Hans Verkuil
---
include/uapi/linux/v4l2-common.h | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 4f7b892377cd..7d21c1634b4d 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -79,24 +79,11 @@
/* Current composing area plus all padding pixels */
#define V4L2_SEL_TGT_COMPOSE_PADDED0x0103
-/* Backward compatibility target definitions --- to be removed. */
-#define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
-#define V4L2_SEL_TGT_COMPOSE_ACTIVEV4L2_SEL_TGT_COMPOSE
-#define V4L2_SUBDEV_SEL_TGT_CROP_ACTUALV4L2_SEL_TGT_CROP
-#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL V4L2_SEL_TGT_COMPOSE
-#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDSV4L2_SEL_TGT_CROP_BOUNDS
-#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS V4L2_SEL_TGT_COMPOSE_BOUNDS
-
/* Selection flags */
#define V4L2_SEL_FLAG_GE (1 << 0)
#define V4L2_SEL_FLAG_LE (1 << 1)
#define V4L2_SEL_FLAG_KEEP_CONFIG (1 << 2)
-/* Backward compatibility flag definitions --- to be removed. */
-#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE V4L2_SEL_FLAG_GE
-#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE V4L2_SEL_FLAG_LE
-#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG V4L2_SEL_FLAG_KEEP_CONFIG
-
struct v4l2_edid {
__u32 pad;
__u32 start_block;
@@ -105,4 +92,19 @@ struct v4l2_edid {
__u8 *edid;
};
+#ifndef __KERNEL__
+/* Backward compatibility target definitions --- to be removed. */
+#define V4L2_SEL_TGT_CROP_ACTIVE V4L2_SEL_TGT_CROP
+#define V4L2_SEL_TGT_COMPOSE_ACTIVEV4L2_SEL_TGT_COMPOSE
+#define V4L2_SUBDEV_SEL_TGT_CROP_ACTUALV4L2_SEL_TGT_CROP
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL V4L2_SEL_TGT_COMPOSE
+#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDSV4L2_SEL_TGT_CROP_BOUNDS
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS V4L2_SEL_TGT_COMPOSE_BOUNDS
+
+/* Backward compatibility flag definitions --- to be removed. */
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE V4L2_SEL_FLAG_GE
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE V4L2_SEL_FLAG_LE
+#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG V4L2_SEL_FLAG_KEEP_CONFIG
+#endif
+
#endif /* __V4L2_COMMON__ */
--
2.18.0
[RFC PATCH 03/11] v4l2-ioctl: add QUIRK_INVERTED_CROP
From: Hans Verkuil
Some old Samsung drivers use the legacy crop API incorrectly:
the crop and compose targets are swapped. Normally VIDIOC_G_CROP
will return the CROP rectangle of a CAPTURE stream and the COMPOSE
rectangle of an OUTPUT stream.
The Samsung drivers do the opposite. Note that these drivers predate
the selection API.
If this 'QUIRK' flag is set, then the v4l2-ioctl core will swap
the CROP and COMPOSE targets as well.
That way backwards compatibility is ensured and we can convert the
Samsung drivers to the selection API.
Signed-off-by: Hans Verkuil
---
drivers/media/v4l2-core/v4l2-ioctl.c | 17 -
include/media/v4l2-dev.h | 13 +++--
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 9c2370e4d05c..63a92285de39 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2200,6 +2200,7 @@ static int v4l_s_selection(const struct v4l2_ioctl_ops
*ops,
static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
+ struct video_device *vfd = video_devdata(file);
struct v4l2_crop *p = arg;
struct v4l2_selection s = {
.type = p->type,
@@ -2216,6 +2217,10 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
else
s.target = V4L2_SEL_TGT_CROP;
+ if (test_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags))
+ s.target = s.target == V4L2_SEL_TGT_COMPOSE ?
+ V4L2_SEL_TGT_CROP : V4L2_SEL_TGT_COMPOSE;
+
ret = v4l_g_selection(ops, file, fh, );
/* copying results to old structure on success */
@@ -2227,6 +2232,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
+ struct video_device *vfd = video_devdata(file);
struct v4l2_crop *p = arg;
struct v4l2_selection s = {
.type = p->type,
@@ -2243,12 +2249,17 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
else
s.target = V4L2_SEL_TGT_CROP;
+ if (test_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags))
+ s.target = s.target == V4L2_SEL_TGT_COMPOSE ?
+ V4L2_SEL_TGT_CROP : V4L2_SEL_TGT_COMPOSE;
+
return v4l_s_selection(ops, file, fh, );
}
static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
+ struct video_device *vfd = video_devdata(file);
struct v4l2_cropcap *p = arg;
struct v4l2_selection s = { .type = p->type };
int ret = 0;
@@ -2285,13 +2296,17 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
else
s.target = V4L2_SEL_TGT_CROP_BOUNDS;
+ if (test_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags))
+ s.target = s.target == V4L2_SEL_TGT_COMPOSE_BOUNDS ?
+ V4L2_SEL_TGT_CROP_BOUNDS : V4L2_SEL_TGT_COMPOSE_BOUNDS;
+
ret = v4l_g_selection(ops, file, fh, );
if (ret)
return ret;
p->bounds = s.r;
/* obtaining defrect */
- if (V4L2_TYPE_IS_OUTPUT(p->type))
+ if (s.target == V4L2_SEL_TGT_COMPOSE_BOUNDS)
s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
else
s.target = V4L2_SEL_TGT_CROP_DEFAULT;
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 456ac13eca1d..48531e57cc5a 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -74,10 +74,19 @@ struct v4l2_ctrl_handler;
* indicates that file->private_data points to v4l2_fh.
* This flag is set by the core when v4l2_fh_init() is called.
* All new drivers should use it.
+ * @V4L2_FL_QUIRK_INVERTED_CROP:
+ * some old M2M drivers use g/s_crop/cropcap incorrectly: crop and
+ * compose are swapped. If this flag is set, then the selection
+ * targets are swapped in the g/s_crop/cropcap functions in v4l2-ioctl.c.
+ * This allows those drivers to correctly implement the selection API,
+ * but the old crop API will still work as expected in order to preserve
+ * backwards compatibility.
+ * Never set this flag for new drivers.
*/
enum v4l2_video_device_flags {
- V4L2_FL_REGISTERED = 0,
- V4L2_FL_USES_V4L2_FH= 1,
+ V4L2_FL_REGISTERED = 0,
+ V4L2_FL_USES_V4L2_FH= 1,
+ V4L2_FL_QUIRK_INVERTED_CROP = 2,
};
/* Priority helper functions */
--
2.18.0
[RFC PATCH 07/11] s5p_mfc_dec.c: convert g_crop to g_selection
From: Hans Verkuil
The g_crop really implemented composition for the CAPTURE stream.
Replace g_crop by g_selection and set the V4L2_FL_QUIRK_INVERTED_CROP
flag since this is one of the old drivers that predates the selection
API. Those old drivers allowed g_crop when it really shouldn't have
since g_crop returns a compose rectangle instead of a crop rectangle.
Signed-off-by: Hans Verkuil
---
drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 +
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 49 +---
2 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 927a1235408d..8a5ba3bec3af 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1342,6 +1342,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
vfd->lock = >mfc_mutex;
vfd->v4l2_dev = >v4l2_dev;
vfd->vfl_dir= VFL_DIR_M2M;
+ set_bit(V4L2_FL_QUIRK_INVERTED_CROP, >flags);
snprintf(vfd->name, sizeof(vfd->name), "%s", S5P_MFC_DEC_NAME);
dev->vfd_dec= vfd;
video_set_drvdata(vfd, dev);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 670ca869babb..eaaf1418b0fa 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -773,19 +773,23 @@ static const struct v4l2_ctrl_ops s5p_mfc_dec_ctrl_ops = {
.g_volatile_ctrl = s5p_mfc_dec_g_v_ctrl,
};
-/* Get cropping information */
-static int vidioc_g_crop(struct file *file, void *priv,
- struct v4l2_crop *cr)
+/* Get compose information */
+static int vidioc_g_selection(struct file *file, void *priv,
+ struct v4l2_selection *s)
{
struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
struct s5p_mfc_dev *dev = ctx->dev;
u32 left, right, top, bottom;
+ u32 width, height;
+
+ if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
if (ctx->state != MFCINST_HEAD_PARSED &&
ctx->state != MFCINST_RUNNING &&
ctx->state != MFCINST_FINISHING &&
ctx->state != MFCINST_FINISHED) {
- mfc_err("Can not get crop information\n");
+ mfc_err("Can not get compose information\n");
return -EINVAL;
}
if (ctx->src_fmt->fourcc == V4L2_PIX_FMT_H264) {
@@ -795,22 +799,33 @@ static int vidioc_g_crop(struct file *file, void *priv,
top = s5p_mfc_hw_call(dev->mfc_ops, get_crop_info_v, ctx);
bottom = top >> S5P_FIMV_SHARED_CROP_BOTTOM_SHIFT;
top = top & S5P_FIMV_SHARED_CROP_TOP_MASK;
- cr->c.left = left;
- cr->c.top = top;
- cr->c.width = ctx->img_width - left - right;
- cr->c.height = ctx->img_height - top - bottom;
- mfc_debug(2, "Cropping info [h264]: l=%d t=%d w=%d h=%d (r=%d
b=%d fw=%d fh=%d\n",
- left, top, cr->c.width, cr->c.height, right, bottom,
+ width = ctx->img_width - left - right;
+ height = ctx->img_height - top - bottom;
+ mfc_debug(2, "Composing info [h264]: l=%d t=%d w=%d h=%d (r=%d
b=%d fw=%d fh=%d\n",
+ left, top, s->r.width, s->r.height, right, bottom,
ctx->buf_width, ctx->buf_height);
} else {
- cr->c.left = 0;
- cr->c.top = 0;
- cr->c.width = ctx->img_width;
- cr->c.height = ctx->img_height;
- mfc_debug(2, "Cropping info: w=%d h=%d fw=%d fh=%d\n",
- cr->c.width, cr->c.height, ctx->buf_width,
+ left = 0;
+ top = 0;
+ width = ctx->img_width;
+ height = ctx->img_height;
+ mfc_debug(2, "Composing info: w=%d h=%d fw=%d fh=%d\n",
+ s->r.width, s->r.height, ctx->buf_width,
ctx->buf_height);
}
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_COMPOSE:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ s->r.left = left;
+ s->r.top = top;
+ s->r.width = width;
+ s->r.height = height;
+ break;
+ default:
+ return -EINVAL;
+ }
return 0;
}
@@ -887,7 +902,7 @@ static const struct v4l2_ioctl_ops s5p_mfc_dec_ioctl_ops = {
.vidioc_expbuf = vidioc_expbuf,
.vidioc_streamon = vidioc_streamon,
.vidioc_streamoff = vidioc_streamoff,
- .vidioc_g_crop = vidioc_g_crop,
+ .vidioc_g_selection = vidioc_g_selection,
.vidioc_decoder_cmd = vidioc_decoder_cmd,
.vidioc_subscribe_event = vidioc_subscribe_event,
.vidioc_unsubscribe_event =
[RFC PATCH 09/11] s5p-g2d: convert g/s_crop to g/s_selection
From: Hans Verkuil
Replace g/s_crop by g/s_selection and set the V4L2_FL_QUIRK_INVERTED_CROP
flag since this is one of the old drivers that predates the selection
API. Those old drivers allowed g_crop when it really shouldn't have since
g_crop returns a compose rectangle instead of a crop rectangle for the
CAPTURE stream, and vice versa for the OUTPUT stream.
Also drop the now unused vidioc_cropcap.
Signed-off-by: Hans Verkuil
---
drivers/media/platform/s5p-g2d/g2d.c | 102 +--
1 file changed, 64 insertions(+), 38 deletions(-)
diff --git a/drivers/media/platform/s5p-g2d/g2d.c
b/drivers/media/platform/s5p-g2d/g2d.c
index e901201b6fcc..57ab1d1085d1 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -89,7 +89,7 @@ static struct g2d_fmt *find_fmt(struct v4l2_format *f)
static struct g2d_frame *get_frame(struct g2d_ctx *ctx,
- enum v4l2_buf_type type)
+ enum v4l2_buf_type type)
{
switch (type) {
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
@@ -408,51 +408,76 @@ static int vidioc_s_fmt(struct file *file, void *prv,
struct v4l2_format *f)
return 0;
}
-static int vidioc_cropcap(struct file *file, void *priv,
- struct v4l2_cropcap *cr)
-{
- struct g2d_ctx *ctx = priv;
- struct g2d_frame *f;
-
- f = get_frame(ctx, cr->type);
- if (IS_ERR(f))
- return PTR_ERR(f);
-
- cr->bounds.left = 0;
- cr->bounds.top = 0;
- cr->bounds.width= f->width;
- cr->bounds.height = f->height;
- cr->defrect = cr->bounds;
- return 0;
-}
-
-static int vidioc_g_crop(struct file *file, void *prv, struct v4l2_crop *cr)
+static int vidioc_g_selection(struct file *file, void *prv,
+ struct v4l2_selection *s)
{
struct g2d_ctx *ctx = prv;
struct g2d_frame *f;
- f = get_frame(ctx, cr->type);
+ f = get_frame(ctx, s->type);
if (IS_ERR(f))
return PTR_ERR(f);
- cr->c.left = f->o_height;
- cr->c.top = f->o_width;
- cr->c.width = f->c_width;
- cr->c.height= f->c_height;
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ return -EINVAL;
+ break;
+ case V4L2_SEL_TGT_COMPOSE:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ case V4L2_SEL_TGT_COMPOSE:
+ s->r.left = f->o_height;
+ s->r.top = f->o_width;
+ s->r.width = f->c_width;
+ s->r.height = f->c_height;
+ break;
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = f->width;
+ s->r.height = f->height;
+ break;
+ default:
+ return -EINVAL;
+ }
return 0;
}
-static int vidioc_try_crop(struct file *file, void *prv, const struct
v4l2_crop *cr)
+static int vidioc_try_selection(struct file *file, void *prv,
+ const struct v4l2_selection *s)
{
struct g2d_ctx *ctx = prv;
struct g2d_dev *dev = ctx->dev;
struct g2d_frame *f;
- f = get_frame(ctx, cr->type);
+ f = get_frame(ctx, s->type);
if (IS_ERR(f))
return PTR_ERR(f);
- if (cr->c.top < 0 || cr->c.left < 0) {
+ if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+ if (s->target != V4L2_SEL_TGT_COMPOSE)
+ return -EINVAL;
+ } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+ if (s->target != V4L2_SEL_TGT_CROP)
+ return -EINVAL;
+ }
+
+ if (s->r.top < 0 || s->r.left < 0) {
v4l2_err(>v4l2_dev,
"doesn't support negative values for top & left\n");
return -EINVAL;
@@ -461,23 +486,24 @@ static int vidioc_try_crop(struct file *file, void *prv,
const struct v4l2_crop
return 0;
}
-static int vidioc_s_crop(struct file *file, void *prv, const struct v4l2_crop
*cr)
+static int vidioc_s_selection(struct file *file, void *prv,
+ struct v4l2_selection *s)
{
struct g2d_ctx *ctx = prv;
struct g2d_frame *f;
int
[RFC PATCH 11/11] vidioc_cropcap -> vidioc_g_pixelaspect
From: Hans Verkuil
Now vidioc_cropcap is only used to return the pixelaspect, so
rename it accordingly.
Signed-off-by: Hans Verkuil
---
drivers/media/pci/bt8xx/bttv-driver.c | 12 +--
drivers/media/pci/cobalt/cobalt-v4l2.c| 10 +
drivers/media/pci/cx18/cx18-ioctl.c | 13 ++--
drivers/media/pci/cx23885/cx23885-video.c | 12 +--
drivers/media/pci/ivtv/ivtv-ioctl.c | 17 ---
drivers/media/pci/saa7134/saa7134-video.c | 21 +--
drivers/media/platform/am437x/am437x-vpfe.c | 13 ++--
drivers/media/platform/davinci/vpbe_display.c | 10 -
drivers/media/platform/davinci/vpfe_capture.c | 12 +--
drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 -
drivers/media/platform/vivid/vivid-core.c | 9
drivers/media/platform/vivid/vivid-vid-cap.c | 18 +++-
drivers/media/platform/vivid/vivid-vid-cap.h | 2 +-
drivers/media/platform/vivid/vivid-vid-out.c | 18 +++-
drivers/media/platform/vivid/vivid-vid-out.h | 2 +-
drivers/media/usb/au0828/au0828-video.c | 12 +--
drivers/media/usb/cx231xx/cx231xx-417.c | 12 +--
drivers/media/usb/cx231xx/cx231xx-video.c | 12 +--
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 13 +++-
drivers/media/v4l2-core/v4l2-dev.c| 6 +++---
drivers/media/v4l2-core/v4l2-ioctl.c | 15 +++--
include/media/v4l2-ioctl.h| 8 +++
22 files changed, 131 insertions(+), 126 deletions(-)
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c
b/drivers/media/pci/bt8xx/bttv-driver.c
index b2cfcbb0008e..52cac1d3f577 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -2792,19 +2792,17 @@ static int bttv_g_tuner(struct file *file, void *priv,
return 0;
}
-static int bttv_cropcap(struct file *file, void *priv,
- struct v4l2_cropcap *cap)
+static int bttv_g_pixelaspect(struct file *file, void *priv,
+ int type, struct v4l2_fract *f)
{
struct bttv_fh *fh = priv;
struct bttv *btv = fh->btv;
- if (cap->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
- cap->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
+ if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
/* defrect and bounds are set via g_selection */
- cap->pixelaspect = bttv_tvnorms[btv->tvnorm].cropcap.pixelaspect;
-
+ *f = bttv_tvnorms[btv->tvnorm].cropcap.pixelaspect;
return 0;
}
@@ -3162,7 +3160,7 @@ static const struct v4l2_ioctl_ops bttv_ioctl_ops = {
.vidioc_g_fmt_vbi_cap = bttv_g_fmt_vbi_cap,
.vidioc_try_fmt_vbi_cap = bttv_try_fmt_vbi_cap,
.vidioc_s_fmt_vbi_cap = bttv_s_fmt_vbi_cap,
- .vidioc_cropcap = bttv_cropcap,
+ .vidioc_g_pixelaspect = bttv_g_pixelaspect,
.vidioc_reqbufs = bttv_reqbufs,
.vidioc_querybuf= bttv_querybuf,
.vidioc_qbuf= bttv_qbuf,
diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c
b/drivers/media/pci/cobalt/cobalt-v4l2.c
index 4a0205aae4b4..c088de551081 100644
--- a/drivers/media/pci/cobalt/cobalt-v4l2.c
+++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
@@ -1077,20 +1077,22 @@ static int cobalt_g_parm(struct file *file, void *fh,
struct v4l2_streamparm *a)
return 0;
}
-static int cobalt_cropcap(struct file *file, void *fh, struct v4l2_cropcap *cc)
+static int cobalt_g_pixelaspect(struct file *file, void *fh,
+ int type, struct v4l2_fract *f)
{
struct cobalt_stream *s = video_drvdata(file);
struct v4l2_dv_timings timings;
int err = 0;
- if (cc->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
+
if (s->input == 1)
timings = cea1080p60;
else
err = v4l2_subdev_call(s->sd, video, g_dv_timings, );
if (!err)
- cc->pixelaspect = v4l2_dv_timings_aspect_ratio();
+ *f = v4l2_dv_timings_aspect_ratio();
return err;
}
@@ -1132,7 +1134,7 @@ static const struct v4l2_ioctl_ops cobalt_ioctl_ops = {
.vidioc_log_status = cobalt_log_status,
.vidioc_streamon= vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,
- .vidioc_cropcap = cobalt_cropcap,
+ .vidioc_g_pixelaspect = cobalt_g_pixelaspect,
.vidioc_g_selection = cobalt_g_selection,
.vidioc_enum_input = cobalt_enum_input,
.vidioc_g_input = cobalt_g_input,
diff --git a/drivers/media/pci/cx18/cx18-ioctl.c
b/drivers/media/pci/cx18/cx18-ioctl.c
index
[RFC PATCH 01/11] v4l2-ioctl: don't use CROP/COMPOSE_ACTIVE
From: Hans Verkuil Drop the deprecated _ACTIVE part. Signed-off-by: Hans Verkuil --- drivers/media/v4l2-core/v4l2-ioctl.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 7de041bae84f..9c2370e4d05c 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -2212,9 +2212,9 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops, /* crop means compose for output devices */ if (V4L2_TYPE_IS_OUTPUT(p->type)) - s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE; + s.target = V4L2_SEL_TGT_COMPOSE; else - s.target = V4L2_SEL_TGT_CROP_ACTIVE; + s.target = V4L2_SEL_TGT_CROP; ret = v4l_g_selection(ops, file, fh, ); @@ -2239,9 +2239,9 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops, /* crop means compose for output devices */ if (V4L2_TYPE_IS_OUTPUT(p->type)) - s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE; + s.target = V4L2_SEL_TGT_COMPOSE; else - s.target = V4L2_SEL_TGT_CROP_ACTIVE; + s.target = V4L2_SEL_TGT_CROP; return v4l_s_selection(ops, file, fh, ); } -- 2.18.0
[RFC PATCH 10/11] v4l2-ioctl: remove unused vidioc_g/s_crop
From: Hans Verkuil
Now that all drivers have dropped vidioc_g/s_crop we can remove
support for them in the V4L2 core.
Signed-off-by: Hans Verkuil
---
drivers/media/v4l2-core/v4l2-dev.c | 4 ++--
drivers/media/v4l2-core/v4l2-ioctl.c | 4
include/media/v4l2-ioctl.h | 8
3 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-dev.c
b/drivers/media/v4l2-core/v4l2-dev.c
index 69e775930fc4..d81141d51faa 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -621,9 +621,9 @@ static void determine_valid_ioctls(struct video_device
*vdev)
SET_VALID_IOCTL(ops, VIDIOC_TRY_DECODER_CMD,
vidioc_try_decoder_cmd);
SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMESIZES,
vidioc_enum_framesizes);
SET_VALID_IOCTL(ops, VIDIOC_ENUM_FRAMEINTERVALS,
vidioc_enum_frameintervals);
- if (ops->vidioc_g_crop || ops->vidioc_g_selection)
+ if (ops->vidioc_g_selection)
set_bit(_IOC_NR(VIDIOC_G_CROP), valid_ioctls);
- if (ops->vidioc_s_crop || ops->vidioc_s_selection)
+ if (ops->vidioc_s_selection)
set_bit(_IOC_NR(VIDIOC_S_CROP), valid_ioctls);
SET_VALID_IOCTL(ops, VIDIOC_G_SELECTION, vidioc_g_selection);
SET_VALID_IOCTL(ops, VIDIOC_S_SELECTION, vidioc_s_selection);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 63a92285de39..a59954d351a2 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2207,8 +2207,6 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
};
int ret;
- if (ops->vidioc_g_crop)
- return ops->vidioc_g_crop(file, fh, p);
/* simulate capture crop using selection api */
/* crop means compose for output devices */
@@ -2239,8 +2237,6 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
.r = p->c,
};
- if (ops->vidioc_s_crop)
- return ops->vidioc_s_crop(file, fh, p);
/* simulate capture crop using selection api */
/* crop means compose for output devices */
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 5848d92c30da..85fdd3f4b8ad 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -222,10 +222,6 @@ struct v4l2_fh;
* :ref:`VIDIOC_S_MODULATOR ` ioctl
* @vidioc_cropcap: pointer to the function that implements
* :ref:`VIDIOC_CROPCAP ` ioctl
- * @vidioc_g_crop: pointer to the function that implements
- * :ref:`VIDIOC_G_CROP ` ioctl
- * @vidioc_s_crop: pointer to the function that implements
- * :ref:`VIDIOC_S_CROP ` ioctl
* @vidioc_g_selection: pointer to the function that implements
* :ref:`VIDIOC_G_SELECTION ` ioctl
* @vidioc_s_selection: pointer to the function that implements
@@ -493,10 +489,6 @@ struct v4l2_ioctl_ops {
/* Crop ioctls */
int (*vidioc_cropcap)(struct file *file, void *fh,
struct v4l2_cropcap *a);
- int (*vidioc_g_crop)(struct file *file, void *fh,
-struct v4l2_crop *a);
- int (*vidioc_s_crop)(struct file *file, void *fh,
-const struct v4l2_crop *a);
int (*vidioc_g_selection)(struct file *file, void *fh,
struct v4l2_selection *s);
int (*vidioc_s_selection)(struct file *file, void *fh,
--
2.18.0
Re: [PATCH] media: cec: name for RC passthrough device does not need 'RC for'
On 10/05/2018 12:21 AM, Sean Young wrote:
> An RC device is does not need to be called 'RC for'. Simply the name
> will suffice.
>
> Signed-off-by: Sean Young
Reviewed-by: Hans Verkuil
OK if I take this patch? I have a cec pull request upcoming anyway.
Regards,
Hans
> ---
> drivers/media/cec/cec-core.c | 6 ++
> include/media/cec.h | 2 --
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 74596f089ec9..e4edc930d4ed 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -307,12 +307,10 @@ struct cec_adapter *cec_allocate_adapter(const struct
> cec_adap_ops *ops,
> return ERR_PTR(-ENOMEM);
> }
>
> - snprintf(adap->device_name, sizeof(adap->device_name),
> - "RC for %s", name);
> snprintf(adap->input_phys, sizeof(adap->input_phys),
> - "%s/input0", name);
> + "%s/input0", adap->name);
>
> - adap->rc->device_name = adap->device_name;
> + adap->rc->device_name = adap->name;
> adap->rc->input_phys = adap->input_phys;
> adap->rc->input_id.bustype = BUS_CEC;
> adap->rc->input_id.vendor = 0;
> diff --git a/include/media/cec.h b/include/media/cec.h
> index 9f382f0c2970..73ed28b076ce 100644
> --- a/include/media/cec.h
> +++ b/include/media/cec.h
> @@ -198,9 +198,7 @@ struct cec_adapter {
> u16 phys_addrs[15];
> u32 sequence;
>
> - char device_name[32];
> char input_phys[32];
> - char input_drv[32];
> };
>
> static inline void *cec_get_drvdata(const struct cec_adapter *adap)
>
Re: s5p_mfc and H.264 frame cropping question
On 10/05/2018 05:12 AM, Tomasz Figa wrote: > Hi Hans, > > On Fri, Oct 5, 2018 at 5:02 AM Hans Verkuil wrote: >> >> Hi all, >> >> I'm looking at removing the last users of vidioc_g/s_crop from the driver and >> I came across vidioc_g_crop in drivers/media/platform/s5p-mfc/s5p_mfc_dec.c. >> >> What this really does AFAICS is return the H.264 frame crop as read from the >> bitstream. This has nothing to do with regular cropping/composing but it >> might be >> something that could be implemented as a new selection target. > > It has a lot to do, because the output frame buffer may contain (and > on the hardware I worked with, s5p-mfc and mtk-vcodec, indeed does) > the whole encoded stream and the frame crop from the bitstream > specifies the rectangle within it that corresponds to the part that > should be displayed. Yes, but is that part actually cropped? Or is the full uncropped image DMAed to the capture buffer? To take a practical example: a H.264 stream with a 1920x1088 image and a frame crop rectangle of 1920x1080. What is the G_FMT width/height for the decoder capture stream: 1920x1088 or 1920x1080? If it is 1920x1088, then you have a compose rectangle. If it is 1920x1080 then you have a crop rectangle. As far as I can tell from this driver it actually has a compose rectangle and the use of g_crop is wrong and is there due to historical reasons (the driver predates the selection API). > >> >> I'm not really sure what to do with the existing code since it is an abuse of >> the crop API, but I guess the first step is to decide how this should be >> handled >> properly. >> >> Are there other decoders that can retrieve this information? Should this be >> mentioned in the stateful codec API? > > coda [1], mtk-vcodec [2] and venus [3] expose this using the > V4L2_SEL_TGT_COMPOSE selection target. v1 of the specification defines > the selection targets in a way, which is compatible with that: > V4L2_SEL_TGT_COMPOSE defaults to V4L2_SEL_TGT_COMPOSE_DEFAULT, which > equals to V4L2_SEL_TGT_CROP, which defaults to > V4L2_SEL_TGT_CROP_DEFAULT, which is defined as follows: > > + ``V4L2_SEL_TGT_CROP_DEFAULT`` > + a rectangle covering the part of the frame buffer that contains > + meaningful picture data (visible area); width and height will be > + equal to visible resolution of the stream Where do you get that from? That's the crop definition for an output stream, not a capture stream (assuming we have a codec). I kind of lost you with "which equals to V4L2_SEL_TGT_CROP". In any case, this particular driver should implement g_selection for CAPTURE and implement the COMPOSE targets. That makes sense. Regards, Hans > > AFAIR s5p-mfc was added before the selection API went into active use, > so there is some user space that relies on the crop API. We should > make the driver expose proper selection targets and add a comment next > to the crop API implementation explaining the historical reasons. The > specification should mention only the selection API. > > [1] > https://elixir.bootlin.com/linux/v4.19-rc6/source/drivers/media/platform/coda/coda-common.c#L892 > [2] > https://elixir.bootlin.com/linux/v4.19-rc6/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c#L740 > [3] > https://elixir.bootlin.com/linux/v4.19-rc6/source/drivers/media/platform/qcom/venus/vdec.c#L300 > > Best regards, > Tomasz >
