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@xxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
