3fdfedaaa7f243f3347084231c64f6c1be0ba131 causes a regression

3fdfedaaa7f243f3347084231c64f6c1be0ba131 causes a regression

[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

Regards,

Florian

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

[Florian Vaussard]

Hello Laurent,

On 12/17/2013 11:42 AM, Florian Vaussard wrote:
> 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
> 

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

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

[Laurent Pinchart]

Hi Florian,

Sorry for the late reply.

On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
> > 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
> 
> 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.

-- 
Regards,

Laurent Pinchart

Re: Regression inside omap3isp/resizer

[Florian Vaussard]

Hi Laurent,

On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
> Hi Florian,
> 
> Sorry for the late reply.
> 

Now it is my turn to be late.

> On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
>> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
>>> 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
>>
>> 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]

Re: Regression inside omap3isp/resizer

[Laurent Pinchart]

Hi Florian,

On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
> On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
> > Hi Florian,
> > 
> > Sorry for the late reply.
> 
> Now it is my turn to be late.
> 
> > On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
> >> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
> >>> 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
> >> 
> >> 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.

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 ?

> 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]
-- 
Regards,

Laurent Pinchart

Re: Regression inside omap3isp/resizer

[Florian Vaussard]

Hi Laurent,

On 01/09/2014 09:34 PM, Laurent Pinchart wrote:
> Hi Florian,
> 
> On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
>> On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
>>> Hi Florian,
>>>
>>> Sorry for the late reply.
>>
>> Now it is my turn to be late.
>>
>>> On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
>>>> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
>>>>> 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
>>>>
>>>> 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.
> 
> 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.

> 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.

> 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 ?
> 

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

Best,

Florian

Re: Regression inside omap3isp/resizer

[Laurent Pinchart]

Hi Florian,

On Thursday 09 January 2014 21:45:54 Florian Vaussard wrote:
> On 01/09/2014 09:34 PM, Laurent Pinchart wrote:
> > On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
> >> On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
> >>> Hi Florian,
> >>> 
> >>> Sorry for the late reply.
> >> 
> >> Now it is my turn to be late.
> >> 
> >>> On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
> >>>> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
> >>>>> 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
> >>>> 
> >>>> 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.
> > 
> > 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.
> 
> > 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.
> 
> > 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 ?
>
> 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 :-)

-- 
Regards,

Laurent Pinchart

Re: Regression inside omap3isp/resizer

[Sakari Ailus]

Hi Laurent and Florian,

Laurent Pinchart wrote:
> Hi Florian,
> 
> On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
>> On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
>>> Hi Florian,
>>>
>>> Sorry for the late reply.
>>
>> Now it is my turn to be late.
>>
>>> On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
>>>> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
>>>>> 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
>>>>
>>>> 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.
> 
> 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 ?

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.

-- 
Kind regards,

Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: Regression inside omap3isp/resizer

[Florian Vaussard]

Hi Laurent and Sakari,

On 01/17/2014 08:15 AM, Sakari Ailus wrote:
> Hi Laurent and Florian,
> 
> Laurent Pinchart wrote:
>> Hi Florian,
>>
>> On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
>>> On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
>>>> Hi Florian,
>>>>
>>>> Sorry for the late reply.
>>>
>>> Now it is my turn to be late.
>>>
>>>> On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
>>>>> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
>>>>>> 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
>>>>>
>>>>> 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.
>>
>> 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 ?
> 
> 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.
> 

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.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/media/platform/omap3isp/isppreview.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/omap3isp/isppreview.c
b/drivers/media/platform/omap3isp/isppreview.c
index cd8831a..e2e4610 100644
--- a/drivers/media/platform/omap3isp/isppreview.c
+++ b/drivers/media/platform/omap3isp/isppreview.c
@@ -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);

-- 
1.8.1.2

Re: Regression inside omap3isp/resizer

[Laurent Pinchart]

Hi Florian,

On Friday 17 January 2014 15:45:12 Florian Vaussard wrote:
> On 01/17/2014 08:15 AM, Sakari Ailus wrote:
> > Laurent Pinchart wrote:
> >> On Thursday 09 January 2014 19:09:48 Florian Vaussard wrote:
> >>> On 12/31/2013 09:51 AM, Laurent Pinchart wrote:
> >>>> On Monday 23 December 2013 22:47:45 Florian Vaussard wrote:
> >>>>> On 12/17/2013 11:42 AM, Florian Vaussard wrote:
> >>>>>> 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
> >>>>> 
> >>>>> 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.
> >> 
> >> 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 ?
> > 
> > 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.
> 
> 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.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Could you please submit an uncorrupted patch ?

> ---
>  drivers/media/platform/omap3isp/isppreview.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/platform/omap3isp/isppreview.c
> b/drivers/media/platform/omap3isp/isppreview.c
> index cd8831a..e2e4610 100644
> --- a/drivers/media/platform/omap3isp/isppreview.c
> +++ b/drivers/media/platform/omap3isp/isppreview.c
> @@ -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);
-- 
Regards,

Laurent Pinchart