Skip to contentSkip to conversation

3fdfedaaa7f243f3347084231c64f6c1be0ba131 causes a regression

DISCUSSION Florian Vaussard 3 participants 10 messages Dec 17, 2013 – Jan 17, 2014 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Regards,

Florian

#1 lore raw

Re: Regression inside omap3isp/resizer (was: 3fdfeda causes a regression)

Hello Laurent,

Quoted 13 lines · Florian Vaussard

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

Quoted 5 lines

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

(I changed the topic to make it more clear)

Regards,

Florian

#2 lore raw

Re: Regression inside omap3isp/resizer (was: 3fdfeda causes a regression)

Hi Florian,

Sorry for the late reply.

Quoted 14 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 13 lines · Florian Vaussard

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

Quoted 7 lines
Quoted 4 lines

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Signature · 3 lines
Regards,

Laurent Pinchart
#3 lore raw

Re: Regression inside omap3isp/resizer

Hi Laurent,

Quoted 4 lines · Laurent Pinchart

Hi Florian,

Sorry for the late reply.

Now it is my turn to be late.

Quoted 20 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard
Quoted 17 lines

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Quoted 8 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

For example, with the current Gumstix Overo, most people are using the 3.6, which is the latest officially supported. I saw a number of them fighting to get a working pipeline. So when they will update to a 3.10+, their application will stop working correctly, and a big number of the users will have a hard time figuring out that they will have to update the pipeline's configuration to get back an image.

The result of meida-ctl -p in the same with and without the patch applied, as the same script is calling media-ctl at startup to configure the pipeline. Stripped-down and ordered (camera -> output) result is coming below. Obviously the configuration should be updated when the patch is applied.

Best regards,

Florian

- entity 16: mt9v032 2-005c (1 pad, 1 link) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev8
pad0: Output [SGRBG10 752x480 (1,5)/752x480]
-> 'OMAP3 ISP CCDC':pad0 [ACTIVE]

- entity 5: OMAP3 ISP CCDC (3 pads, 9 links)

            type V4L2 subdev subtype Unknown
            device node name /dev/v4l-subdev2
	pad0: Input [SGRBG10 752x480]
		<- 'mt9v032 2-005c':pad0 [ACTIVE]
	pad2: Output [SGRBG10 752x479]
		-> 'OMAP3 ISP preview':pad0 [ACTIVE]

- entity 7: OMAP3 ISP preview (2 pads, 4 links)

            type V4L2 subdev subtype Unknown
            device node name /dev/v4l-subdev3
	pad0: Input [SGRBG10 752x479 (10,4)/734x471]
		<- 'OMAP3 ISP CCDC':pad2 [ACTIVE]
	pad1: Output [UYVY 734x471]
		-> 'OMAP3 ISP resizer':pad0 [ACTIVE]

- entity 10: OMAP3 ISP resizer (2 pads, 4 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev4
pad0: Input [UYVY 734x471 (0,0)/734x471]
<- 'OMAP3 ISP preview':pad1 [ACTIVE]
pad1: Output [UYVY 752x480]
-> 'OMAP3 ISP resizer output':pad0 [ACTIVE]

- entity 12: OMAP3 ISP resizer output (1 pad, 1 link) type Node subtype V4L
device node name /dev/video6
pad0: Input
<- 'OMAP3 ISP resizer':pad1 [ACTIVE]

#4 lore raw

Re: Regression inside omap3isp/resizer

Hi Florian,

Quoted 35 lines · On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
Quoted 3 lines · Laurent Pinchart

Hi Florian,

Sorry for the late reply.

Now it is my turn to be late.

Quoted 20 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard
Quoted 17 lines

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Quoted 7 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Quoted 6 lines

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

But they shouldn't have. This has never been considered as an ABI. Userspace needs to computes and propagates resolutions through the pipeline dynamically, no hardcode them.

If your initialization script read the kernel version and aborted for any version other than v3.6, an upgrade to a newer kernel would break the system but you wouldn't call it a kernel regression :-)

Problems with pipeline configuration shouldn't result in distorted images though. The driver is supposed to refuse to start streaming when the pipeline is misconfigured by making sure that resolutions on connected source and sink pads are identical. A valid pipeline should not distort the image.

After a quick look at the code the problem we're dealing with seems to be different and shouldn't affect userspace scripts if solved properly. I haven't touched the preview engine crop configuration code for some time now, so I'll need to refresh my memory, but it seems that the removal of

- if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
- format->code != V4L2_MBUS_FMT_Y10_1X10) {

-               sph -= 2;
-               eph += 2;
-               slv -= 2;
-               elv += 2;

- }

