Re: S3C244X/S3C64XX SoC camera host interface driver questions

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

Hi Andrey,

Cc: LMML

On 11/04/2012 09:43 PM, Andrey Gusakov wrote:
> Hi all.
>
>>> I'm testing your FIMC driver on S3C6410 hardware with OV9650 (in plans
>>> OV2640).
>>> I fixed some register definition, gpio init, fixed IRQ handling for
>>> S3C6410. So now i can get test frames from internal pattern generator.
>>> But when i try to read from external sensor i got blue or green trash.
>> Does it change when external light conditions change ? Or is it completely
>> random ?
> Yes. When it's dark - almost all picture is green. when I light on
> sensor, picture become more noisy with more blue pixels.
> If i comment out ov965x_init_regs write to sensor, I get black and
> white noise, like on TV when no signal.
> My opinion is there is some mistmatch in formats between sensor and FIMC.

Yes, I also suspect this is the root cause.

> But documentation on ov9650 is too conflicting and did not cover all
> registers used in driver.

Do you mean the OV9650 datasheet, version 1.3, from September 24, 2004 ?

>> What resolution are you testing with ?
> 320x240.
>> Are you using media-ctl to configure resolution on the camif and sensor
>> subdev ?
> I'm using GStreamer:
> gst-launch -v v4l2src device=/dev/video0 \
>          ! video/x-raw-yuv, width=320, height=240 \
>          ! ffmpegcolorspace \
>          ! fbdevsink

AFAIR, in the s3c-camif-v3.5 branch there was a bug that the CAMIF input
resolution was not being properly set to what was reported by the sensor
driver (default s3c-camif resolution is 640 x 480). The s3c-camif driver
is supposed to get format from a sensor subdev and set it on the S3C-CAMIF
subdev, upon image sensor subdev registration. Please see function
camif_register_sensor() for details.

The above issue should be fixed in this branch:
[1] http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif

Also, it could be verified by setting the formats with media-ctl manually,
before running gst-launch, i.e.

media-ctl --set-v4l2 '"OV9650":0 [fmt: YUYV2X8/320x240]'
media-ctl --set-v4l2 '"S3C-CAMIF":0 [fmt: YUYV2X8/320x240]'

with your current kernel and the s3c-camif driver.
media-ctl was integrated in the OSELAS mini2440 BSP and probably is also
in the mini6440 version.

>> And what kernel version is it ?
> 3.6.0-rc3 with some modifications for mini6410 devboard. Have no time
> to move my stuff to last samsung kernel.

OK, it's recent enough. The patches from s3c-camif branch should apply
cleanly.

>> Can you measure the frequency of the master clock that CAMIF provides
>> to the sensor ?
> 16.67 MHz. As i understand this is limitation of divider. Min clock is
> HCLKx2 (266) / 16 = 16.67 in my case.
> PCLK is near 4 MHz.

Looks fine (16.625).

>>> :) I spend two days trying to fix it, and now I'm out of ideas. May be
>>> you can help me.
>>> I have some questions.
>>> Did you have to invert some of camera signals (VSYNC, HREF, PCLK)?
>>> Maybe you had to make some hardware modifications? (i use CAM130
>>> module from FriendlyARM).
>>
>>
>> I'm also using CAM130, attached to mini2440 board without any hardware
>> modifications. Tomasz, (added at Cc) tested the driver on mini6410 and
>> it worked with same camera module. VSYNC, PCLK, etc. configuration is
>> specified in the board patch (as in the below branch).
>>
>> Are you using this version of patches:
>> http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif ?
> I'm using patches from
> https://github.com/snawrocki/linux/commits/s3c-camif-v3.5 wich was in
> your mail to samsung maillist.

I suggest you to update to the s3c-camif branch as above, it includes some
bug fixes. Sorry, I don't have exact patch for your issue handy right now.

>> I've also added at Cc In-Bae Jeong, who had successfully used the driver
>> on s3c6410 based board. However I'm not really sure now which board exactly
>> was it.
> I'm using mini6410. But i think there is only one way sensor can be
> connected to FIMC. Some differents may be in Reset and Power-down
> signals, but not in interface.

Yes, right. But since I2C communication works, the GPIOs and the clock
should be fine. It looks like there is a mismatch in the sensor and
the CAMIF registers configuration.

> P.S. Try to connect ov2640, but seems it need some modifications in
> FIMC driver to be connected thrue soc_camera_link.

Hmm, this is a general problem in the v4l2 core code. The are two slightly
incompatible interfaces that sensor subdevs are using. There were attempts
to make soc_camera subdevs usable with non-soc_camera host drivers, but it
is not finished yet. I could try to look at that, but I can't promise when
that happens.

There are pad level ops needed in the ov2640 driver - in addition to or
instead of g/s/try_mbus_fmt. Regulator supply names are not supposed to be
passed through platform data, those should be defined with fixed names as
specified in the sensor datasheet and board code should be providing 
required
regulator supply names or the dummy regulator should be used. IMO passing
regulator supply names through platform data is the regulator API misuse.

We need to resolve this issue at the v4l2 core level though, as problems
like this appear more and more often.

> Best regards.
> Andrey.

--

Regards,
Sylwester

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Tomasz Figa]

Hi Andrey,

On Sunday 04 of November 2012 23:14:47 Sylwester Nawrocki wrote:
> >> Are you using this version of patches:
> >> http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-cam
> >> if ?> 
> > I'm using patches from
> > https://github.com/snawrocki/linux/commits/s3c-camif-v3.5 wich was in
> > your mail to samsung maillist.
> 
> I suggest you to update to the s3c-camif branch as above, it includes
> some bug fixes. Sorry, I don't have exact patch for your issue handy
> right now.

Yes, you should definitely try the branch pointed by Sylwester, because the 
one you originally tested does not contain my fixes for S3C6410.

Best regards,
Tomasz Figa

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Andrey Gusakov]

Hi.

>> But documentation on ov9650 is too conflicting and did not cover all
>> registers used in driver.
> Do you mean the OV9650 datasheet, version 1.3, from September 24, 2004 ?
Yes. Also I have datasheet version 1.91 from January 28, 2005 and
Application note 1.1 from 7 December 2004
All can be found here [1].
It seems there is different versions of sensor exist. With VER=0x50
and 0x52. I have second one.

>>> What resolution are you testing with ?
>>
>> 320x240.
>>>
>>> Are you using media-ctl to configure resolution on the camif and sensor
>>> subdev ?
>>
>> I'm using GStreamer:
>> gst-launch -v v4l2src device=/dev/video0 \
>>          ! video/x-raw-yuv, width=320, height=240 \
>>          ! ffmpegcolorspace \
>>          ! fbdevsink
>
>
> AFAIR, in the s3c-camif-v3.5 branch there was a bug that the CAMIF input
> resolution was not being properly set to what was reported by the sensor
> driver (default s3c-camif resolution is 640 x 480). The s3c-camif driver
> is supposed to get format from a sensor subdev and set it on the S3C-CAMIF
> subdev, upon image sensor subdev registration. Please see function
> camif_register_sensor() for details.
>
> The above issue should be fixed in this branch:
> [1] http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif
>
> Also, it could be verified by setting the formats with media-ctl manually,
> before running gst-launch, i.e.
>
> media-ctl --set-v4l2 '"OV9650":0 [fmt: YUYV2X8/320x240]'
> media-ctl --set-v4l2 '"S3C-CAMIF":0 [fmt: YUYV2X8/320x240]'
>
> with your current kernel and the s3c-camif driver.
> media-ctl was integrated in the OSELAS mini2440 BSP and probably is also
> in the mini6440 version.
Thanks. I'll try it. media-ctl from OpenEmbedded for some reason don't
like this params.

>>> And what kernel version is it ?
>>
>> 3.6.0-rc3 with some modifications for mini6410 devboard. Have no time
>> to move my stuff to last samsung kernel.
>
>
> OK, it's recent enough. The patches from s3c-camif branch should apply
> cleanly.
>
>
>>> Can you measure the frequency of the master clock that CAMIF provides
>>> to the sensor ?
>>
>> 16.67 MHz. As i understand this is limitation of divider. Min clock is
>> HCLKx2 (266) / 16 = 16.67 in my case.
>> PCLK is near 4 MHz.
>
>
> Looks fine (16.625).
>
>
>>>> :) I spend two days trying to fix it, and now I'm out of ideas. May be
>>>> you can help me.
>>>> I have some questions.
>>>> Did you have to invert some of camera signals (VSYNC, HREF, PCLK)?
>>>> Maybe you had to make some hardware modifications? (i use CAM130
>>>> module from FriendlyARM).
>>>
>>>
>>>
>>> I'm also using CAM130, attached to mini2440 board without any hardware
>>> modifications. Tomasz, (added at Cc) tested the driver on mini6410 and
>>> it worked with same camera module. VSYNC, PCLK, etc. configuration is
>>> specified in the board patch (as in the below branch).
>>>
>>> Are you using this version of patches:
>>> http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif
>>> ?
>>
>> I'm using patches from
>> https://github.com/snawrocki/linux/commits/s3c-camif-v3.5 wich was in
>> your mail to samsung maillist.
>
>
> I suggest you to update to the s3c-camif branch as above, it includes some
> bug fixes. Sorry, I don't have exact patch for your issue handy right now.
I just try this branch. No luck. Now it fails on __ov965x_set_params
while writing some registers:
...
OV9650: i2c_write: 0x40 : 0xc1. ret: 2
OV9650: i2c_write: 0x29 : 0x3f. ret: 2
OV9650: i2c_write: 0x0F : 0x43. ret: -6
...
If I comment out exits on this errors, following writes in sensors
give -6 (ENXIO) or -111 (ECONNREFUSED) erros. Seems sensors hung after
write to some registers.

