[PATCH 1/4] [media] pxa_camera: Add remaining Bayer 8 formats

[PATCH 1/4] [media] pxa_camera: Add remaining Bayer 8 formats

[Petr Cvek]

This patch adds Bayer 8 GBRG and RGGB support and move GRBG definition
close to BGGR (so all Bayer 8 variants are together). No other changes are
needed as the driver handles them as RAW data stream.

The RGGB variant was tested in a modified OV9640 driver.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/media/platform/pxa_camera.c | 40 +++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 6615f80fe059..5f007713417e 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -345,6 +345,36 @@ static const struct pxa_mbus_lookup mbus_fmt[] = {
 		.layout			= PXA_MBUS_LAYOUT_PACKED,
 	},
 }, {
+	.code = MEDIA_BUS_FMT_SGBRG8_1X8,
+	.fmt = {
+		.fourcc			= V4L2_PIX_FMT_SGBRG8,
+		.name			= "Bayer 8 GBRG",
+		.bits_per_sample	= 8,
+		.packing		= PXA_MBUS_PACKING_NONE,
+		.order			= PXA_MBUS_ORDER_LE,
+		.layout			= PXA_MBUS_LAYOUT_PACKED,
+	},
+}, {
+	.code = MEDIA_BUS_FMT_SGRBG8_1X8,
+	.fmt = {
+		.fourcc			= V4L2_PIX_FMT_SGRBG8,
+		.name			= "Bayer 8 GRBG",
+		.bits_per_sample	= 8,
+		.packing		= PXA_MBUS_PACKING_NONE,
+		.order			= PXA_MBUS_ORDER_LE,
+		.layout			= PXA_MBUS_LAYOUT_PACKED,
+	},
+}, {
+	.code = MEDIA_BUS_FMT_SRGGB8_1X8,
+	.fmt = {
+		.fourcc			= V4L2_PIX_FMT_SRGGB8,
+		.name			= "Bayer 8 RGGB",
+		.bits_per_sample	= 8,
+		.packing		= PXA_MBUS_PACKING_NONE,
+		.order			= PXA_MBUS_ORDER_LE,
+		.layout			= PXA_MBUS_LAYOUT_PACKED,
+	},
+}, {
 	.code = MEDIA_BUS_FMT_SBGGR10_1X10,
 	.fmt = {
 		.fourcc			= V4L2_PIX_FMT_SBGGR10,
@@ -445,16 +475,6 @@ static const struct pxa_mbus_lookup mbus_fmt[] = {
 		.layout			= PXA_MBUS_LAYOUT_PACKED,
 	},
 }, {
-	.code = MEDIA_BUS_FMT_SGRBG8_1X8,
-	.fmt = {
-		.fourcc			= V4L2_PIX_FMT_SGRBG8,
-		.name			= "Bayer 8 GRBG",
-		.bits_per_sample	= 8,
-		.packing		= PXA_MBUS_PACKING_NONE,
-		.order			= PXA_MBUS_ORDER_LE,
-		.layout			= PXA_MBUS_LAYOUT_PACKED,
-	},
-}, {
 	.code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8,
 	.fmt = {
 		.fourcc			= V4L2_PIX_FMT_SGRBG10DPCM8,
-- 
2.11.0

Re: [PATCH 1/4] [media] pxa_camera: Add remaining Bayer 8 formats

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> This patch adds Bayer 8 GBRG and RGGB support and move GRBG definition
> close to BGGR (so all Bayer 8 variants are together). No other changes are
> needed as the driver handles them as RAW data stream.
>
> The RGGB variant was tested in a modified OV9640 driver.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

[PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation

[Petr Cvek]

During the transfer from the soc_camera a test in pxa_mbus_image_size()
got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either
the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles
the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and
PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg
(for some of the YUYV subvariants).

This patch re-adds the same test as in soc_camera version.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/media/platform/pxa_camera.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 5f007713417e..f71e7e0a652b 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -575,6 +575,9 @@ static s32 pxa_mbus_bytes_per_line(u32 width, const struct pxa_mbus_pixelfmt *mf
 static s32 pxa_mbus_image_size(const struct pxa_mbus_pixelfmt *mf,
 			u32 bytes_per_line, u32 height)
 {
+	if (mf->layout == PXA_MBUS_LAYOUT_PACKED)
+		return bytes_per_line * height;
+
 	switch (mf->packing) {
 	case PXA_MBUS_PACKING_2X8_PADHI:
 		return bytes_per_line * height * 2;
-- 
2.11.0

Re: [PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> During the transfer from the soc_camera a test in pxa_mbus_image_size()
> got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either
> the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles
> the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and
> PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg
> (for some of the YUYV subvariants).
>
> This patch re-adds the same test as in soc_camera version.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
Did you test that with YUV422P format ?
If yes, then you can have my ack.

And you should add Hans to the reviewers list, it's his call ultimately, and his
tree which should carry it on.

Cheers.

--
Robert

Re: [PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation

[Petr Cvek]

Dne 2.5.2017 v 16:43 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> During the transfer from the soc_camera a test in pxa_mbus_image_size()
>> got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either
>> the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles
>> the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and
>> PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg
>> (for some of the YUYV subvariants).
>>
>> This patch re-adds the same test as in soc_camera version.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> Did you test that with YUV422P format ?
> If yes, then you can have my ack.

pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=320x240:50323234)

And mplayer to framebuffer "somewhat" works (it timeouts after some time but it does regardless on format, ffmpeg is fine).

Anyway the patch does not affect V4L2_PIX_FMT_YUV422P in any way as the .layout field is PXA_MBUS_LAYOUT_PLANAR_2Y_U_V and test is only for "== PXA_MBUS_LAYOUT_PACKED"

> 
> And you should add Hans to the reviewers list, it's his call ultimately, and his
> tree which should carry it on.
> 
> Cheers.
> 
> --
> Robert
> 

Petr

Re: [PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation

[Petr Cvek]

Dne 2.5.2017 v 16:43 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> During the transfer from the soc_camera a test in pxa_mbus_image_size()
>> got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either
>> the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles
>> the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and
>> PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg
>> (for some of the YUYV subvariants).
>>
>> This patch re-adds the same test as in soc_camera version.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> Did you test that with YUV422P format ?
> If yes, then you can have my ack.
> 

I was trying to add RGB888 and then I've noticed that "YVYU 16bit" formats (and permutation) and "Bayer 12" formats have incorrect values in the bits_per_sample field. I didn't notice it in the patch creation, because it doesn't affect the computations of the image size. And later in the code it just defaults to 8.

A comment for a switch in the pxa_camera_setup_cicr() function:

	"Datawidth is now guaranteed to be equal to one of the three values..."

Values are 10, 9 and 8 and they describe a bit-vector length of the interface for a sensor.

So I will include a fix for the patchset v2 for this. I will test the YUYV formats, but my sensor does not support the Bayer 12 formats. 

In the addition I propose a patch for an enum type (or define) of the interface configuration field. Something like:
	#define PXA_MBUS_BUSWIDTH_8	8
	#define PXA_MBUS_BUSWIDTH_9	9
	#define PXA_MBUS_BUSWIDTH_10	10
and a patch for something like:
	s/bits_per_sample/buswidth/g
and a formatting patch for excessive tabulators in the mbus_fmt structure initialization ;-).

Petr

[PATCH 3/4] [media] pxa_camera: Add (un)subscribe_event ioctl

[Petr Cvek]

The v4l2-compliance complains about nonexistent vidioc_subscribe_event
and vidioc_unsubscribe_event calls. Add them to fix the complaints.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/media/platform/pxa_camera.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index f71e7e0a652b..79fd7269d1e6 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -37,7 +37,9 @@
 #include <media/v4l2-async.h>
 #include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-of.h>
 
@@ -2089,6 +2091,8 @@ static const struct v4l2_ioctl_ops pxa_camera_ioctl_ops = {
 	.vidioc_g_register		= pxac_vidioc_g_register,
 	.vidioc_s_register		= pxac_vidioc_s_register,
 #endif
+	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
+	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
 };
 
 static struct v4l2_clk_ops pxa_camera_mclk_ops = {
-- 
2.11.0

Re: [PATCH 3/4] [media] pxa_camera: Add (un)subscribe_event ioctl

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> The v4l2-compliance complains about nonexistent vidioc_subscribe_event
> and vidioc_unsubscribe_event calls. Add them to fix the complaints.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
Don't know on that one, let others on the mailing list comment, I'm not familiar
with the event interface of v4l2.

Cheers.

--
Robert

> ---
>  drivers/media/platform/pxa_camera.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index f71e7e0a652b..79fd7269d1e6 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -37,7 +37,9 @@
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-clk.h>
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-of.h>
>  
> @@ -2089,6 +2091,8 @@ static const struct v4l2_ioctl_ops pxa_camera_ioctl_ops = {
>  	.vidioc_g_register		= pxac_vidioc_g_register,
>  	.vidioc_s_register		= pxac_vidioc_s_register,
>  #endif
> +	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
> +	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
>  };
>  
>  static struct v4l2_clk_ops pxa_camera_mclk_ops = {

[PATCH 4/4] [media] pxa_camera: Fix a call with an uninitialized device pointer

[Petr Cvek]

In 'commit 295ab497d6357 ("[media] media: platform: pxa_camera: make
printk consistent")' a pointer to the device structure in
mclk_get_divisor() was changed to pcdev_to_dev(pcdev). The pointer used
by pcdev_to_dev() is still uninitialized during the call to
mclk_get_divisor() as it happens in v4l2_device_register() at the end
of the probe. The dev_warn and dev_dbg caused a line in the log:

	(NULL device *): Limiting master clock to 26000000

Fix this by using an initialized pointer from the platform_device
(as before the old patch).

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/media/platform/pxa_camera.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 79fd7269d1e6..c8466c63be22 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -1124,7 +1124,7 @@ static u32 mclk_get_divisor(struct platform_device *pdev,
 	/* mclk <= ciclk / 4 (27.4.2) */
 	if (mclk > lcdclk / 4) {
 		mclk = lcdclk / 4;
-		dev_warn(pcdev_to_dev(pcdev),
+		dev_warn(&pdev->dev,
 			 "Limiting master clock to %lu\n", mclk);
 	}
 
@@ -1135,7 +1135,7 @@ static u32 mclk_get_divisor(struct platform_device *pdev,
 	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
 		pcdev->mclk = lcdclk / (2 * (div + 1));
 
-	dev_dbg(pcdev_to_dev(pcdev), "LCD clock %luHz, target freq %luHz, divisor %u\n",
+	dev_dbg(&pdev->dev, "LCD clock %luHz, target freq %luHz, divisor %u\n",
 		lcdclk, mclk, div);
 
 	return div;
-- 
2.11.0

Re: [PATCH 4/4] [media] pxa_camera: Fix a call with an uninitialized device pointer

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> In 'commit 295ab497d6357 ("[media] media: platform: pxa_camera: make
> printk consistent")' a pointer to the device structure in
> mclk_get_divisor() was changed to pcdev_to_dev(pcdev). The pointer used
> by pcdev_to_dev() is still uninitialized during the call to
> mclk_get_divisor() as it happens in v4l2_device_register() at the end
> of the probe. The dev_warn and dev_dbg caused a line in the log:
>
> 	(NULL device *): Limiting master clock to 26000000
>
> Fix this by using an initialized pointer from the platform_device
> (as before the old patch).
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
Right, would be good to add to the commit message :
Fixes: 295ab497d635 ("[media] media: platform: pxa_camera: make printk consistent")

And :
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

> ---
>  drivers/media/platform/pxa_camera.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index 79fd7269d1e6..c8466c63be22 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -1124,7 +1124,7 @@ static u32 mclk_get_divisor(struct platform_device *pdev,
>  	/* mclk <= ciclk / 4 (27.4.2) */
>  	if (mclk > lcdclk / 4) {
>  		mclk = lcdclk / 4;
> -		dev_warn(pcdev_to_dev(pcdev),
> +		dev_warn(&pdev->dev,
>  			 "Limiting master clock to %lu\n", mclk);
>  	}
>  
> @@ -1135,7 +1135,7 @@ static u32 mclk_get_divisor(struct platform_device *pdev,
>  	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
>  		pcdev->mclk = lcdclk / (2 * (div + 1));
>  
> -	dev_dbg(pcdev_to_dev(pcdev), "LCD clock %luHz, target freq %luHz, divisor %u\n",
> +	dev_dbg(&pdev->dev, "LCD clock %luHz, target freq %luHz, divisor %u\n",
>  		lcdclk, mclk, div);
>  
>  	return div;

Re: [PATCH 4/4] [media] pxa_camera: Fix a call with an uninitialized device pointer

[Petr Cvek]

Dne 2.5.2017 v 16:53 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> In 'commit 295ab497d6357 ("[media] media: platform: pxa_camera: make
>> printk consistent")' a pointer to the device structure in
>> mclk_get_divisor() was changed to pcdev_to_dev(pcdev). The pointer used
>> by pcdev_to_dev() is still uninitialized during the call to
>> mclk_get_divisor() as it happens in v4l2_device_register() at the end
>> of the probe. The dev_warn and dev_dbg caused a line in the log:
>>
>> 	(NULL device *): Limiting master clock to 26000000
>>
>> Fix this by using an initialized pointer from the platform_device
>> (as before the old patch).
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> Right, would be good to add to the commit message :
> Fixes: 295ab497d635 ("[media] media: platform: pxa_camera: make printk consistent")
> 

OK I will add it in v2.