was wrong. The change to the margins and to preview_try_crop() seem correct, but the preview_config_input_size() function should probably have been kept unmodified. Could you please test reverting that part of the patch only ?

Sakari, if you have time, could you please have a look at the code and give me your opinion ?

Quoted 52 lines

For example, with the current Gumstix Overo, most people are using the 3.6, which is the latest officially supported. I saw a number of them fighting to get a working pipeline. So when they will update to a 3.10+, their application will stop working correctly, and a big number of the users will have a hard time figuring out that they will have to update the pipeline's configuration to get back an image.

The result of meida-ctl -p in the same with and without the patch applied, as the same script is calling media-ctl at startup to configure the pipeline. Stripped-down and ordered (camera -> output) result is coming below. Obviously the configuration should be updated when the patch is applied.

Best regards,

Florian

- entity 16: mt9v032 2-005c (1 pad, 1 link) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev8
pad0: Output [SGRBG10 752x480 (1,5)/752x480]
-> 'OMAP3 ISP CCDC':pad0 [ACTIVE]

- entity 5: OMAP3 ISP CCDC (3 pads, 9 links)

            type V4L2 subdev subtype Unknown
            device node name /dev/v4l-subdev2
	pad0: Input [SGRBG10 752x480]
		<- 'mt9v032 2-005c':pad0 [ACTIVE]
	pad2: Output [SGRBG10 752x479]
		-> 'OMAP3 ISP preview':pad0 [ACTIVE]

- entity 7: OMAP3 ISP preview (2 pads, 4 links)

            type V4L2 subdev subtype Unknown
            device node name /dev/v4l-subdev3
	pad0: Input [SGRBG10 752x479 (10,4)/734x471]
		<- 'OMAP3 ISP CCDC':pad2 [ACTIVE]
	pad1: Output [UYVY 734x471]
		-> 'OMAP3 ISP resizer':pad0 [ACTIVE]

- entity 10: OMAP3 ISP resizer (2 pads, 4 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev4
pad0: Input [UYVY 734x471 (0,0)/734x471]
<- 'OMAP3 ISP preview':pad1 [ACTIVE]
pad1: Output [UYVY 752x480]
-> 'OMAP3 ISP resizer output':pad0 [ACTIVE]

- entity 12: OMAP3 ISP resizer output (1 pad, 1 link) type Node subtype V4L
device node name /dev/video6
pad0: Input
<- 'OMAP3 ISP resizer':pad1 [ACTIVE]

Signature · 3 lines
Regards,

Laurent Pinchart
#5 lore raw

Re: Regression inside omap3isp/resizer

Hi Laurent,

Quoted 45 lines · Laurent Pinchart

Hi Florian,

Quoted 35 lines · On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
Quoted 3 lines · Laurent Pinchart

Hi Florian,

Sorry for the late reply.

Now it is my turn to be late.

Quoted 20 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard
Quoted 17 lines

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Quoted 7 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Quoted 6 lines

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

Quoted 8 lines

But they shouldn't have. This has never been considered as an ABI. Userspace needs to computes and propagates resolutions through the pipeline dynamically, no hardcode them.

If your initialization script read the kernel version and aborted for any version other than v3.6, an upgrade to a newer kernel would break the system but you wouldn't call it a kernel regression :-)

:-) I should have a closer look to the configuration step, I agree that hardcoding is bad.