>>> I've also added at Cc In-Bae Jeong, who had successfully used the driver
>>> on s3c6410 based board. However I'm not really sure now which board
>>> exactly
>>> was it.
>>
>> I'm using mini6410. But i think there is only one way sensor can be
>> connected to FIMC. Some differents may be in Reset and Power-down
>> signals, but not in interface.
>
>
> Yes, right. But since I2C communication works, the GPIOs and the clock
> should be fine. It looks like there is a mismatch in the sensor and
> the CAMIF registers configuration.
>
>
>> P.S. Try to connect ov2640, but seems it need some modifications in
>> FIMC driver to be connected thrue soc_camera_link.
>
>
> Hmm, this is a general problem in the v4l2 core code. The are two slightly
> incompatible interfaces that sensor subdevs are using. There were attempts
> to make soc_camera subdevs usable with non-soc_camera host drivers, but it
> is not finished yet. I could try to look at that, but I can't promise when
> that happens.
>
> There are pad level ops needed in the ov2640 driver - in addition to or
> instead of g/s/try_mbus_fmt. Regulator supply names are not supposed to be
> passed through platform data, those should be defined with fixed names as
> specified in the sensor datasheet and board code should be providing
> required
> regulator supply names or the dummy regulator should be used. IMO passing
> regulator supply names through platform data is the regulator API misuse.
>
> We need to resolve this issue at the v4l2 core level though, as problems
> like this appear more and more often.
Thanks for the advice. Will probably have to deal with this in the near future.

[1] http://roboforum.ru/forum36/topic7835.html

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

Hi,

On 11/05/2012 10:44 AM, Andrey Gusakov wrote:
>>> But documentation on ov9650 is too conflicting and did not cover all
>>> registers used in driver.
>> Do you mean the OV9650 datasheet, version 1.3, from September 24, 2004 ?
> Yes. Also I have datasheet version 1.91 from January 28, 2005 and
> Application note 1.1 from 7 December 2004
> All can be found here [1].
> It seems there is different versions of sensor exist. With VER=0x50
> and 0x52. I have second one.

Hmm, in my case VER was 0x50. PID, VER = 0x96, 0x50. And this a default 
value
after reset according to the datasheet, ver. 1.3. For ver. 1.91 it is
PID, VER = 0x96, 0x52. Perhaps it just indicates ov9652 sensor ov9652.
Obviously I didn't test the driver with this one. Possibly the differences
can be resolved by comparing the documentation. Not sure if those are
significant and how much it makes sense to have single driver for both
sensor versions. I'll try to have a look at that.

...
>>>> Are you using media-ctl to configure resolution on the camif and sensor
>>>> subdev ?
>>>
>>> I'm using GStreamer:
>>> gst-launch -v v4l2src device=/dev/video0 \
>>>           ! video/x-raw-yuv, width=320, height=240 \
>>>           ! ffmpegcolorspace \
>>>           ! fbdevsink
>>
>>
>> AFAIR, in the s3c-camif-v3.5 branch there was a bug that the CAMIF input
>> resolution was not being properly set to what was reported by the sensor
>> driver (default s3c-camif resolution is 640 x 480). The s3c-camif driver
>> is supposed to get format from a sensor subdev and set it on the S3C-CAMIF
>> subdev, upon image sensor subdev registration. Please see function
>> camif_register_sensor() for details.
>>
>> The above issue should be fixed in this branch:
>> [1] http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif
>>
>> Also, it could be verified by setting the formats with media-ctl manually,
>> before running gst-launch, i.e.
>>
>> media-ctl --set-v4l2 '"OV9650":0 [fmt: YUYV2X8/320x240]'
>> media-ctl --set-v4l2 '"S3C-CAMIF":0 [fmt: YUYV2X8/320x240]'
>>
>> with your current kernel and the s3c-camif driver.
>> media-ctl was integrated in the OSELAS mini2440 BSP and probably is also
>> in the mini6440 version.
> Thanks. I'll try it. media-ctl from OpenEmbedded for some reason don't
> like this params.

As you might know the development version can be found here:
http://git.ideasonboard.org/?p=media-ctl.git;a=summary

There has been some change in the command line semantics recently.
...
>> I suggest you to update to the s3c-camif branch as above, it includes some
>> bug fixes. Sorry, I don't have exact patch for your issue handy right now.
> I just try this branch. No luck. Now it fails on __ov965x_set_params
> while writing some registers:
> ...
> OV9650: i2c_write: 0x40 : 0xc1. ret: 2
> OV9650: i2c_write: 0x29 : 0x3f. ret: 2
> OV9650: i2c_write: 0x0F : 0x43. ret: -6
> ...
> If I comment out exits on this errors, following writes in sensors
> give -6 (ENXIO) or -111 (ECONNREFUSED) erros. Seems sensors hung after
> write to some registers.

Hmm, that looks bad. The driver wasn't tested with VER=0x52 and there
must be some differences that need to get sorted out.

> [1] http://roboforum.ru/forum36/topic7835.html

--

Regards,
Sylwester

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Andrey Gusakov]

Hi.

Thanks all!
I make it work! Have to comment out write { REG_GRCOM, 0x3f },	/*
Analog BLC & regulator */ and have to enable gate clock for fimc at
probe.

> Hmm, in my case VER was 0x50. PID, VER = 0x96, 0x50. And this a default
> value
> after reset according to the datasheet, ver. 1.3. For ver. 1.91 it is
> PID, VER = 0x96, 0x52. Perhaps it just indicates ov9652 sensor ov9652.
> Obviously I didn't test the driver with this one. Possibly the differences
> can be resolved by comparing the documentation. Not sure if those are
> significant and how much it makes sense to have single driver for both
> sensor versions. I'll try to have a look at that.
Ok. I also try to compate init sequenses from different sources on web.

Next step is to make ov2460 work.

Best regards.
Andrey.

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

Hi Andrey,

On 11/05/2012 12:11 PM, Andrey Gusakov wrote:
> Hi.
>
> Thanks all!
> I make it work! Have to comment out write { REG_GRCOM, 0x3f },	/*

Great news!

Does the sensor still hang after 0x2f is written to REG_GRCOM instead ?

> Analog BLC&  regulator */ and have to enable gate clock for fimc at
> probe.

Do you have CONFIG_PM_RUNTIME enabled ? Can you try and see it works
if you enable it, without additional changes to the clock handling ?

I hope to eventually prepare the ov9650 sensor driver for mainline. Your
help in making it ready for VER=0x52 would be very much appreciated. :-)

>> Hmm, in my case VER was 0x50. PID, VER = 0x96, 0x50. And this a default
>> value
>> after reset according to the datasheet, ver. 1.3. For ver. 1.91 it is
>> PID, VER = 0x96, 0x52. Perhaps it just indicates ov9652 sensor ov9652.
>> Obviously I didn't test the driver with this one. Possibly the differences
>> can be resolved by comparing the documentation. Not sure if those are
>> significant and how much it makes sense to have single driver for both
>> sensor versions. I'll try to have a look at that.
> Ok. I also try to compate init sequenses from different sources on web.
>
> Next step is to make ov2460 work.

For now I can only recommend you to make the ov2460 driver more similar
to the ov9650 one.

--
Regards,
Sylwester

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Andrey Gusakov]

Hi.

> Does the sensor still hang after 0x2f is written to REG_GRCOM instead ?
Work!
I'm looking at drivers/media/usb/gspca/m5602/m5602_ov9650.h
It use significantly different init sequence. Some of settings
described in Application note for ov9650, some look like magic.

> Do you have CONFIG_PM_RUNTIME enabled ? Can you try and see it works
> if you enable it, without additional changes to the clock handling ?
Work. With CONFIG_PM_RUNTIME and without enabling CLK_GATE at probe.

> I hope to eventually prepare the ov9650 sensor driver for mainline. Your
> help in making it ready for VER=0x52 would be very much appreciated. :-)
I'll try to helpful.


>> Next step is to make ov2460 work.
> For now I can only recommend you to make the ov2460 driver more similar
> to the ov9650 one.
Thanks, I'll try.

P.S. I add support of image effects just for fun. And found in DS that
s3c2450 also support effects. It's FIMC in-between of 2440 and
6400/6410. Does anyone have s3c2450 hardware to test it?

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

On 11/06/2012 10:34 PM, Andrey Gusakov wrote:
> Hi.
>
>> Does the sensor still hang after 0x2f is written to REG_GRCOM instead ?
> Work!
> I'm looking at drivers/media/usb/gspca/m5602/m5602_ov9650.h
> It use significantly different init sequence. Some of settings
> described in Application note for ov9650, some look like magic.

I guess there are many ways the sensor can be configured initially.
I'd like to keep this initialization sequence as thin as possible,
and to move relevant settings to corresponding v4l2 controls.
Then after v4l2_control_handler_setup() is called, following the initial
register list write, the sensor would be configured into some known
state. I realize it might be more difficult in practice than it sounds
now. :-)

>> Do you have CONFIG_PM_RUNTIME enabled ? Can you try and see it works
>> if you enable it, without additional changes to the clock handling ?
> Work. With CONFIG_PM_RUNTIME and without enabling CLK_GATE at probe.

Ok, thanks. I will add the missing CONFIG_PM_RUNTIME dependency in Kconfig.
The driver has to have PM_RUNTIME enabled since on s3c64xx SoCs there are
power domains and the camera power domain needs to be enabled for the CAMIF
operation. The pm_runtime_* calls in the driver are supposed to ensure 
that.
I wonder why it works for you without PM_RUNTIME, i.e. how comes the power
domain is enabled. It is supposed to be off by default.

>> I hope to eventually prepare the ov9650 sensor driver for mainline. Your
>> help in making it ready for VER=0x52 would be very much appreciated. :-)
> I'll try to helpful.
>
>
>>> Next step is to make ov2460 work.
>> For now I can only recommend you to make the ov2460 driver more similar
>> to the ov9650 one.
> Thanks, I'll try.
>
> P.S. I add support of image effects just for fun. And found in DS that
> s3c2450 also support effects. It's FIMC in-between of 2440 and
> 6400/6410. Does anyone have s3c2450 hardware to test it?

