| Message ID | 20201112030557.8540-1-mirela.rabulea@oss.nxp.com |
|---|---|
| Headers | show |
| Series | Add V4L2 driver for i.MX8 JPEG Encoder/Decoder | expand |
Hi Mirela, On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > Add jpeg decoder/encoder nodes, for now on imx8qxp only. > The same should work on imx8qm, but it was not tested. > Does imx8qm need changes in the dt bindings? Unless you are aware of reasons preventing us from enabling it on imx8qm, then we could go for imx8qm as well (reusing imx8qxp- compatible). > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 8 ++++ > arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 37 +++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > index 46437d3c7a04..a0ad9789e9b8 100644 > --- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > +++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > @@ -270,3 +270,11 @@ > >; > }; > }; > + > +&jpegdec { > + status = "okay"; > +}; > + > +&jpegenc { > + status = "okay"; > +}; Please drop this. See below. > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > index e46faac1fe71..1d9a16388fa8 100644 > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > @@ -629,4 +629,41 @@ > }; > }; > }; > + > + img_subsys: bus@58000000 { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x58000000 0x0 0x58000000 0x1000000>; > + > + jpegdec: jpegdec@58400000 { > + compatible = "nxp,imx8qxp-jpgdec"; > + reg = <0x58400000 0x00050000 >; > + interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>; > + power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>, > + <&pd IMX_SC_R_MJPEG_DEC_S0>, > + <&pd IMX_SC_R_MJPEG_DEC_S1>, > + <&pd IMX_SC_R_MJPEG_DEC_S2>, > + <&pd IMX_SC_R_MJPEG_DEC_S3>; > + status = "disabled"; Pure memory-to-memory are typically not enabled per-board, but just per-platform. So you can drop the disabled status here. Thanks, Ezequiel > + }; > + > + jpegenc: jpegenc@58450000 { > + compatible = "nxp,imx8qxp-jpgenc"; > + reg = <0x58450000 0x00050000 >; > + interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>; > + power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>, > + <&pd IMX_SC_R_MJPEG_ENC_S0>, > + <&pd IMX_SC_R_MJPEG_ENC_S1>, > + <&pd IMX_SC_R_MJPEG_ENC_S2>, > + <&pd IMX_SC_R_MJPEG_ENC_S3>; > + status = "disabled"; > + }; > + }; > };
Hi Ezequiel, On Thu, 2020-11-12 at 05:36 -0300, Ezequiel Garcia wrote: > > Hi Mirela, > > On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > > > Add jpeg decoder/encoder nodes, for now on imx8qxp only. > > The same should work on imx8qm, but it was not tested. > > > > Does imx8qm need changes in the dt bindings? > > Unless you are aware of reasons preventing us from enabling > it on imx8qm, then we could go for imx8qm as well (reusing > imx8qxp- compatible). I think it will be possible to reuse the same compatile for 8qm, too. There is no dts for 8qm upstream for now, I understand Aisheng has something started on that. We'll see how it goes. > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > --- > > arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 8 ++++ > > arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 37 > > +++++++++++++++++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > > b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > > index 46437d3c7a04..a0ad9789e9b8 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts > > @@ -270,3 +270,11 @@ > > >; > > }; > > }; > > + > > +&jpegdec { > > + status = "okay"; > > +}; > > + > > +&jpegenc { > > + status = "okay"; > > +}; > > Please drop this. See below. Done for the next version. > > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > > index e46faac1fe71..1d9a16388fa8 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi > > @@ -629,4 +629,41 @@ > > }; > > }; > > }; > > + > > + img_subsys: bus@58000000 { > > + compatible = "simple-bus"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges = <0x58000000 0x0 0x58000000 0x1000000>; > > + > > + jpegdec: jpegdec@58400000 { > > + compatible = "nxp,imx8qxp-jpgdec"; > > + reg = <0x58400000 0x00050000 >; > > + interrupts = <GIC_SPI 309 > > IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 310 > > IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 311 > > IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 312 > > IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>, > > + <&pd IMX_SC_R_MJPEG_DEC_S0>, > > + <&pd IMX_SC_R_MJPEG_DEC_S1>, > > + <&pd IMX_SC_R_MJPEG_DEC_S2>, > > + <&pd IMX_SC_R_MJPEG_DEC_S3>; > > + status = "disabled"; > > Pure memory-to-memory are typically not enabled per-board, > but just per-platform. > > So you can drop the disabled status here. Done for the next version. Thanks, Mirela > > Thanks, > Ezequiel > > > + }; > > + > > + jpegenc: jpegenc@58450000 { > > + compatible = "nxp,imx8qxp-jpgenc"; > > + reg = <0x58450000 0x00050000 >; > > + interrupts = <GIC_SPI 305 > > IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 306 > > IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 307 > > IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 308 > > IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>, > > + <&pd IMX_SC_R_MJPEG_ENC_S0>, > > + <&pd IMX_SC_R_MJPEG_ENC_S1>, > > + <&pd IMX_SC_R_MJPEG_ENC_S2>, > > + <&pd IMX_SC_R_MJPEG_ENC_S3>; > > + status = "disabled"; > > + }; > > + }; > > }; > >
On Thu, Nov 12, 2020 at 05:05:49AM +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > The power domains are for imx8qxp/imx8qm JPEG encoder & decoder. > Each has 4 slots and a wrapper. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > Acked-by: Daniel Baluta <daniel.baluta@nxp.com> Applied, thanks.
Hi Philipp, any thoughts on the jpeg helpers related patches from this patch set (7,8,9,10)? Thanks, Mirela On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > According to Rec. ITU-T T.872 (06/2012) 6.5.3 > APP14 segment is for color encoding, it contains a transform flag, > which > may have values of 0, 1 and 2 and are interpreted as follows: > 0 - CMYK for images that are encoded with four components > - RGB for images that are encoded with three components > 1 - An image encoded with three components using YCbCr colour > encoding. > 2 - An image encoded with four components using YCCK colour encoding. > > This is used in imx-jpeg decoder, to distinguish between > YUV444 and RGB24. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > Changes in v5: > This was patch 8 in previous version > Replaced a struct for app14 data with just an int, since the > transform flag is the only meaningfull information from this segment > > drivers/media/v4l2-core/v4l2-jpeg.c | 39 > +++++++++++++++++++++++++++-- > include/media/v4l2-jpeg.h | 6 +++-- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c > b/drivers/media/v4l2-core/v4l2-jpeg.c > index 8947fd95c6f1..3181ce544f79 100644 > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL"); > #define DHP 0xffde /* hierarchical progression */ > #define EXP 0xffdf /* expand reference */ > #define APP0 0xffe0 /* application data */ > +#define APP14 0xffee /* application data for colour > encoding */ > #define APP15 0xffef > #define JPG0 0xfff0 /* extensions */ > #define JPG13 0xfffd > @@ -444,8 +445,37 @@ static int jpeg_skip_segment(struct jpeg_stream > *stream) > return jpeg_skip(stream, len - 2); > } > > +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */ > +static int jpeg_parse_app14_data(struct jpeg_stream *stream) > +{ > + int ret; > + int Lp; > + int skip; > + int tf; > + > + Lp = jpeg_get_word_be(stream); > + if (Lp < 0) > + return Lp; > + > + /* get to Ap12 */ > + ret = jpeg_skip(stream, 11); > + if (ret < 0) > + return -EINVAL; > + > + tf = jpeg_get_byte(stream); > + if (tf < 0) > + return tf; > + > + skip = Lp - 2 - 11; > + ret = jpeg_skip(stream, skip); > + if (ret < 0) > + return -EINVAL; > + else > + return tf; > +} > + > /** > - * jpeg_parse_header - locate marker segments and optionally parse > headers > + * v4l2_jpeg_parse_header - locate marker segments and optionally > parse headers > * @buf: address of the JPEG buffer, should start with a SOI marker > * @len: length of the JPEG buffer > * @out: returns marker segment positions and optionally parsed > headers > @@ -476,6 +506,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, > struct v4l2_jpeg_header *out) > if (marker != SOI) > return -EINVAL; > > + /* init value to signal if this marker is not present */ > + out->app14_tf = -EINVAL; > + > /* loop through marker segments */ > while ((marker = jpeg_next_marker(&stream)) >= 0) { > switch (marker) { > @@ -519,7 +552,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, > struct v4l2_jpeg_header *out) > ret = jpeg_parse_restart_interval(&stream, > &out- > >restart_interval); > break; > - > + case APP14: > + out->app14_tf = jpeg_parse_app14_data(&stream); > + break; > case SOS: > ret = jpeg_reference_segment(&stream, &out- > >sos); > if (ret < 0) > diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h > index ddba2a56c321..008e0476d928 100644 > --- a/include/media/v4l2-jpeg.h > +++ b/include/media/v4l2-jpeg.h > @@ -100,10 +100,11 @@ struct v4l2_jpeg_scan_header { > * order, optional > * @restart_interval: number of MCU per restart interval, Ri > * @ecs_offset: buffer offset in bytes to the entropy coded segment > + * @app14_tf: transform flag from app14 data > * > * When this structure is passed to v4l2_jpeg_parse_header, the > optional scan, > - * quantization_tables, and huffman_tables pointers must be > initialized to NULL > - * or point at valid memory. > + * quantization_tables and huffman_tables pointers must be > initialized > + * to NULL or point at valid memory. > */ > struct v4l2_jpeg_header { > struct v4l2_jpeg_reference sof; > @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { > struct v4l2_jpeg_reference *huffman_tables; > u16 restart_interval; > size_t ecs_offset; > + int app14_tf; > }; > > int v4l2_jpeg_parse_header(void *buf, size_t len, struct > v4l2_jpeg_header *out);
On 12/11/2020 04:05, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > These are optional in struct v4l2_jpeg_header, so do not parse if > not requested, save some time. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > index d77e04083d57..7576cd0ce6b9 100644 > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream, > { > int len = jpeg_get_word_be(stream); > > + if (!tables) > + return 0; > + It feels more natural to check for a non-NULL out->quantization_tables or non-NULL out->huffman_tables pointer in v4l2_jpeg_parse_header() rather than in these low-level functions. It's weird to have this check here. Regards, Hans > if (len < 0) > return len; > /* Lq = 2 + n * 65 (for baseline DCT), n >= 1 */ > @@ -361,6 +364,9 @@ static int jpeg_parse_huffman_tables(struct jpeg_stream *stream, > int mt; > int len = jpeg_get_word_be(stream); > > + if (!tables) > + return 0; > + > if (len < 0) > return len; > /* Table B.5 - Huffman table specification parameter sizes and values */ >
Hi Mirela, On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > According to Rec. ITU-T T.872 (06/2012) 6.5.3 > APP14 segment is for color encoding, it contains a transform flag, which > may have values of 0, 1 and 2 and are interpreted as follows: > 0 - CMYK for images that are encoded with four components > - RGB for images that are encoded with three components > 1 - An image encoded with three components using YCbCr colour encoding. > 2 - An image encoded with four components using YCCK colour encoding. > > This is used in imx-jpeg decoder, to distinguish between > YUV444 and RGB24. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > Changes in v5: > This was patch 8 in previous version > Replaced a struct for app14 data with just an int, since the > transform flag is the only meaningfull information from this segment Could we turn this into an enum for the transform flag, and include the above spec reference in its kerneldoc comment? I think this would be better than checking for (app14_tf == <magic_number>) in the drivers. > drivers/media/v4l2-core/v4l2-jpeg.c | 39 +++++++++++++++++++++++++++-- > include/media/v4l2-jpeg.h | 6 +++-- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > index 8947fd95c6f1..3181ce544f79 100644 > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL"); > #define DHP 0xffde /* hierarchical progression */ > #define EXP 0xffdf /* expand reference */ > #define APP0 0xffe0 /* application data */ > +#define APP14 0xffee /* application data for colour encoding */ > #define APP15 0xffef > #define JPG0 0xfff0 /* extensions */ > #define JPG13 0xfffd > @@ -444,8 +445,37 @@ static int jpeg_skip_segment(struct jpeg_stream *stream) > return jpeg_skip(stream, len - 2); > } > > +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */ > +static int jpeg_parse_app14_data(struct jpeg_stream *stream) > +{ > + int ret; > + int Lp; Let's keep variables lower case. > + int skip; > + int tf; > + > + Lp = jpeg_get_word_be(stream); > + if (Lp < 0) > + return Lp; Should we check that Ap1 to 6 are actually "Adobe\0"? > + /* get to Ap12 */ > + ret = jpeg_skip(stream, 11); > + if (ret < 0) > + return -EINVAL; > + > + tf = jpeg_get_byte(stream); > + if (tf < 0) > + return tf; > + > + skip = Lp - 2 - 11; > + ret = jpeg_skip(stream, skip); > + if (ret < 0) > + return -EINVAL; > + else > + return tf; > +} > + > /** > - * jpeg_parse_header - locate marker segments and optionally parse headers > + * v4l2_jpeg_parse_header - locate marker segments and optionally parse headers > * @buf: address of the JPEG buffer, should start with a SOI marker > * @len: length of the JPEG buffer > * @out: returns marker segment positions and optionally parsed headers > @@ -476,6 +506,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) > if (marker != SOI) > return -EINVAL; > > + /* init value to signal if this marker is not present */ > + out->app14_tf = -EINVAL; > + > /* loop through marker segments */ > while ((marker = jpeg_next_marker(&stream)) >= 0) { > switch (marker) { > @@ -519,7 +552,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) > ret = jpeg_parse_restart_interval(&stream, > &out->restart_interval); > break; > - > + case APP14: > + out->app14_tf = jpeg_parse_app14_data(&stream); > + break; > case SOS: > ret = jpeg_reference_segment(&stream, &out->sos); > if (ret < 0) > diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h > index ddba2a56c321..008e0476d928 100644 > --- a/include/media/v4l2-jpeg.h > +++ b/include/media/v4l2-jpeg.h > @@ -100,10 +100,11 @@ struct v4l2_jpeg_scan_header { > * order, optional > * @restart_interval: number of MCU per restart interval, Ri > * @ecs_offset: buffer offset in bytes to the entropy coded segment > + * @app14_tf: transform flag from app14 data > * > * When this structure is passed to v4l2_jpeg_parse_header, the optional scan, > - * quantization_tables, and huffman_tables pointers must be initialized to NULL > - * or point at valid memory. > + * quantization_tables and huffman_tables pointers must be initialized > + * to NULL or point at valid memory. Unnecessary, unrelated change? I'd drop this. > */ > struct v4l2_jpeg_header { > struct v4l2_jpeg_reference sof; > @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { > struct v4l2_jpeg_reference *huffman_tables; > u16 restart_interval; > size_t ecs_offset; > + int app14_tf; > }; > > int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out); regards Philipp
On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > In the case we get an invalid stream, such as from v4l2-compliance > streaming test, jpeg_next_marker will end up parsing the entire > stream. The standard describes the high level syntax of a jpeg > as starting with SOI, ending with EOI, so return error if the very > first 2 bytes are not SOI. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > drivers/media/v4l2-core/v4l2-jpeg.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > index 3181ce544f79..d77e04083d57 100644 > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > @@ -499,11 +499,8 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct v4l2_jpeg_header *out) > out->num_dht = 0; > out->num_dqt = 0; > > - /* the first marker must be SOI */ > - marker = jpeg_next_marker(&stream); > - if (marker < 0) > - return marker; > - if (marker != SOI) > + /* the first bytes must be SOI, B.2.1 High-level syntax */ > + if (jpeg_get_word_be(&stream) != SOI) > return -EINVAL; > > /* init value to signal if this marker is not present */ Yes, shorter, potentially faster code, and it adheres to the specification more strictly. Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
Hi Mirela, On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > These are optional in struct v4l2_jpeg_header, so do not parse if > not requested, save some time. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > index d77e04083d57..7576cd0ce6b9 100644 > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream, > { > int len = jpeg_get_word_be(stream); > > + if (!tables) > + return 0; > + > if (len < 0) > return len; > /* Lq = 2 + n * 65 (for baseline DCT), n >= 1 */ > @@ -361,6 +364,9 @@ static int jpeg_parse_huffman_tables(struct jpeg_stream *stream, > int mt; > int len = jpeg_get_word_be(stream); > > + if (!tables) > + return 0; > + > if (len < 0) > return len; > /* Table B.5 - Huffman table specification parameter sizes and values */ I don't understand this. jpeg_parse_quantization_tables() is only called if v4l2_jpeg_parse_header() finds a DQT marker. The entire quantization table-specification parameter block is optional, but when a DQT marker is present, IIUC the block must contain at least one table (see B.2.4.1, specifically figure B.6). regards Philipp
On Wed, 2020-12-02 at 13:12 +0100, Hans Verkuil wrote: > On 12/11/2020 04:05, Mirela Rabulea (OSS) wrote: > > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > > > These are optional in struct v4l2_jpeg_header, so do not parse if > > not requested, save some time. > > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > --- > > drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c > > index d77e04083d57..7576cd0ce6b9 100644 > > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > > @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct jpeg_stream *stream, > > { > > int len = jpeg_get_word_be(stream); > > > > + if (!tables) > > + return 0; > > + > > It feels more natural to check for a non-NULL out->quantization_tables > or non-NULL out->huffman_tables pointer in v4l2_jpeg_parse_header() > rather than in these low-level functions. It's weird to have this check here. Ah, now I get it. Yes, if you want to skip the entire DQT for performance reasons, it is probably better to just call jpeg_skip_segment() instead of jpeg_parse_quantization_table(). Otherwise the next jpeg_next_marker has to scan the whole quantization table for segment markers. regards Philipp
On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > Use v4l2_jpeg_parse_header in mxc_jpeg_parse, remove the old > parsing way, which was duplicated in other jpeg drivers. > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > Changes in v5: > This was patch 11 in previous version > Change triggered by patch 7 (app14 data change struct -> int) > > drivers/media/platform/imx-jpeg/Kconfig | 1 + > drivers/media/platform/imx-jpeg/mxc-jpeg.c | 267 ++++++--------------- > drivers/media/platform/imx-jpeg/mxc-jpeg.h | 26 +- > 3 files changed, 80 insertions(+), 214 deletions(-) > > diff --git a/drivers/media/platform/imx-jpeg/Kconfig b/drivers/media/platform/imx-jpeg/Kconfig > index 7cc89e5eff90..d875f7c88cda 100644 > --- a/drivers/media/platform/imx-jpeg/Kconfig > +++ b/drivers/media/platform/imx-jpeg/Kconfig > @@ -4,6 +4,7 @@ config VIDEO_IMX8_JPEG > depends on VIDEO_DEV && VIDEO_V4L2 > select VIDEOBUF2_DMA_CONTIG > select V4L2_MEM2MEM_DEV > + select V4L2_JPEG_HELPER > default m > help > This is a video4linux2 driver for the i.MX8 QXP/QM integrated > diff --git a/drivers/media/platform/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/imx-jpeg/mxc-jpeg.c > index 8f297803f2c3..d3b7581ce46e 100644 > --- a/drivers/media/platform/imx-jpeg/mxc-jpeg.c > +++ b/drivers/media/platform/imx-jpeg/mxc-jpeg.c [...] > @@ -1448,12 +1317,11 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, > * encoded with 3 components have RGB colorspace, see Recommendation > * ITU-T T.872 chapter 6.5.3 APP14 marker segment for colour encoding > */ > - if (img_fmt == MXC_JPEG_YUV444 && app14 && app14_transform == 0) > - img_fmt = MXC_JPEG_RGB; > - > - if (mxc_jpeg_imgfmt_to_fourcc(img_fmt, &fourcc)) { > - dev_err(dev, "Fourcc not found for %d", img_fmt); > - return -EINVAL; > + if (fourcc == V4L2_PIX_FMT_YUV24 || fourcc == V4L2_PIX_FMT_RGB24) { > + if (header.app14_tf == 0) This is what I meant in patch 7, I think it would be more clear to have an enum value that says "RGB color coding" than to rely on the reader to know what the value 0 means. > + fourcc = V4L2_PIX_FMT_RGB24; > + else > + fourcc = V4L2_PIX_FMT_YUV24; > } > > /* > Otherwise this patch looks fine to me. regards Philipp
Hi Phipipp, On Wed, 2020-12-02 at 16:18 +0100, Philipp Zabel wrote: > > Hi Mirela, > > On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > > > According to Rec. ITU-T T.872 (06/2012) 6.5.3 > > APP14 segment is for color encoding, it contains a transform flag, > > which > > may have values of 0, 1 and 2 and are interpreted as follows: > > 0 - CMYK for images that are encoded with four components > > - RGB for images that are encoded with three components > > 1 - An image encoded with three components using YCbCr colour > > encoding. > > 2 - An image encoded with four components using YCCK colour > > encoding. > > > > This is used in imx-jpeg decoder, to distinguish between > > YUV444 and RGB24. > > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > --- > > Changes in v5: > > This was patch 8 in previous version > > Replaced a struct for app14 data with just an int, since the > > transform flag is the only meaningfull information from this > > segment > > Could we turn this into an enum for the transform flag, and include > the > above spec reference in its kerneldoc comment? I think this would be > better than checking for (app14_tf == <magic_number>) in the drivers. Appreciate your feedback, for all patches, I'll address it in v6. Where would be a better place for this enum, v4l2-jpeg.h, or maybe include/uapi/linux/v4l2-controls.h? Thanks, Mirela > > > drivers/media/v4l2-core/v4l2-jpeg.c | 39 > > +++++++++++++++++++++++++++-- > > include/media/v4l2-jpeg.h | 6 +++-- > > 2 files changed, 41 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c > > b/drivers/media/v4l2-core/v4l2-jpeg.c > > index 8947fd95c6f1..3181ce544f79 100644 > > --- a/drivers/media/v4l2-core/v4l2-jpeg.c > > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c > > @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL"); > > #define DHP 0xffde /* hierarchical progression */ > > #define EXP 0xffdf /* expand reference */ > > #define APP0 0xffe0 /* application data */ > > +#define APP14 0xffee /* application data for colour > > encoding */ > > #define APP15 0xffef > > #define JPG0 0xfff0 /* extensions */ > > #define JPG13 0xfffd > > @@ -444,8 +445,37 @@ static int jpeg_skip_segment(struct > > jpeg_stream *stream) > > return jpeg_skip(stream, len - 2); > > } > > > > +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */ > > +static int jpeg_parse_app14_data(struct jpeg_stream *stream) > > +{ > > + int ret; > > + int Lp; > > Let's keep variables lower case. > > > + int skip; > > + int tf; > > + > > + Lp = jpeg_get_word_be(stream); > > + if (Lp < 0) > > + return Lp; > > Should we check that Ap1 to 6 are actually "Adobe\0"? > > > + /* get to Ap12 */ > > + ret = jpeg_skip(stream, 11); > > + if (ret < 0) > > + return -EINVAL; > > + > > + tf = jpeg_get_byte(stream); > > + if (tf < 0) > > + return tf; > > + > > + skip = Lp - 2 - 11; > > + ret = jpeg_skip(stream, skip); > > + if (ret < 0) > > + return -EINVAL; > > + else > > + return tf; > > +} > > + > > /** > > - * jpeg_parse_header - locate marker segments and optionally parse > > headers > > + * v4l2_jpeg_parse_header - locate marker segments and optionally > > parse headers > > * @buf: address of the JPEG buffer, should start with a SOI > > marker > > * @len: length of the JPEG buffer > > * @out: returns marker segment positions and optionally parsed > > headers > > @@ -476,6 +506,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t > > len, struct v4l2_jpeg_header *out) > > if (marker != SOI) > > return -EINVAL; > > > > + /* init value to signal if this marker is not present */ > > + out->app14_tf = -EINVAL; > > + > > /* loop through marker segments */ > > while ((marker = jpeg_next_marker(&stream)) >= 0) { > > switch (marker) { > > @@ -519,7 +552,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t > > len, struct v4l2_jpeg_header *out) > > ret = jpeg_parse_restart_interval(&stream, > > &out- > > >restart_interval); > > break; > > - > > + case APP14: > > + out->app14_tf = > > jpeg_parse_app14_data(&stream); > > + break; > > case SOS: > > ret = jpeg_reference_segment(&stream, &out- > > >sos); > > if (ret < 0) > > diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h > > index ddba2a56c321..008e0476d928 100644 > > --- a/include/media/v4l2-jpeg.h > > +++ b/include/media/v4l2-jpeg.h > > @@ -100,10 +100,11 @@ struct v4l2_jpeg_scan_header { > > * order, optional > > * @restart_interval: number of MCU per restart interval, Ri > > * @ecs_offset: buffer offset in bytes to the entropy coded > > segment > > + * @app14_tf: transform flag from app14 data > > * > > * When this structure is passed to v4l2_jpeg_parse_header, the > > optional scan, > > - * quantization_tables, and huffman_tables pointers must be > > initialized to NULL > > - * or point at valid memory. > > + * quantization_tables and huffman_tables pointers must be > > initialized > > + * to NULL or point at valid memory. > > Unnecessary, unrelated change? I'd drop this. > > > */ > > struct v4l2_jpeg_header { > > struct v4l2_jpeg_reference sof; > > @@ -119,6 +120,7 @@ struct v4l2_jpeg_header { > > struct v4l2_jpeg_reference *huffman_tables; > > u16 restart_interval; > > size_t ecs_offset; > > + int app14_tf; > > }; > > > > int v4l2_jpeg_parse_header(void *buf, size_t len, struct > > v4l2_jpeg_header *out); > > regards > Philipp
On Fri, 2020-12-04 at 14:13 +0000, Mirela Rabulea (OSS) wrote: > Hi Phipipp, > > On Wed, 2020-12-02 at 16:18 +0100, Philipp Zabel wrote: > > Hi Mirela, > > > > On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote: > > > From: Mirela Rabulea <mirela.rabulea@nxp.com> > > > > > > According to Rec. ITU-T T.872 (06/2012) 6.5.3 > > > APP14 segment is for color encoding, it contains a transform flag, > > > which > > > may have values of 0, 1 and 2 and are interpreted as follows: > > > 0 - CMYK for images that are encoded with four components > > > - RGB for images that are encoded with three components > > > 1 - An image encoded with three components using YCbCr colour > > > encoding. > > > 2 - An image encoded with four components using YCCK colour > > > encoding. > > > > > > This is used in imx-jpeg decoder, to distinguish between > > > YUV444 and RGB24. > > > > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > > --- > > > Changes in v5: > > > This was patch 8 in previous version > > > Replaced a struct for app14 data with just an int, since the > > > transform flag is the only meaningfull information from this > > > segment > > > > Could we turn this into an enum for the transform flag, and include > > the > > above spec reference in its kerneldoc comment? I think this would be > > better than checking for (app14_tf == <magic_number>) in the drivers. > > Appreciate your feedback, for all patches, I'll address it in v6. > Where would be a better place for this enum, v4l2-jpeg.h, or maybe > include/uapi/linux/v4l2-controls.h? v4l2-jpeg.h seems like the right place to me. regards Philipp
From: Mirela Rabulea <mirela.rabulea@nxp.com> This patch set adds the V4L2 driver for i.MX8QXP/QM JPEG encoder/decoder and it's dependencies. The driver was tested on i.MX8QXP, using a unit test application and the v4l2-compliance tool, including the streaming tests for decoder & encoder. The output of latest v4l2-compliance on i.MX8QXP, decoder & encoder: root@imx8qxpmek:/unit_tests/JPEG# ./v4l2-compliance-master -d /dev/video0 -s v4l2-compliance 1.21.0-4679, 64 bits, 64-bit time_t v4l2-compliance SHA: 225c6c2a17be 2020-10-30 15:13:07 Compliance test for mxc-jpeg decode device /dev/video0: Driver Info: Driver name : mxc-jpeg decode Card type : mxc-jpeg decoder Bus info : platform:58400000.jpegdec Driver version : 5.10.0 Capabilities : 0x84204000 Video Memory-to-Memory Multiplanar Streaming Extended Pix Format Device Capabilities Device Caps : 0x04204000 Video Memory-to-Memory Multiplanar Streaming Extended Pix Format Detected JPEG Decoder Required ioctls: test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK (Not Supported) test blocking wait: OK Video Capture Multiplanar: Captured 58 buffers test MMAP (no poll): OK Video Capture Multiplanar: Captured 58 buffers test MMAP (select): OK Video Capture Multiplanar: Captured 58 buffers test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for mxc-jpeg decode device /dev/video0: 52, Succeeded: 52, Failed: 0, Warnings: 0 root@imx8qxpmek:/unit_tests/JPEG# ./v4l2-compliance-master -d /dev/video1 -s v4l2-compliance 1.21.0-4679, 64 bits, 64-bit time_t v4l2-compliance SHA: 225c6c2a17be 2020-10-30 15:13:07 Compliance test for mxc-jpeg decode device /dev/video1: Driver Info: Driver name : mxc-jpeg decode Card type : mxc-jpeg decoder Bus info : platform:58450000.jpegenc Driver version : 5.10.0 Capabilities : 0x84204000 Video Memory-to-Memory Multiplanar Streaming Extended Pix Format Device Capabilities Device Caps : 0x04204000 Video Memory-to-Memory Multiplanar Streaming Extended Pix Format Detected JPEG Encoder Required ioctls: test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second /dev/video1 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Test input 0: Streaming ioctls: test read/write: OK (Not Supported) test blocking wait: OK Video Capture Multiplanar: Captured 58 buffers test MMAP (no poll): OK Video Capture Multiplanar: Captured 58 buffers test MMAP (select): OK Video Capture Multiplanar: Captured 58 buffers test MMAP (epoll): OK test USERPTR (no poll): OK (Not Supported) test USERPTR (select): OK (Not Supported) test DMABUF: Cannot test, specify --expbuf-device Total for mxc-jpeg decode device /dev/video1: 52, Succeeded: 52, Failed: 0, Warnings: 0 Mirela Rabulea (10): media: v4l: Add packed YUV444 24bpp pixel format firmware: imx: scu-pd: Add power domains for imx-jpeg media: dt-bindings: Add bindings for i.MX8QXP/QM JPEG driver media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder arm64: dts: imx8qxp: Add jpeg encoder/decoder nodes Add maintainer for IMX jpeg v4l2 driver media: Add parsing for APP14 data segment in jpeg helpers media: Quit parsing stream if doesn't start with SOI media: Avoid parsing quantization and huffman tables media: imx-jpeg: Use v4l2 jpeg helpers in mxc-jpeg .../bindings/media/nxp,imx8-jpeg.yaml | 84 + .../media/v4l/pixfmt-packed-yuv.rst | 37 +- MAINTAINERS | 8 + arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 8 + arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 37 + drivers/firmware/imx/scu-pd.c | 6 + drivers/media/platform/Kconfig | 2 + drivers/media/platform/Makefile | 1 + drivers/media/platform/imx-jpeg/Kconfig | 11 + drivers/media/platform/imx-jpeg/Makefile | 3 + drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c | 168 ++ drivers/media/platform/imx-jpeg/mxc-jpeg-hw.h | 140 ++ drivers/media/platform/imx-jpeg/mxc-jpeg.c | 2193 +++++++++++++++++ drivers/media/platform/imx-jpeg/mxc-jpeg.h | 180 ++ drivers/media/v4l2-core/v4l2-ioctl.c | 1 + drivers/media/v4l2-core/v4l2-jpeg.c | 52 +- include/media/v4l2-jpeg.h | 6 +- include/uapi/linux/videodev2.h | 1 + 18 files changed, 2928 insertions(+), 10 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml create mode 100644 drivers/media/platform/imx-jpeg/Kconfig create mode 100644 drivers/media/platform/imx-jpeg/Makefile create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-hw.h create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.c create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.h