Quoted 5 lines

Problems with pipeline configuration shouldn't result in distorted images though. The driver is supposed to refuse to start streaming when the pipeline is misconfigured by making sure that resolutions on connected source and sink pads are identical. A valid pipeline should not distort the image.

Indeed.

Quoted 13 lines

After a quick look at the code the problem we're dealing with seems to be different and shouldn't affect userspace scripts if solved properly. I haven't touched the preview engine crop configuration code for some time now, so I'll need to refresh my memory, but it seems that the removal of

(quoted diff) +0 -7
-       if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
-           format->code != V4L2_MBUS_FMT_Y10_1X10) {
-               sph -= 2;
-               eph += 2;
-               slv -= 2;
-               elv += 2;
-       }
Quoted 4 lines

was wrong. The change to the margins and to preview_try_crop() seem correct, but the preview_config_input_size() function should probably have been kept unmodified. Could you please test reverting that part of the patch only ?

Ok. I will be away from my hardware until Tuesday, but I will test ASAP.

Best,

Florian

#6 lore raw

Re: Regression inside omap3isp/resizer

Hi Florian,

Quoted 77 lines · On Thursday 09 January 2014 21:45:54 Florian Vaussard wrote:
Quoted 44 lines · Laurent Pinchart
Quoted 36 lines · On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
Quoted 3 lines · Laurent Pinchart

Hi Florian,

Sorry for the late reply.

Now it is my turn to be late.

Quoted 20 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard
Quoted 17 lines

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media]
omap3isp: preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Quoted 8 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Quoted 6 lines

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

Quoted 7 lines

But they shouldn't have. This has never been considered as an ABI. Userspace needs to computes and propagates resolutions through the pipeline dynamically, no hardcode them.

If your initialization script read the kernel version and aborted for any version other than v3.6, an upgrade to a newer kernel would break the system but you wouldn't call it a kernel regression :-)

:-) I should have a closer look to the configuration step, I agree that hardcoding is bad.

Quoted 5 lines

Problems with pipeline configuration shouldn't result in distorted images though. The driver is supposed to refuse to start streaming when the pipeline is misconfigured by making sure that resolutions on connected source and sink pads are identical. A valid pipeline should not distort the image.

Indeed.

Quoted 13 lines

After a quick look at the code the problem we're dealing with seems to be different and shouldn't affect userspace scripts if solved properly. I haven't touched the preview engine crop configuration code for some time now, so I'll need to refresh my memory, but it seems that the removal of

(quoted diff) +0 -7
-       if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
-           format->code != V4L2_MBUS_FMT_Y10_1X10) {
-               sph -= 2;
-               eph += 2;
-               slv -= 2;
-               elv += 2;
-       }
Quoted 6 lines
Quoted 4 lines

was wrong. The change to the margins and to preview_try_crop() seem correct, but the preview_config_input_size() function should probably have been kept unmodified. Could you please test reverting that part of the patch only ?

Ok. I will be away from my hardware until Tuesday, but I will test ASAP.

I'll be away from my hardware next week, so you can take your time :-)

Signature · 3 lines
Regards,

Laurent Pinchart
#7 lore raw

Re: Regression inside omap3isp/resizer

In reply to Laurent Pinchart

Hi Laurent and Florian,

Quoted 71 lines · Laurent Pinchart

Hi Florian,

Quoted 35 lines · On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
Quoted 3 lines · Laurent Pinchart

Hi Florian,

Sorry for the late reply.

Now it is my turn to be late.

Quoted 20 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard
Quoted 17 lines

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Quoted 7 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Quoted 6 lines

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