Patches adding image effect are welcome. I'm bit to busy to play with these
things, other than I don't have hardware to test it.
I wasn't really aware of CAMIF in s3c2450. I think a separate variant data
structure would need to be defined for s3c2450. If anyone ever needs it
it could be added easily. For now I'll pretend this version doesn't 
exist. :-)

--

Regards,
Sylwester

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Andrey Gusakov]

[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]

Hi.

On Thu, Nov 8, 2012 at 1:57 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 11/06/2012 10:34 PM, Andrey Gusakov wrote:
>>
>> Hi.
>>
>>> Does the sensor still hang after 0x2f is written to REG_GRCOM instead ?
>>
>> Work!
>> I'm looking at drivers/media/usb/gspca/m5602/m5602_ov9650.h
>> It use significantly different init sequence. Some of settings
>> described in Application note for ov9650, some look like magic.
>
>
> I guess there are many ways the sensor can be configured initially.
> I'd like to keep this initialization sequence as thin as possible,
> and to move relevant settings to corresponding v4l2 controls.
> Then after v4l2_control_handler_setup() is called, following the initial
> register list write, the sensor would be configured into some known
> state. I realize it might be more difficult in practice than it sounds
> now. :-)
>
>
>>> Do you have CONFIG_PM_RUNTIME enabled ? Can you try and see it works
>>> if you enable it, without additional changes to the clock handling ?
>>
>> Work. With CONFIG_PM_RUNTIME and without enabling CLK_GATE at probe.
>
>
> Ok, thanks. I will add the missing CONFIG_PM_RUNTIME dependency in Kconfig.
> The driver has to have PM_RUNTIME enabled since on s3c64xx SoCs there are
> power domains and the camera power domain needs to be enabled for the CAMIF
> operation. The pm_runtime_* calls in the driver are supposed to ensure that.
> I wonder why it works for you without PM_RUNTIME, i.e. how comes the power
> domain is enabled. It is supposed to be off by default.
DS says that all power domaint are on after reset. My bootloader did
not switch then off. So when linux start everything is on.
CONFIG_PM_RUNTIME was disabled, so nothing switch them off in linux
too.

>>> I hope to eventually prepare the ov9650 sensor driver for mainline. Your
>>> help in making it ready for VER=0x52 would be very much appreciated. :-)
>>
>> I'll try to helpful.
>>
>>
>>>> Next step is to make ov2460 work.
>>>
>>> For now I can only recommend you to make the ov2460 driver more similar
>>> to the ov9650 one.
>>
>> Thanks, I'll try.
>>
>> P.S. I add support of image effects just for fun. And found in DS that
>> s3c2450 also support effects. It's FIMC in-between of 2440 and
>> 6400/6410. Does anyone have s3c2450 hardware to test it?

> Patches adding image effect are welcome. I'm bit to busy to play with these
> things, other than I don't have hardware to test it.
> I wasn't really aware of CAMIF in s3c2450. I think a separate variant data
> structure would need to be defined for s3c2450. If anyone ever needs it
> it could be added easily. For now I'll pretend this version doesn't exist.
> :-)
Attached.

I often get "VIDIOC_QUERYCAP: failed: Inappropriate ioctl for device"
or "system error: Inappropriate ioctl for device"
Is it because of not implemented set/get framerate func? How this
should work? I mean framerate heavy depend of sensor's settings. So
set/get framerate call to fimc should get/set framerate from sensor.
What is mechanism of such things?
And same question about synchronizing format of sensor and FIMC pads.
I make ov2640 work, but if did not call media-ctl for sensor, format
of FIMC sink pad and format of sensor source pad different. I think I
missed something, but reading other sources did not help.

Thanks.
Best regards.

[-- Attachment #2: 0001-S3C-FIMC-add-effect-controls.patch --]
[-- Type: application/octet-stream, Size: 8433 bytes --]

From 04b88737f65f772f8b375234a92c7cdd481eac1b Mon Sep 17 00:00:00 2001
From: Andrey Gusakov <dron_gus@mail.ru>
Date: Mon, 5 Nov 2012 15:50:23 +0400
Subject: [PATCH] S3C-FIMC: add effect controls


Signed-off-by: Andrey Gusakov <dron_gus@mail.ru>
---
 drivers/media/platform/s3c-camif/camif-capture.c |   58 ++++++++++++++++++++--
 drivers/media/platform/s3c-camif/camif-core.h    |    5 ++
 drivers/media/platform/s3c-camif/camif-regs.c    |   38 ++++++++++----
 drivers/media/platform/s3c-camif/camif-regs.h    |    6 ++-
 4 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index ca31c45..046ebf6 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -81,6 +81,9 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
 	camif_hw_set_source_format(camif);
 	camif_hw_set_camera_crop(camif);
 	camif_hw_set_test_pattern(camif, camif->test_pattern->val);
+	if (ip_rev >= S3C2450_CAMIF_IP_REV)
+		camif_hw_set_effect(camif, camif->effect->val,
+				camif->effect_cr->val, camif->effect_cb->val);
 	if (ip_rev == S3C6410_CAMIF_IP_REV)
 		camif_hw_set_input_path(vp);
 	camif_cfg_video_path(vp);
@@ -108,8 +111,8 @@ static int s3c_camif_hw_vp_init(struct camif_dev *camif, struct camif_vp *vp)
 	if (ip_rev == S3C244X_CAMIF_IP_REV)
 		camif_hw_clear_fifo_overflow(vp);
 	camif_cfg_video_path(vp);
-	if (ip_rev == S3C6410_CAMIF_IP_REV)
-		camif_hw_set_effect(vp, false);
+	if (ip_rev >= S3C2450_CAMIF_IP_REV)
+		camif_hw_set_effect(camif, 0, 0, 0);
 	vp->state &= ~ST_VP_CONFIG;
 
 	spin_unlock_irqrestore(&camif->slock, flags);
@@ -374,6 +377,10 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
 		camif_hw_set_scaler(vp);
 		camif_hw_set_flip(vp);
 		camif_hw_set_test_pattern(camif, camif->test_pattern->val);
+		if (ip_rev >= S3C2450_CAMIF_IP_REV)
+			camif_hw_set_effect(camif, camif->effect->val,
+					camif->effect_cr->val,
+					camif->effect_cb->val);
 		vp->state &= ~ST_VP_CONFIG;
 	}
 unlock:
@@ -1530,7 +1537,7 @@ static const struct v4l2_ctrl_ops s3c_camif_subdev_ctrl_ops = {
 	.s_ctrl	= s3c_camif_subdev_s_ctrl,
 };
 
-static const struct v4l2_ctrl_config s3c_camif_priv_ctrl = {
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_test = {
 	.ops	= &s3c_camif_subdev_ctrl_ops,
 	.id	= V4L2_CTRL_CLASS_USER | 0x1001,
 	.type	= V4L2_CTRL_TYPE_INTEGER,
@@ -1541,6 +1548,39 @@ static const struct v4l2_ctrl_config s3c_camif_priv_ctrl = {
 	.def	= 0,
 };
 
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_eff = {
+	.ops	= &s3c_camif_subdev_ctrl_ops,
+	.id	= V4L2_CTRL_CLASS_USER | 0x1002,
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.name	= "Effect",
+	.min	= 0,
+	.max	= 5,
+	.step	= 1,
+	.def	= 0,
+};
+
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_eff_cb = {
+	.ops	= &s3c_camif_subdev_ctrl_ops,
+	.id	= V4L2_CTRL_CLASS_USER | 0x1003,
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.name	= "PAT_Cb",
+	.min	= 16,
+	.max	= 240,
+	.step	= 1,
+	.def	= 128,
+};
+
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_eff_cr = {
+	.ops	= &s3c_camif_subdev_ctrl_ops,
+	.id	= V4L2_CTRL_CLASS_USER | 0x1004,
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.name	= "PAT_Cr",
+	.min	= 16,
+	.max	= 240,
+	.step	= 1,
+	.def	= 128,
+};
+
 int s3c_camif_create_subdev(struct camif_dev *camif)
 {
 	struct v4l2_ctrl_handler *handler = &camif->ctrl_handler;
@@ -1560,10 +1600,18 @@ int s3c_camif_create_subdev(struct camif_dev *camif)
 	if (ret)
 		return ret;
 
-	v4l2_ctrl_handler_init(handler, 1);
+	v4l2_ctrl_handler_init(handler, 4);
 	camif->test_pattern = v4l2_ctrl_new_custom(handler,
-					&s3c_camif_priv_ctrl, NULL);
+					&s3c_camif_priv_ctrl_test, NULL);
+	camif->effect = v4l2_ctrl_new_custom(handler,
+					&s3c_camif_priv_ctrl_eff, NULL);
+	camif->effect_cr = v4l2_ctrl_new_custom(handler,
+					&s3c_camif_priv_ctrl_eff_cb, NULL);
+	camif->effect_cb = v4l2_ctrl_new_custom(handlerEffect,
+					&s3c_camif_priv_ctrl_eff_cr, NULL);
+
 	if (handler->error) {
+		v4l2_ctrl_handler_free(handler);
 		media_entity_cleanup(&sd->entity);
 		return handler->error;
 	}
diff --git a/drivers/media/platform/s3c-camif/camif-core.h b/drivers/media/platform/s3c-camif/camif-core.h
index 96f5d3d..5f9eb3a 100644
--- a/drivers/media/platform/s3c-camif/camif-core.h
+++ b/drivers/media/platform/s3c-camif/camif-core.h
@@ -39,6 +39,8 @@
 #define CAMIF_STOP_TIMEOUT	1500 /* ms */
 
 #define S3C244X_CAMIF_IP_REV	0x20 /* 2.0 */
+#define S3C2450_CAMIF_IP_REV	0x30 /* 3.0 - not implemented, not tested */
+#define S3C6400_CAMIF_IP_REV	0x31 /* 3.1 - not implemented, not tested */
 #define S3C6410_CAMIF_IP_REV	0x32 /* 3.2 */
 
 /* struct camif_vp::state */
@@ -277,6 +279,9 @@ struct camif_dev {
 
 	struct v4l2_ctrl_handler	ctrl_handler;
 	struct v4l2_ctrl		*test_pattern;
+	struct v4l2_ctrl		*effect;
+	struct v4l2_ctrl		*effect_cb;
+	struct v4l2_ctrl		*effect_cr;
 
 	struct camif_vp			vp[CAMIF_VP_NUM];
 	struct vb2_alloc_ctx		*alloc_ctx;
diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c
index d8c55dc..1b03f73 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.c
+++ b/drivers/media/platform/s3c-camif/camif-regs.c
@@ -57,6 +57,33 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
 	camif_write(camif, S3C_CAMIF_REG_CIGCTRL, cfg);
 }
 
+void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
+			unsigned int cr, unsigned int cb)
+{
+	u32 cfg;
+
+	if (camif->variant->ip_revision < S3C2450_CAMIF_IP_REV)
+		return;
+
+	cfg = camif_read(camif, S3C_CAMIF_REG_CIIMGEFF(camif->vp->offset));
+	/* Set effect */
+	cfg &= ~CIIMGEFF_FIN_MASK;
+	cfg |= (effect << 26);
+	/* Set both paths */
+	if (camif->variant->ip_revision >= S3C6400_CAMIF_IP_REV) {
+		if (effect)
+			cfg |= CIIMGEFF_IE_ENABLE_MASK;
+		else
+			cfg &= ~CIIMGEFF_IE_ENABLE_MASK;
+	}
+	/* Set Cr, Cb */
+	if (effect == CIIMGEFF_FIN_ARBITRARY) {
+		cfg &= ~CIIMGEFF_PAT_CBCR_MASK;
+		cfg |= ((cb & 0xFF) << 13) | (cr & 0xFF);
+	}
+	camif_write(camif, S3C_CAMIF_REG_CIIMGEFF(camif->vp->offset), cfg);
+}
+
 static const u32 src_pixfmt_map[8][2] = {
 	{ V4L2_MBUS_FMT_YUYV8_2X8, CISRCFMT_ORDER422_YCBYCR },
 	{ V4L2_MBUS_FMT_YVYU8_2X8, CISRCFMT_ORDER422_YCRYCB },
@@ -473,17 +500,6 @@ void camif_hw_set_lastirq(struct camif_vp *vp, int enable)
 	camif_write(vp->camif, addr, cfg);
 }
 
-void camif_hw_set_effect(struct camif_vp *vp, bool active)
-{
-	u32 cfg = 0;
-
-	if (active) {
-		/* TODO: effects support on 64xx */
-	}
-
-	camif_write(vp->camif, S3C_CAMIF_REG_CIIMGEFF, cfg);
-}
-
 void camif_hw_enable_capture(struct camif_vp *vp)
 {
 	struct camif_dev *camif = vp->camif;
diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h
index a3488ca..213adbc 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.h
+++ b/drivers/media/platform/s3c-camif/camif-regs.h
@@ -177,8 +177,9 @@
 #define S3C_CAMIF_REG_CICPTSEQ			0xc4
 
 /* Image effects */
-#define S3C_CAMIF_REG_CIIMGEFF			0xd0
+#define S3C_CAMIF_REG_CIIMGEFF(_offs)		(0xb0 + (_offs))
 #define  CIIMGEFF_IE_ENABLE(id)			(1 << (30 + (id)))
+#define  CIIMGEFF_IE_ENABLE_MASK		(3 << 30)
 /* Image effect: 1 - after scaler, 0 - before scaler */
 #define  CIIMGEFF_IE_AFTER_SC			(1 << 29)
 #define  CIIMGEFF_FIN_MASK			(7 << 26)
@@ -243,7 +244,6 @@ void camif_hw_clear_fifo_overflow(struct camif_vp *vp);
 void camif_hw_set_lastirq(struct camif_vp *vp, int enable);
 void camif_hw_set_input_path(struct camif_vp *vp);
 void camif_hw_enable_scaler(struct camif_vp *vp, bool on);
-void camif_hw_set_effect(struct camif_vp *vp, bool active);
 void camif_hw_enable_capture(struct camif_vp *vp);
 void camif_hw_disable_capture(struct camif_vp *vp);
 void camif_hw_set_camera_bus(struct camif_dev *camif);
@@ -254,6 +254,8 @@ void camif_hw_set_flip(struct camif_vp *vp);
 void camif_hw_set_output_dma(struct camif_vp *vp);
 void camif_hw_set_target_format(struct camif_vp *vp);
 void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
+void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
+			unsigned int cr, unsigned int cb);
 void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
 			      int index);
 void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
-- 
1.7.0.4

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

Hi,

On 11/08/2012 07:47 PM, Andrey Gusakov wrote:
>> Ok, thanks. I will add the missing CONFIG_PM_RUNTIME dependency in Kconfig.
>> The driver has to have PM_RUNTIME enabled since on s3c64xx SoCs there are
>> power domains and the camera power domain needs to be enabled for the CAMIF
>> operation. The pm_runtime_* calls in the driver are supposed to ensure that.
>> I wonder why it works for you without PM_RUNTIME, i.e. how comes the power
>> domain is enabled. It is supposed to be off by default.
> DS says that all power domaint are on after reset. My bootloader did
> not switch then off. So when linux start everything is on.
> CONFIG_PM_RUNTIME was disabled, so nothing switch them off in linux
> too.

Yes, indeed. But there was a PM code added that is supposed to disable all
unused power domains on the system boot. I noticed that one needs to call
explicitly s3c64xx_pm_init() function from machine_init() callback within
the board file. So far this function is called only in mach-crag6410.c.
I'm not sure it it won't kill the display if you use it though. Probably
PM domain state should be read from a respective register and this
information passed to pm_genpd_init() function within s3c64_pm_init().

>>>> I hope to eventually prepare the ov9650 sensor driver for mainline. Your
>>>> help in making it ready for VER=0x52 would be very much appreciated. :-)
>>>
>>> I'll try to helpful.
>>>
>>>
>>>>> Next step is to make ov2460 work.
>>>>
>>>> For now I can only recommend you to make the ov2460 driver more similar
>>>> to the ov9650 one.
>>>
>>> Thanks, I'll try.
>>>
>>> P.S. I add support of image effects just for fun. And found in DS that
>>> s3c2450 also support effects. It's FIMC in-between of 2440 and
>>> 6400/6410. Does anyone have s3c2450 hardware to test it?
>
>> Patches adding image effect are welcome. I'm bit to busy to play with these
>> things, other than I don't have hardware to test it.
>> I wasn't really aware of CAMIF in s3c2450. I think a separate variant data
>> structure would need to be defined for s3c2450. If anyone ever needs it
>> it could be added easily. For now I'll pretend this version doesn't exist.
>> :-)
> Attached.
>
> I often get "VIDIOC_QUERYCAP: failed: Inappropriate ioctl for device"

This is an issue in the v4l2-ctl, it is going to be fixed by adding
VIDIOC_SUBDEV_QUERYCAP ioctl for subdevs. It has been just discussed today.
I guess you get it when running v4l2-ctl on /dev/v4l-subdev* ?

> or "system error: Inappropriate ioctl for device"

I think this one is caused by unimplemented VIDIOC_G/S_PARM ioctls
at the s3c-camif driver.

> Is it because of not implemented set/get framerate func? How this

Yes, I think so. ioctls as above.

> should work? I mean framerate heavy depend of sensor's settings. So
> set/get framerate call to fimc should get/set framerate from sensor.
> What is mechanism of such things?

With user space subdev API one should control frame interval directly
on the sensor subdev device node [1]. For Gstreamer to work with
VIDIOC_G/S_PARM ioctls we need a dedicated v4l2 library (possibly with
a plugin for s3c-camif, but that shouldn't be needed since it is very
simple driver) that will translate those video node ioctls into the
subdev node ioctls [2]. Unfortunately such library is still not available.

> And same question about synchronizing format of sensor and FIMC pads.
> I make ov2640 work, but if did not call media-ctl for sensor, format
> of FIMC sink pad and format of sensor source pad different. I think I
> missed something, but reading other sources did not help.

As I explained previously, s3c-fimc is supposed to synchronize format
with the sensor subdev. Have you got pad level get_fmt callback
implemented in the ov2640 driver ?
Could you post your 'media-ctl -p' output, run right after the system boot ?

[1] 
http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-subdev-g-frame-interval.html
[2] http://git.linuxtv.org/v4l-utils.git/tree/
-----------------------------------------------------------------------------

 From 04b88737f65f772f8b375234a92c7cdd481eac1b Mon Sep 17 00:00:00 2001
From: Andrey Gusakov <dron_gus@mail.ru>
Date: Mon, 5 Nov 2012 15:50:23 +0400
Subject: [PATCH] S3C-FIMC: add effect controls

SN:
I prefer using s3c-camif name, FIMC appears only in later version of
the SoCs. Also would be nice to put at least some brief description why
this patch is needed.

Signed-off-by: Andrey Gusakov <dron_gus@mail.ru>
---
  drivers/media/platform/s3c-camif/camif-capture.c |   58 
++++++++++++++++++++--
  drivers/media/platform/s3c-camif/camif-core.h    |    5 ++
  drivers/media/platform/s3c-camif/camif-regs.c    |   38 ++++++++++----
  drivers/media/platform/s3c-camif/camif-regs.h    |    6 ++-
  4 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/s3c-camif/camif-capture.c 
b/drivers/media/platform/s3c-camif/camif-capture.c
index ca31c45..046ebf6 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -81,6 +81,9 @@ static int s3c_camif_hw_init(struct camif_dev *camif, 
struct camif_vp *vp)
  	camif_hw_set_source_format(camif);
  	camif_hw_set_camera_crop(camif);
  	camif_hw_set_test_pattern(camif, camif->test_pattern->val);
+	if (ip_rev >= S3C2450_CAMIF_IP_REV)
+		camif_hw_set_effect(camif, camif->effect->val,
+				camif->effect_cr->val, camif->effect_cb->val);
  	if (ip_rev == S3C6410_CAMIF_IP_REV)
  		camif_hw_set_input_path(vp);
  	camif_cfg_video_path(vp);