But they shouldn't have. This has never been considered as an ABI. Userspace needs to computes and propagates resolutions through the pipeline dynamically, no hardcode them.

If your initialization script read the kernel version and aborted for any version other than v3.6, an upgrade to a newer kernel would break the system but you wouldn't call it a kernel regression :-)

Problems with pipeline configuration shouldn't result in distorted images though. The driver is supposed to refuse to start streaming when the pipeline is misconfigured by making sure that resolutions on connected source and sink pads are identical. A valid pipeline should not distort the image.

After a quick look at the code the problem we're dealing with seems to be different and shouldn't affect userspace scripts if solved properly. I haven't touched the preview engine crop configuration code for some time now, so I'll need to refresh my memory, but it seems that the removal of

(quoted diff) +0 -7
-       if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
-           format->code != V4L2_MBUS_FMT_Y10_1X10) {
-               sph -= 2;
-               eph += 2;
-               slv -= 2;
-               elv += 2;
-       }
Quoted 6 lines

was wrong. The change to the margins and to preview_try_crop() seem correct, but the preview_config_input_size() function should probably have been kept unmodified. Could you please test reverting that part of the patch only ?

Sakari, if you have time, could you please have a look at the code and give me your opinion ?

I reviewed the code mostly raw bayer -> yuv in mind; that appeared correct to me. Now, reading the code again, I agree with you --- the four above variables are how much additional cropping is performed on hardware, and if the CFA is enabled, the cropping is implicit rather than explicit.

It might have been cleaner to add cropping when pixels or lines need to be dropped rather than the other way around, but just adding the above lines back is probably the best way forward right now.

The patch itself (excluding the bug) seems fine. Cropping extra pixels when it wasn't needed for a reason was worth fixing IMO.

Signature · 6 lines
Kind regards,

Kind regards,

Sakari Ailus
[email protected]
#8 lore raw

Re: Regression inside omap3isp/resizer

Hi Laurent and Sakari,

Quoted 87 lines · Sakari Ailus

Hi Laurent and Florian,

Quoted 71 lines · Laurent Pinchart

Hi Florian,

Quoted 35 lines · On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
Quoted 3 lines · Laurent Pinchart

Hi Florian,

Sorry for the late reply.

Now it is my turn to be late.

Quoted 20 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard
Quoted 17 lines

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media] omap3isp:
preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER -> out

Quoted 7 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Quoted 6 lines

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

But they shouldn't have. This has never been considered as an ABI. Userspace needs to computes and propagates resolutions through the pipeline dynamically, no hardcode them.

If your initialization script read the kernel version and aborted for any version other than v3.6, an upgrade to a newer kernel would break the system but you wouldn't call it a kernel regression :-)

Problems with pipeline configuration shouldn't result in distorted images though. The driver is supposed to refuse to start streaming when the pipeline is misconfigured by making sure that resolutions on connected source and sink pads are identical. A valid pipeline should not distort the image.

After a quick look at the code the problem we're dealing with seems to be different and shouldn't affect userspace scripts if solved properly. I haven't touched the preview engine crop configuration code for some time now, so I'll need to refresh my memory, but it seems that the removal of

(quoted diff) +0 -7
-       if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
-           format->code != V4L2_MBUS_FMT_Y10_1X10) {
-               sph -= 2;
-               eph += 2;
-               slv -= 2;
-               elv += 2;
-       }
Quoted 6 lines

was wrong. The change to the margins and to preview_try_crop() seem correct, but the preview_config_input_size() function should probably have been kept unmodified. Could you please test reverting that part of the patch only ?

Sakari, if you have time, could you please have a look at the code and give me your opinion ?

I reviewed the code mostly raw bayer -> yuv in mind; that appeared correct to me. Now, reading the code again, I agree with you --- the four above variables are how much additional cropping is performed on hardware, and if the CFA is enabled, the cropping is implicit rather than explicit.

Quoted 7 lines