@@ -108,8 +111,8 @@ static int s3c_camif_hw_vp_init(struct camif_dev 
*camif, struct camif_vp *vp)
  	if (ip_rev == S3C244X_CAMIF_IP_REV)
  		camif_hw_clear_fifo_overflow(vp);
  	camif_cfg_video_path(vp);
-	if (ip_rev == S3C6410_CAMIF_IP_REV)
-		camif_hw_set_effect(vp, false);
+	if (ip_rev >= S3C2450_CAMIF_IP_REV)
+		camif_hw_set_effect(camif, 0, 0, 0);
  	vp->state &= ~ST_VP_CONFIG;

  	spin_unlock_irqrestore(&camif->slock, flags);
@@ -374,6 +377,10 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
  		camif_hw_set_scaler(vp);
  		camif_hw_set_flip(vp);
  		camif_hw_set_test_pattern(camif, camif->test_pattern->val);
+		if (ip_rev >= S3C2450_CAMIF_IP_REV)
+			camif_hw_set_effect(camif, camif->effect->val,
+					camif->effect_cr->val,
+					camif->effect_cb->val);
  		vp->state &= ~ST_VP_CONFIG;
  	}
  unlock:
@@ -1530,7 +1537,7 @@ static const struct v4l2_ctrl_ops 
s3c_camif_subdev_ctrl_ops = {
  	.s_ctrl	= s3c_camif_subdev_s_ctrl,
  };

-static const struct v4l2_ctrl_config s3c_camif_priv_ctrl = {
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_test = {
  	.ops	= &s3c_camif_subdev_ctrl_ops,
  	.id	= V4L2_CTRL_CLASS_USER | 0x1001,
  	.type	= V4L2_CTRL_TYPE_INTEGER,
@@ -1541,6 +1548,39 @@ static const struct v4l2_ctrl_config 
s3c_camif_priv_ctrl = {
  	.def	= 0,
  };

+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_eff = {
+	.ops	= &s3c_camif_subdev_ctrl_ops,
+	.id	= V4L2_CTRL_CLASS_USER | 0x1002,
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.name	= "Effect",
+	.min	= 0,
+	.max	= 5,
+	.step	= 1,
+	.def	= 0,
+};
+
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_eff_cb = {
+	.ops	= &s3c_camif_subdev_ctrl_ops,
+	.id	= V4L2_CTRL_CLASS_USER | 0x1003,
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.name	= "PAT_Cb",
+	.min	= 16,
+	.max	= 240,
+	.step	= 1,
+	.def	= 128,
+};
+
+static const struct v4l2_ctrl_config s3c_camif_priv_ctrl_eff_cr = {
+	.ops	= &s3c_camif_subdev_ctrl_ops,
+	.id	= V4L2_CTRL_CLASS_USER | 0x1004,
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.name	= "PAT_Cr",
+	.min	= 16,
+	.max	= 240,
+	.step	= 1,
+	.def	= 128,
+};

SN:
There is no need to create these 3 private controls, we have now standard
controls for this image effect. Can you rework it to use V4L2_CID_COLORFX
and V4L2_CID_COLORFX_CBCR controls ? Not used option of V4L2_CID_COLORFX
can be easily masked off by passing proper mask to v4l2_ctrl_new_std().
Unfortunately the CB/CR coefficients will have fixed min and max values
then - 0, 255. That shouldn't be a big issue though.

AFAICT the range for CB/CR depends on the CSCR2Y_c bit which means:

"YCbCr Data Dynamic Range Selection for the Color Space Conversion RGB
to YCbCr"

CSCR2Y_c	1 : Wide => Y/Cb/Cr (0 ~ 255) : Wide default
		0 : Narrow => Y (16 ~ 235), Cb/Cr (16 ~ 240)

By default CSCR2Y_c bit is set so we get 0 ~ 255 range.


  int s3c_camif_create_subdev(struct camif_dev *camif)
  {
  	struct v4l2_ctrl_handler *handler = &camif->ctrl_handler;
@@ -1560,10 +1600,18 @@ int s3c_camif_create_subdev(struct camif_dev *camif)
  	if (ret)
  		return ret;

-	v4l2_ctrl_handler_init(handler, 1);
+	v4l2_ctrl_handler_init(handler, 4);
  	camif->test_pattern = v4l2_ctrl_new_custom(handler,
-					&s3c_camif_priv_ctrl, NULL);
+					&s3c_camif_priv_ctrl_test, NULL);
+	camif->effect = v4l2_ctrl_new_custom(handler,
+					&s3c_camif_priv_ctrl_eff, NULL);
+	camif->effect_cr = v4l2_ctrl_new_custom(handler,
+					&s3c_camif_priv_ctrl_eff_cb, NULL);
+	camif->effect_cb = v4l2_ctrl_new_custom(handlerEffect,
+					&s3c_camif_priv_ctrl_eff_cr, NULL);
+
  	if (handler->error) {
+		v4l2_ctrl_handler_free(handler);
  		media_entity_cleanup(&sd->entity);
  		return handler->error;
  	}
diff --git a/drivers/media/platform/s3c-camif/camif-core.h 
b/drivers/media/platform/s3c-camif/camif-core.h
index 96f5d3d..5f9eb3a 100644
--- a/drivers/media/platform/s3c-camif/camif-core.h
+++ b/drivers/media/platform/s3c-camif/camif-core.h
@@ -39,6 +39,8 @@
  #define CAMIF_STOP_TIMEOUT	1500 /* ms */

  #define S3C244X_CAMIF_IP_REV	0x20 /* 2.0 */
+#define S3C2450_CAMIF_IP_REV	0x30 /* 3.0 - not implemented, not tested */
+#define S3C6400_CAMIF_IP_REV	0x31 /* 3.1 - not implemented, not tested */
  #define S3C6410_CAMIF_IP_REV	0x32 /* 3.2 */

  /* struct camif_vp::state */
@@ -277,6 +279,9 @@ struct camif_dev {

  	struct v4l2_ctrl_handler	ctrl_handler;
  	struct v4l2_ctrl		*test_pattern;
+	struct v4l2_ctrl		*effect;
+	struct v4l2_ctrl		*effect_cb;
+	struct v4l2_ctrl		*effect_cr;

As explained above 2 controls can be used instead.

  	struct camif_vp			vp[CAMIF_VP_NUM];
  	struct vb2_alloc_ctx		*alloc_ctx;
diff --git a/drivers/media/platform/s3c-camif/camif-regs.c 
b/drivers/media/platform/s3c-camif/camif-regs.c
index d8c55dc..1b03f73 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.c
+++ b/drivers/media/platform/s3c-camif/camif-regs.c
@@ -57,6 +57,33 @@ void camif_hw_set_test_pattern(struct camif_dev 
*camif, unsigned int pattern)
  	camif_write(camif, S3C_CAMIF_REG_CIGCTRL, cfg);
  }

+void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
+			unsigned int cr, unsigned int cb)
+{
+	u32 cfg;
+
+	if (camif->variant->ip_revision < S3C2450_CAMIF_IP_REV)
+		return;

I think you can remove this check. Since camif_hw_set_effect() is always
called conditionally.

+
+	cfg = camif_read(camif, S3C_CAMIF_REG_CIIMGEFF(camif->vp->offset));
+	/* Set effect */
+	cfg &= ~CIIMGEFF_FIN_MASK;
+	cfg |= (effect << 26);
+	/* Set both paths */
+	if (camif->variant->ip_revision >= S3C6400_CAMIF_IP_REV) {
+		if (effect)
+			cfg |= CIIMGEFF_IE_ENABLE_MASK;
+		else
+			cfg &= ~CIIMGEFF_IE_ENABLE_MASK;
+	}
+	/* Set Cr, Cb */
+	if (effect == CIIMGEFF_FIN_ARBITRARY) {
+		cfg &= ~CIIMGEFF_PAT_CBCR_MASK;
+		cfg |= ((cb & 0xFF) << 13) | (cr & 0xFF);

Can you please use lower case for hex numbers ?

+	}
+	camif_write(camif, S3C_CAMIF_REG_CIIMGEFF(camif->vp->offset), cfg);
+}
+
  static const u32 src_pixfmt_map[8][2] = {
  	{ V4L2_MBUS_FMT_YUYV8_2X8, CISRCFMT_ORDER422_YCBYCR },
  	{ V4L2_MBUS_FMT_YVYU8_2X8, CISRCFMT_ORDER422_YCRYCB },
@@ -473,17 +500,6 @@ void camif_hw_set_lastirq(struct camif_vp *vp, int 
enable)
  	camif_write(vp->camif, addr, cfg);
  }

-void camif_hw_set_effect(struct camif_vp *vp, bool active)
-{
-	u32 cfg = 0;
-
-	if (active) {
-		/* TODO: effects support on 64xx */
-	}
-
-	camif_write(vp->camif, S3C_CAMIF_REG_CIIMGEFF, cfg);
-}
-
  void camif_hw_enable_capture(struct camif_vp *vp)
  {
  	struct camif_dev *camif = vp->camif;
diff --git a/drivers/media/platform/s3c-camif/camif-regs.h 
b/drivers/media/platform/s3c-camif/camif-regs.h
index a3488ca..213adbc 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.h
+++ b/drivers/media/platform/s3c-camif/camif-regs.h
@@ -177,8 +177,9 @@
  #define S3C_CAMIF_REG_CICPTSEQ			0xc4

  /* Image effects */
-#define S3C_CAMIF_REG_CIIMGEFF			0xd0
+#define S3C_CAMIF_REG_CIIMGEFF(_offs)		(0xb0 + (_offs))
  #define  CIIMGEFF_IE_ENABLE(id)			(1 << (30 + (id)))
+#define  CIIMGEFF_IE_ENABLE_MASK		(3 << 30)
  /* Image effect: 1 - after scaler, 0 - before scaler */
  #define  CIIMGEFF_IE_AFTER_SC			(1 << 29)
  #define  CIIMGEFF_FIN_MASK			(7 << 26)
@@ -243,7 +244,6 @@ void camif_hw_clear_fifo_overflow(struct camif_vp *vp);
  void camif_hw_set_lastirq(struct camif_vp *vp, int enable);
  void camif_hw_set_input_path(struct camif_vp *vp);
  void camif_hw_enable_scaler(struct camif_vp *vp, bool on);
-void camif_hw_set_effect(struct camif_vp *vp, bool active);
  void camif_hw_enable_capture(struct camif_vp *vp);
  void camif_hw_disable_capture(struct camif_vp *vp);
  void camif_hw_set_camera_bus(struct camif_dev *camif);
@@ -254,6 +254,8 @@ void camif_hw_set_flip(struct camif_vp *vp);
  void camif_hw_set_output_dma(struct camif_vp *vp);
  void camif_hw_set_target_format(struct camif_vp *vp);
  void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int 
pattern);
+void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
+			unsigned int cr, unsigned int cb);
  void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr 
*paddr,
  			      int index);
  void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
-- 1.7.0.4


Otherwise looks good.

--
Thanks,
Sylwester

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Andrey Gusakov]

[-- Attachment #1: Type: text/plain, Size: 3262 bytes --]

Hi.

Patch v2 attached. Comments taken into account.

>> I often get "VIDIOC_QUERYCAP: failed: Inappropriate ioctl for device"
> This is an issue in the v4l2-ctl, it is going to be fixed by adding
> VIDIOC_SUBDEV_QUERYCAP ioctl for subdevs. It has been just discussed today.
> I guess you get it when running v4l2-ctl on /dev/v4l-subdev* ?
Yes.
>> or "system error: Inappropriate ioctl for device"
> I think this one is caused by unimplemented VIDIOC_G/S_PARM ioctls
> at the s3c-camif driver.
>> Is it because of not implemented set/get framerate func? How this
> Yes, I think so. ioctls as above.
Ok. I'll implement this ioctls and see what happens.

>> should work? I mean framerate heavy depend of sensor's settings. So
>> set/get framerate call to fimc should get/set framerate from sensor.
>> What is mechanism of such things?
>
>
> With user space subdev API one should control frame interval directly
> on the sensor subdev device node [1]. For Gstreamer to work with
> VIDIOC_G/S_PARM ioctls we need a dedicated v4l2 library (possibly with
> a plugin for s3c-camif, but that shouldn't be needed since it is very
> simple driver) that will translate those video node ioctls into the
> subdev node ioctls [2]. Unfortunately such library is still not available.
>
>
>> And same question about synchronizing format of sensor and FIMC pads.
>> I make ov2640 work, but if did not call media-ctl for sensor, format
>> of FIMC sink pad and format of sensor source pad different. I think I
>> missed something, but reading other sources did not help.
>
>
> As I explained previously, s3c-fimc is supposed to synchronize format
> with the sensor subdev. Have you got pad level get_fmt callback
> implemented in the ov2640 driver ?
Yes.
> Could you post your 'media-ctl -p' output, run right after the system boot ?
Looks like I messed up, after starting formats are the same:

Opening media device /dev/media0ov2640: ov2640_open:1381

Enumerating entities
Found 4 entities
Enumerating pads and links
Media controller API version 0.0.0

Media device information
------------------------
driver          s3c-camif
model           SAMSUNG S3C6410 CAMIF
serial
bus info        platform:%s
hw revision     0x32
driver version  0.0.0

Device topology
- entity 1: ov2640 (1 pad, 1 link)
            type V4L2 subdev subtype Sensor
            device node name /dev/v4l-subdev0
        pad0: Source [YUYV2X8 176x144]
                -> "S3C-CAMIF":0 [ENABLED,IMMUTABLE]

- entity 2: S3C-CAMIF (3 pads, 3 links)
            type V4L2 subdev subtype Unknown
            device node name /dev/v4l-subdev1
        pad0: Sink [YUYV2X8 176x144 (0,0)/176x144]
                <- "ov2640":0 [ENABLED,IMMUTABLE]
        pad1: Source [YUYV2X8 176x144]
                -> "camif-codec":0 [ENABLED,IMMUTABLE]
        pad2: Source [YUYV2X8 176x144]
                -> "camif-preview":0 [ENABLED,IMMUTABLE]

- entity 3: camif-codec (1 pad, 1 link)
            type Node subtype V4L
            device node name /dev/video0
        pad0: Sink
                <- "S3C-CAMIF":1 [ENABLED,IMMUTABLE]

- entity 4: camif-preview (1 pad, 1 link)
            type Node subtype V4L
            device node name /dev/video1
        pad0: Sink
                <- "S3C-CAMIF":2 [ENABLED,IMMUTABLE]

[-- Attachment #2: 0001-ARM-S3C-CAMIF-add-image-effect-controls.patch --]
[-- Type: application/octet-stream, Size: 8027 bytes --]

From 52d5f814a3ea197a3968dc0317696316f21097d4 Mon Sep 17 00:00:00 2001
From: Andrey Gusakov <dron_gus@mail.ru>
Date: Mon, 5 Nov 2012 15:50:23 +0400
Subject: [PATCH] ARM: S3C-CAMIF: add image effect controls

Some Samsung SoC have image effect function on camera imterface.
Use standart v4l2 controls for enabling and adjusting this
effects. On s3c64XX effects enabled for both capture and preview
channels for compatibility with s3c2450.

Signed-off-by: Andrey Gusakov <dron0gus@gmail.com>
---
 drivers/media/platform/s3c-camif/camif-capture.c |   23 +++++++-
 drivers/media/platform/s3c-camif/camif-core.h    |    4 ++
 drivers/media/platform/s3c-camif/camif-regs.c    |   63 ++++++++++++++++++----
 drivers/media/platform/s3c-camif/camif-regs.h    |    6 ++-
 4 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index ca31c45..429604e 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -81,6 +81,10 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
 	camif_hw_set_source_format(camif);
 	camif_hw_set_camera_crop(camif);
 	camif_hw_set_test_pattern(camif, camif->test_pattern->val);
+	if (ip_rev >= S3C2450_CAMIF_IP_REV)
+		camif_hw_set_effect(camif, camif->colorfx->val,
+				camif->colorfx_cbcr->val & 0xff,
+				camif->colorfx_cbcr->val >> 8);
 	if (ip_rev == S3C6410_CAMIF_IP_REV)
 		camif_hw_set_input_path(vp);
 	camif_cfg_video_path(vp);
@@ -108,8 +112,8 @@ static int s3c_camif_hw_vp_init(struct camif_dev *camif, struct camif_vp *vp)
 	if (ip_rev == S3C244X_CAMIF_IP_REV)
 		camif_hw_clear_fifo_overflow(vp);
 	camif_cfg_video_path(vp);
-	if (ip_rev == S3C6410_CAMIF_IP_REV)
-		camif_hw_set_effect(vp, false);
+	if (ip_rev >= S3C2450_CAMIF_IP_REV)
+		camif_hw_set_effect(camif, 0, 0, 0);
 	vp->state &= ~ST_VP_CONFIG;
 
 	spin_unlock_irqrestore(&camif->slock, flags);
@@ -374,6 +378,10 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
 		camif_hw_set_scaler(vp);
 		camif_hw_set_flip(vp);
 		camif_hw_set_test_pattern(camif, camif->test_pattern->val);
+		if (ip_rev >= S3C2450_CAMIF_IP_REV)
+			camif_hw_set_effect(camif, camif->colorfx->val,
+					camif->colorfx_cbcr->val & 0xff,
+					camif->colorfx_cbcr->val >> 8);
 		vp->state &= ~ST_VP_CONFIG;
 	}
 unlock:
@@ -1560,10 +1568,19 @@ int s3c_camif_create_subdev(struct camif_dev *camif)
 	if (ret)
 		return ret;
 
-	v4l2_ctrl_handler_init(handler, 1);
+	v4l2_ctrl_handler_init(handler, 3);
 	camif->test_pattern = v4l2_ctrl_new_custom(handler,
 					&s3c_camif_priv_ctrl, NULL);
+	camif->colorfx = v4l2_ctrl_new_std_menu(handler,
+				&s3c_camif_subdev_ctrl_ops,
+				V4L2_CID_COLORFX, CIIMGEFF_FIN_SILHOUETTE,
+				~0x981F, V4L2_COLORFX_NONE);
+
+	camif->colorfx_cbcr = v4l2_ctrl_new_std(handler,
+				&s3c_camif_subdev_ctrl_ops,
+				V4L2_CID_COLORFX_CBCR, 0, 0xffff, 1, 0);
 	if (handler->error) {
+		v4l2_ctrl_handler_free(handler);
 		media_entity_cleanup(&sd->entity);
 		return handler->error;
 	}
diff --git a/drivers/media/platform/s3c-camif/camif-core.h b/drivers/media/platform/s3c-camif/camif-core.h
index 96f5d3d..ec62936 100644
--- a/drivers/media/platform/s3c-camif/camif-core.h
+++ b/drivers/media/platform/s3c-camif/camif-core.h
@@ -39,6 +39,8 @@
 #define CAMIF_STOP_TIMEOUT	1500 /* ms */
 
 #define S3C244X_CAMIF_IP_REV	0x20 /* 2.0 */
+#define S3C2450_CAMIF_IP_REV	0x30 /* 3.0 - not implemented, not tested */
+#define S3C6400_CAMIF_IP_REV	0x31 /* 3.1 - not implemented, not tested */
 #define S3C6410_CAMIF_IP_REV	0x32 /* 3.2 */
 
 /* struct camif_vp::state */
@@ -277,6 +279,8 @@ struct camif_dev {
 
 	struct v4l2_ctrl_handler	ctrl_handler;
 	struct v4l2_ctrl		*test_pattern;
+	struct v4l2_ctrl		*colorfx;
+	struct v4l2_ctrl		*colorfx_cbcr;
 
 	struct camif_vp			vp[CAMIF_VP_NUM];
 	struct vb2_alloc_ctx		*alloc_ctx;
diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c
index d8c55dc..ecdc99f 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.c
+++ b/drivers/media/platform/s3c-camif/camif-regs.c
@@ -57,6 +57,58 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
 	camif_write(camif, S3C_CAMIF_REG_CIGCTRL, cfg);
 }
 
+void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
+			unsigned int cr, unsigned int cb)
+{
+	u32 cfg;
+	static const struct v4l2_control colorfx[] = {
+		{ V4L2_COLORFX_NONE,		CIIMGEFF_FIN_BYPASS },
+		{ V4L2_COLORFX_BW,		CIIMGEFF_FIN_ARBITRARY },
+		{ V4L2_COLORFX_SEPIA,		CIIMGEFF_FIN_ARBITRARY },
+		{ V4L2_COLORFX_NEGATIVE,	CIIMGEFF_FIN_NEGATIVE },
+		{ V4L2_COLORFX_ART_FREEZE,	CIIMGEFF_FIN_ARTFREEZE },
+		{ V4L2_COLORFX_EMBOSS,		CIIMGEFF_FIN_EMBOSSING },
+		{ V4L2_COLORFX_SILHOUETTE,	CIIMGEFF_FIN_SILHOUETTE },
+		{ V4L2_COLORFX_SET_CBCR,	CIIMGEFF_FIN_ARBITRARY },
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(colorfx); i++) {
+		if (colorfx[i].id == effect)
+			break;
+	}
+	if (i == ARRAY_SIZE(colorfx))
+		return;
+
+	cfg = camif_read(camif, S3C_CAMIF_REG_CIIMGEFF(camif->vp->offset));
+	/* Set effect */
+	cfg &= ~CIIMGEFF_FIN_MASK;
+	cfg |= colorfx[i].value;
+	/* Set both paths */
+	if (camif->variant->ip_revision >= S3C6400_CAMIF_IP_REV) {
+		if (effect != V4L2_COLORFX_NONE)
+			cfg |= CIIMGEFF_IE_ENABLE_MASK;
+		else
+			cfg &= ~CIIMGEFF_IE_ENABLE_MASK;
+	}
+	/* Set Cr, Cb */
+	if (effect == V4L2_COLORFX_SET_CBCR) {
+		/* nop */
+	}
+	else if (effect == V4L2_COLORFX_SEPIA) {
+		cb = 115;
+		cr = 145;
+	}
+	else {
+		/* for V4L2_COLORFX_BW and others */
+		cb = 128;
+		cr = 128;
+	}
+	cfg &= ~CIIMGEFF_PAT_CBCR_MASK;
+	cfg |= cr | (cb << 13);
+	camif_write(camif, S3C_CAMIF_REG_CIIMGEFF(camif->vp->offset), cfg);
+}
+
 static const u32 src_pixfmt_map[8][2] = {
 	{ V4L2_MBUS_FMT_YUYV8_2X8, CISRCFMT_ORDER422_YCBYCR },
 	{ V4L2_MBUS_FMT_YVYU8_2X8, CISRCFMT_ORDER422_YCRYCB },
@@ -473,17 +525,6 @@ void camif_hw_set_lastirq(struct camif_vp *vp, int enable)
 	camif_write(vp->camif, addr, cfg);
 }
 