It might have been cleaner to add cropping when pixels or lines need to be dropped rather than the other way around, but just adding the above lines back is probably the best way forward right now.

The patch itself (excluding the bug) seems fine. Cropping extra pixels when it wasn't needed for a reason was worth fixing IMO.

I added the mentioned lines back, and it works fine again. The patch below fixes the issue (sorry, probably corrupted by my mail client. I can do a proper post if you are ok with it).

8<-----------------------------------------------------------------------

Subject: [PATCH 1/1] [media] omap3isp: preview: Fix the crop margins

Commit 3fdfedaaa "[media] omap3isp: preview: Lower the crop margins" accidentally changed the previewer's cropping, causing the previewer to miss four pixels on each line, thus corrupting the final image. Restored the removed setting.

---
 drivers/media/platform/omap3isp/isppreview.c | 9 +++++++++
 1 file changed, 9 insertions(+)
(quoted diff) +0 -0

b/drivers/media/platform/omap3isp/isppreview.c
index cd8831a..e2e4610 100644

drivers/media/platform/omap3isp/isppreview.c +0 -0
@@ -1079,6 +1079,7 @@ static void preview_config_input_format(struct

isp_prev_device *prev,
*/
static void preview_config_input_size(struct isp_prev_device *prev, u32
active)
{
+ const struct v4l2_mbus_framefmt *format =

&prev->formats[PREV_PAD_SINK];
        struct isp_device *isp = to_isp_device(prev);
        unsigned int sph = prev->crop.left;
        unsigned int eph = prev->crop.left + prev->crop.width - 1;
@@ -1086,6 +1087,14 @@ static void preview_config_input_size(struct
isp_prev_device *prev, u32 active)
        unsigned int elv = prev->crop.top + prev->crop.height - 1;
        u32 features;

+ if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
+ format->code != V4L2_MBUS_FMT_Y10_1X10) {

+               sph -= 2;
+               eph += 2;
+               slv -= 2;
+               elv += 2;
+       }
+
        features = (prev->params.params[0].features & active)
                 | (prev->params.params[1].features & ~active);
Signature · 1 line
1.8.1.2
#9 lore raw

Re: Regression inside omap3isp/resizer

Hi Florian,

Quoted 96 lines · On Friday 17 January 2014 15:45:12 Florian Vaussard wrote:
Quoted 82 lines · Sakari Ailus
Quoted 67 lines · Laurent Pinchart
Quoted 32 lines · On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
Quoted 22 lines · Laurent Pinchart
Quoted 21 lines · On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
Quoted 19 lines · Florian Vaussard

Hello Laurent,

I was working on having a functional IOMMU/ISP for 3.14, and had an issue with an image completely distorted. Comparing with another kernel, I saw that PRV_HORZ_INFO and PRV_VERT_INFO differed. On the newer kernel, sph, eph, svl, and slv were all off-by 2, causing my final image to miss 4 pixels on each line, thus distorting the result.

Your commit 3fdfedaaa7f243f3347084231c64f6c1be0ba131 '[media]
omap3isp: preview: Lower the crop margins' indeed changes PRV_HORZ_INFO and PRV_VERT_INFO by removing the if() condition. Reverting it made my image to be valid again.

FYI, my pipeline is:

MT9V032 (SGRBG10 752x480) -> CCDC -> PREVIEW (UYVY 752x480) -> RESIZER
-> out

Quoted 8 lines
Quoted 2 lines

Just an XMAS ping on this :-) Do you have any idea how to solve this without reverting the patch?

The patch indeed changed the preview engine margins, but the change is supposed to be handled by applications. As a base for this discussion could you please provide the media-ctl -p output before and after applying the patch ? You can strip the unrelated media entities out of the output.

Quoted 6 lines

Ok, so I understand the rationale behind this patch, but I am a bit concerned. If this patch requires a change in userspace, this is somehow breaking the userspace, isn't? For example in my case, I will have to change my initialization scripts in order to pass the correct resolution to the pipeline. Most people have probably hard-coded the resolution into their script / application.

But they shouldn't have. This has never been considered as an ABI. Userspace needs to computes and propagates resolutions through the pipeline dynamically, no hardcode them.

If your initialization script read the kernel version and aborted for any version other than v3.6, an upgrade to a newer kernel would break the system but you wouldn't call it a kernel regression :-)

Problems with pipeline configuration shouldn't result in distorted images though. The driver is supposed to refuse to start streaming when the pipeline is misconfigured by making sure that resolutions on connected source and sink pads are identical. A valid pipeline should not distort the image.

After a quick look at the code the problem we're dealing with seems to be different and shouldn't affect userspace scripts if solved properly. I haven't touched the preview engine crop configuration code for some time now, so I'll need to refresh my memory, but it seems that the removal of

(quoted diff) +0 -7
-       if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
-           format->code != V4L2_MBUS_FMT_Y10_1X10) {
-               sph -= 2;
-               eph += 2;
-               slv -= 2;
-               elv += 2;
-       }
Quoted 7 lines

was wrong. The change to the margins and to preview_try_crop() seem correct, but the preview_config_input_size() function should probably have been kept unmodified. Could you please test reverting that part of the patch only ?

Sakari, if you have time, could you please have a look at the code and give me your opinion ?

I reviewed the code mostly raw bayer -> yuv in mind; that appeared correct to me. Now, reading the code again, I agree with you --- the four above variables are how much additional cropping is performed on hardware, and if the CFA is enabled, the cropping is implicit rather than explicit.

Quoted 6 lines

It might have been cleaner to add cropping when pixels or lines need to be dropped rather than the other way around, but just adding the above lines back is probably the best way forward right now.

The patch itself (excluding the bug) seems fine. Cropping extra pixels when it wasn't needed for a reason was worth fixing IMO.

I added the mentioned lines back, and it works fine again. The patch below fixes the issue (sorry, probably corrupted by my mail client. I can do a proper post if you are ok with it).

8<-----------------------------------------------------------------------

Quoted 8 lines

Subject: [PATCH 1/1] [media] omap3isp: preview: Fix the crop margins

Commit 3fdfedaaa "[media] omap3isp: preview: Lower the crop margins" accidentally changed the previewer's cropping, causing the previewer to miss four pixels on each line, thus corrupting the final image. Restored the removed setting.

Could you please submit an uncorrupted patch ?

Quoted 36 lines
---
 drivers/media/platform/omap3isp/isppreview.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/media/platform/omap3isp/isppreview.c +1 -0
index cd8831a..e2e4610 100644
@@ -1079,6 +1079,7 @@ static void preview_config_input_format(struct
 isp_prev_device *prev,
  */
 static void preview_config_input_size(struct isp_prev_device *prev, u32
 active)
 {
+       const struct v4l2_mbus_framefmt *format =
 &prev->formats[PREV_PAD_SINK];
        struct isp_device *isp = to_isp_device(prev);
        unsigned int sph = prev->crop.left;
        unsigned int eph = prev->crop.left + prev->crop.width - 1;
drivers/media/platform/omap3isp/isppreview.c +8 -0
@@ -1086,6 +1087,14 @@ static void preview_config_input_size(struct
 isp_prev_device *prev, u32 active)
        unsigned int elv = prev->crop.top + prev->crop.height - 1;
        u32 features;
 
+       if (format->code != V4L2_MBUS_FMT_Y8_1X8 &&
+           format->code != V4L2_MBUS_FMT_Y10_1X10) {
+               sph -= 2;
+               eph += 2;
+               slv -= 2;
+               elv += 2;
+       }
+
        features = (prev->params.params[0].features & active)
 
                 | (prev->params.params[1].features & ~active);
Signature · 3 lines
Regards,

Laurent Pinchart