-void camif_hw_set_effect(struct camif_vp *vp, bool active)
-{
-	u32 cfg = 0;
-
-	if (active) {
-		/* TODO: effects support on 64xx */
-	}
-
-	camif_write(vp->camif, S3C_CAMIF_REG_CIIMGEFF, cfg);
-}
-
 void camif_hw_enable_capture(struct camif_vp *vp)
 {
 	struct camif_dev *camif = vp->camif;
diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h
index a3488ca..213adbc 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.h
+++ b/drivers/media/platform/s3c-camif/camif-regs.h
@@ -177,8 +177,9 @@
 #define S3C_CAMIF_REG_CICPTSEQ			0xc4
 
 /* Image effects */
-#define S3C_CAMIF_REG_CIIMGEFF			0xd0
+#define S3C_CAMIF_REG_CIIMGEFF(_offs)		(0xb0 + (_offs))
 #define  CIIMGEFF_IE_ENABLE(id)			(1 << (30 + (id)))
+#define  CIIMGEFF_IE_ENABLE_MASK		(3 << 30)
 /* Image effect: 1 - after scaler, 0 - before scaler */
 #define  CIIMGEFF_IE_AFTER_SC			(1 << 29)
 #define  CIIMGEFF_FIN_MASK			(7 << 26)
@@ -243,7 +244,6 @@ void camif_hw_clear_fifo_overflow(struct camif_vp *vp);
 void camif_hw_set_lastirq(struct camif_vp *vp, int enable);
 void camif_hw_set_input_path(struct camif_vp *vp);
 void camif_hw_enable_scaler(struct camif_vp *vp, bool on);
-void camif_hw_set_effect(struct camif_vp *vp, bool active);
 void camif_hw_enable_capture(struct camif_vp *vp);
 void camif_hw_disable_capture(struct camif_vp *vp);
 void camif_hw_set_camera_bus(struct camif_dev *camif);
@@ -254,6 +254,8 @@ void camif_hw_set_flip(struct camif_vp *vp);
 void camif_hw_set_output_dma(struct camif_vp *vp);
 void camif_hw_set_target_format(struct camif_vp *vp);
 void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
+void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
+			unsigned int cr, unsigned int cb);
 void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
 			      int index);
 void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
-- 
1.7.0.4

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

Hi Andrey,

On 11/11/2012 02:26 PM, Andrey Gusakov wrote:
> Hi.
>
> Patch v2 attached. Comments taken into account.

Thanks, I had to rework the S3C-CAMIF subdev controls handling to avoid
races when the control's value is modified in the control framwework and
accessed from interrupt context in the driver.

I've pushed all patches to branch s3c-camif-for-v3.8 at
git://linuxtv.org/snawrocki/media.git

I'd like to squash all the s3c-camif patches before sending upstream,
if you don't mind. And to add your Signed-off at the final patch.
Please let me know if you'd rather keep your patch separate.

I might have introduced bugs in the image effects handling, hopefully
there is none. I couldn't test it though. Could you test that on your
side with s3c64xx ?

I'm planning to send a pull request for the CAMIF driver this week.

>>> I often get "VIDIOC_QUERYCAP: failed: Inappropriate ioctl for device"
>> This is an issue in the v4l2-ctl, it is going to be fixed by adding
>> VIDIOC_SUBDEV_QUERYCAP ioctl for subdevs. It has been just discussed today.
>> I guess you get it when running v4l2-ctl on /dev/v4l-subdev* ?
> Yes.
>>> or "system error: Inappropriate ioctl for device"
>> I think this one is caused by unimplemented VIDIOC_G/S_PARM ioctls
>> at the s3c-camif driver.
>>> Is it because of not implemented set/get framerate func? How this
>> Yes, I think so. ioctls as above.
> Ok. I'll implement this ioctls and see what happens.

They are not supposed to be implemented in the s3c-camif driver.
Instead they should be emulated by a user-space library, based on
the sensor subdev operations.

>>> should work? I mean framerate heavy depend of sensor's settings. So
>>> set/get framerate call to fimc should get/set framerate from sensor.
>>> What is mechanism of such things?
>>
>>
>> With user space subdev API one should control frame interval directly
>> on the sensor subdev device node [1]. For Gstreamer to work with
>> VIDIOC_G/S_PARM ioctls we need a dedicated v4l2 library (possibly with
>> a plugin for s3c-camif, but that shouldn't be needed since it is very
>> simple driver) that will translate those video node ioctls into the
>> subdev node ioctls [2]. Unfortunately such library is still not available.
>>
>>
>>> And same question about synchronizing format of sensor and FIMC pads.
>>> I make ov2640 work, but if did not call media-ctl for sensor, format
>>> of FIMC sink pad and format of sensor source pad different. I think I
>>> missed something, but reading other sources did not help.
>>
>>
>> As I explained previously, s3c-fimc is supposed to synchronize format
>> with the sensor subdev. Have you got pad level get_fmt callback
>> implemented in the ov2640 driver ?
> Yes.
>> Could you post your 'media-ctl -p' output, run right after the system boot ?
>
> Looks like I messed up, after starting formats are the same:
>
> Opening media device /dev/media0ov2640: ov2640_open:1381
>
> Enumerating entities
> Found 4 entities
> Enumerating pads and links
> Media controller API version 0.0.0
>
> Media device information
> ------------------------
> driver          s3c-camif
> model           SAMSUNG S3C6410 CAMIF
> serial
> bus info        platform:%s

I've removed this stray "%s" already.

> hw revision     0x32
> driver version  0.0.0
>
> Device topology
> - entity 1: ov2640 (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor
>              device node name /dev/v4l-subdev0
>          pad0: Source [YUYV2X8 176x144]
>                  ->  "S3C-CAMIF":0 [ENABLED,IMMUTABLE]
>
> - entity 2: S3C-CAMIF (3 pads, 3 links)
>              type V4L2 subdev subtype Unknown
>              device node name /dev/v4l-subdev1
>          pad0: Sink [YUYV2X8 176x144 (0,0)/176x144]
>                  <- "ov2640":0 [ENABLED,IMMUTABLE]
>          pad1: Source [YUYV2X8 176x144]
>                  ->  "camif-codec":0 [ENABLED,IMMUTABLE]
>          pad2: Source [YUYV2X8 176x144]
>                  ->  "camif-preview":0 [ENABLED,IMMUTABLE]
>
> - entity 3: camif-codec (1 pad, 1 link)
>              type Node subtype V4L
>              device node name /dev/video0
>          pad0: Sink
>                  <- "S3C-CAMIF":1 [ENABLED,IMMUTABLE]
>
> - entity 4: camif-preview (1 pad, 1 link)
>              type Node subtype V4L
>              device node name /dev/video1
>          pad0: Sink
>                  <- "S3C-CAMIF":2 [ENABLED,IMMUTABLE]

Looks fine, thanks.

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Andrey Gusakov]

Hi.

Just have time to test. I apologize for delay.

> I'd like to squash all the s3c-camif patches before sending upstream,
> if you don't mind. And to add your Signed-off at the final patch.
Ok. Squash.

> I might have introduced bugs in the image effects handling, hopefully
> there is none. I couldn't test it though. Could you test that on your
> side with s3c64xx ?
Got some error. Seems effect updated only when I set new CrCb value.
Seems it's incorrect:
	case V4L2_CID_COLORFX:
		if (camif->ctrl_colorfx_cbcr->is_new) {
			camif->colorfx = camif->ctrl_colorfx->val;
			/* Set Cb, Cr */
			switch (ctrl->val) {
			case V4L2_COLORFX_SEPIA:
				camif->ctrl_colorfx_cbcr->val = 0x7391;
				break;
			case V4L2_COLORFX_SET_CBCR: /* noop */
				break;
			default:
				/* for V4L2_COLORFX_BW and others */
				camif->ctrl_colorfx_cbcr->val = 0x8080;
			}
		}
		camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val & 0xff;
		camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val >> 8;
		break;
Moving "camif->colorfx = camif->ctrl_colorfx->val;" out of condition
fixes the problem, but setting CrCb value control affect all effects
(sepia and BW), not only V4L2_COLORFX_SET_CBCR. Seems condition should
be removed and colorfx value should be checked set on every effect
change.

diff --git a/drivers/media/platform/s3c-camif/camif-capture.c
b/drivers/media/platform/s3c-camif/camif-capture.c
index ceab03a..9c01f4f 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -1526,19 +1526,17 @@ static int s3c_camif_subdev_s_ctrl(struct
v4l2_ctrl *ctrl)

 	switch (ctrl->id) {
 	case V4L2_CID_COLORFX:
-		if (camif->ctrl_colorfx_cbcr->is_new) {
-			camif->colorfx = camif->ctrl_colorfx->val;
-			/* Set Cb, Cr */
-			switch (ctrl->val) {
-			case V4L2_COLORFX_SEPIA:
-				camif->ctrl_colorfx_cbcr->val = 0x7391;
-				break;
-			case V4L2_COLORFX_SET_CBCR: /* noop */
-				break;
-			default:
-				/* for V4L2_COLORFX_BW and others */
-				camif->ctrl_colorfx_cbcr->val = 0x8080;
-			}
+		camif->colorfx = camif->ctrl_colorfx->val;
+		/* Set Cb, Cr */
+		switch (ctrl->val) {
+		case V4L2_COLORFX_SEPIA:
+			camif->ctrl_colorfx_cbcr->val = 0x7391;
+			break;
+		case V4L2_COLORFX_SET_CBCR: /* noop */
+			break;
+		default:
+			/* for V4L2_COLORFX_BW and others */
+			camif->ctrl_colorfx_cbcr->val = 0x8080;
 		}
 		camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val & 0xff;
 		camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val >> 8;

With this modification got another issue: set CRCB effect, set CRCB
value, set BW effect, set CRCB effect back cause CRCB-value to be
reseted to default 0x8080. Is it correct?

Re: S3C244X/S3C64XX SoC camera host interface driver questions

[Sylwester Nawrocki]

Hi,

On 11/17/2012 01:07 PM, Andrey Gusakov wrote:
> Hi.
>
> Just have time to test. I apologize for delay.

No problem, thanks for the feedback.

>> I'd like to squash all the s3c-camif patches before sending upstream,
>> if you don't mind. And to add your Signed-off at the final patch.
> Ok. Squash.
>
>> I might have introduced bugs in the image effects handling, hopefully
>> there is none. I couldn't test it though. Could you test that on your
>> side with s3c64xx ?
> Got some error. Seems effect updated only when I set new CrCb value.
> Seems it's incorrect:
> 	case V4L2_CID_COLORFX:
> 		if (camif->ctrl_colorfx_cbcr->is_new) {

Uh, copy/paste error, this should have been

		if (camif->ctrl_colorfx->is_new) {

> 			camif->colorfx = camif->ctrl_colorfx->val;
> 			/* Set Cb, Cr */
> 			switch (ctrl->val) {
> 			case V4L2_COLORFX_SEPIA:
> 				camif->ctrl_colorfx_cbcr->val = 0x7391;
> 				break;
> 			case V4L2_COLORFX_SET_CBCR: /* noop */
> 				break;
> 			default:
> 				/* for V4L2_COLORFX_BW and others */
> 				camif->ctrl_colorfx_cbcr->val = 0x8080;
> 			}
> 		}
> 		camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val&  0xff;
> 		camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val>>  8;
> 		break;
> Moving "camif->colorfx = camif->ctrl_colorfx->val;" out of condition
> fixes the problem, but setting CrCb value control affect all effects
> (sepia and BW), not only V4L2_COLORFX_SET_CBCR. Seems condition should
> be removed and colorfx value should be checked set on every effect
> change.
>
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c
> b/drivers/media/platform/s3c-camif/camif-capture.c
> index ceab03a..9c01f4f 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -1526,19 +1526,17 @@ static int s3c_camif_subdev_s_ctrl(struct
> v4l2_ctrl *ctrl)
>
>   	switch (ctrl->id) {
>   	case V4L2_CID_COLORFX:
> -		if (camif->ctrl_colorfx_cbcr->is_new) {
> -			camif->colorfx = camif->ctrl_colorfx->val;
> -			/* Set Cb, Cr */
> -			switch (ctrl->val) {
> -			case V4L2_COLORFX_SEPIA:
> -				camif->ctrl_colorfx_cbcr->val = 0x7391;
> -				break;
> -			case V4L2_COLORFX_SET_CBCR: /* noop */
> -				break;
> -			default:
> -				/* for V4L2_COLORFX_BW and others */
> -				camif->ctrl_colorfx_cbcr->val = 0x8080;
> -			}
> +		camif->colorfx = camif->ctrl_colorfx->val;
> +		/* Set Cb, Cr */
> +		switch (ctrl->val) {
> +		case V4L2_COLORFX_SEPIA:
> +			camif->ctrl_colorfx_cbcr->val = 0x7391;
> +			break;
> +		case V4L2_COLORFX_SET_CBCR: /* noop */
> +			break;
> +		default:
> +			/* for V4L2_COLORFX_BW and others */
> +			camif->ctrl_colorfx_cbcr->val = 0x8080;
>   		}
>   		camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val&  0xff;
>   		camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val>>  8;
>
> With this modification got another issue: set CRCB effect, set CRCB
> value, set BW effect, set CRCB effect back cause CRCB-value to be
> reseted to default 0x8080. Is it correct?

We could do better. The control values are already cached twice - in
struct v4l2_ctrl and struct camif_dev.

It seems more intuitive to save CB/CR coefficients for V4L2_COLORFX_SET_CBCR
and then restore them as needed. There is probably not much use of letting
user space know that the driver uses CBCR for V4L2_COLORFX_SEPIA and
V4L2_COLORFX_BW internally.

I propose change as below, it includes disabling the control for SoCs that
don't support it and a fixed cbcr order, to match documentation:

"V4L2_CID_COLORFX_CBCR	integer	Determines the Cb and Cr coefficients for
V4L2_COLORFX_SET_CBCR color effect. Bits [7:0] of the supplied 32 bit 
value are
interpreted as Cr component, bits [15:8] as Cb component and bits 
[31:16] must
be zero."

I have pushed it all to the testing/s3c-camif branch. Please let me know
if you find any further issues.

-----8<-------
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c 
b/drivers/media/platform/s3c-camif/camif-capture.c
index 6401fdb..b52cc59 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -1520,22 +1520,22 @@ static int s3c_camif_subdev_s_ctrl(struct 
v4l2_ctrl *ctrl)

  	switch (ctrl->id) {
  	case V4L2_CID_COLORFX:
-		if (camif->ctrl_colorfx_cbcr->is_new) {
-			camif->colorfx = camif->ctrl_colorfx->val;
-			/* Set Cb, Cr */
-			switch (ctrl->val) {
-			case V4L2_COLORFX_SEPIA:
-				camif->ctrl_colorfx_cbcr->val = 0x7391;
-				break;
-			case V4L2_COLORFX_SET_CBCR: /* noop */
-				break;
-			default:
-				/* for V4L2_COLORFX_BW and others */
-				camif->ctrl_colorfx_cbcr->val = 0x8080;
-			}
+		camif->colorfx = camif->ctrl_colorfx->val;
+		/* Set Cb, Cr */
+		switch (ctrl->val) {
+		case V4L2_COLORFX_SEPIA:
+			camif->colorfx_cb = 115;
+			camif->colorfx_cr = 145;
+			break;
+		case V4L2_COLORFX_SET_CBCR:
+			camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val >> 8;
+			camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val & 0xff;
+			break;
+		default:
+			/* for V4L2_COLORFX_BW and others */
+			camif->colorfx_cb = 128;
+			camif->colorfx_cr = 128;
  		}
-		camif->colorfx_cb = camif->ctrl_colorfx_cbcr->val & 0xff;
-		camif->colorfx_cr = camif->ctrl_colorfx_cbcr->val >> 8;
  		break;
  	case V4L2_CID_TEST_PATTERN:
  		camif->test_pattern = camif->ctrl_test_pattern->val;
@@ -1603,6 +1603,10 @@ int s3c_camif_create_subdev(struct camif_dev *camif)

  	v4l2_ctrl_auto_cluster(2, &camif->ctrl_colorfx,
  			       V4L2_COLORFX_SET_CBCR, false);
+	if (!camif->variant->has_img_effect) {
+		camif->ctrl_colorfx->flags |= V4L2_CTRL_FLAG_DISABLED;
+		camif->ctrl_colorfx_cbcr->flags |= V4L2_CTRL_FLAG_DISABLED;
+	}
  	sd->ctrl_handler = handler;
  	v4l2_set_subdevdata(sd, camif);

----->8-------

Thanks,
Sylwester