Re: [PATCH v2 0/4] media: soc_camera: ov9640: switch driver to v4l2_async
Dne 14.9.2018 v 14:59 Sakari Ailus napsal(a): > Hi Petr, > > On Wed, Aug 15, 2018 at 03:30:23PM +0200, petrcve...@gmail.com wrote: >> From: Petr Cvek >> >> This patch series transfer the ov9640 driver from the soc_camera subsystem >> into a standalone v4l2 driver. There is no changes except the required >> v4l2_async calls, GPIO allocation, deletion of now unused variables, >> a change from mdelay() to msleep() and an addition of SPDX identifiers >> (as suggested in the v1 version RFC). >> >> The config symbol has been changed from CONFIG_SOC_CAMERA_OV9640 to >> CONFIG_VIDEO_OV9640. >> >> Also as the drivers of the soc_camera seems to be orphaned I'm volunteering >> as a maintainer of the driver (I own the hardware). > > Thanks for the set. The patches seem good to me as such but there's some > more work to do there. For one, the depedency to v4l2_clk should be > removed; the common clock framework has supported clocks from random > devices for many, many years now. > > The PXA camera driver does still depend on v4l2_clk so I guess this is > better to do later on in a different patchset. > Yeah I too would like to remove the v4l2_clk from both of them. We've had the discussion about clock dependency around v1 patch set: "[BUG, RFC] media: Wrong module gets acquired" cheers, Petr
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Dne 14.9.2018 v 14:59 Sakari Ailus napsal(a):
> Hi Petr,
>
> Thanks for the patchset, and my apologies for reviewing it so late!
>
> I'm commenting this one but feel free to add patches to address the
> comments.
>
Hi and thanks for the review. I would like to have this patch set to be
as much as possible only a conversion from soc-camera, but I guess I can
fix the error handling in probe and the missing newlines. For the
enhanced functionality, I would like to have a new patch set after I'll
patch the controller (pxa camera) on my testing platform.
>> +/* Start/Stop streaming from the device */
>> +static int ov9640_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +return 0;
>
> Doesn't the sensor provide any control over the streaming state? Just
> wondering...
>
Before the PXA camera switch from soc-camera I've found some
deficiencies in register settings so I'm planning to test them all. With
the current state of vanilla I wouldn't be able to test the change.
After the quick search in datasheet I wasn't able to find any (stream,
capture, start) flag. It may be controlled by just setting the output
format flags, but these registers are some of those I will be changing
in the future.
>> +static int ov9640_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> +struct ov9640_priv *priv = to_ov9640_sensor(sd);
>> +
>> +return soc_camera_set_power(>dev, ssdd, priv->clk, on);
>
> Runtime PM support would be nice --- but not needed in this set IMO.
>
If I remember correctly a suspend to mem will freeze the whole machine,
so in the future :-/.
>> +}
>> +
>> +/* select nearest higher resolution for capture */
>> +static void ov9640_res_roundup(u32 *width, u32 *height)
>> +{
>> +int i;
>
> unsigned int
>
> Same for other loops where no negative values or test below zero are
> needed (or where the value which is compared against is signed).
>
Just re-declaring: unsigned int i; ... OK
>> +
>> +cfg->try_fmt = *mf;
>
> Newline here?
>
>> +return 0;
>> +}
>> +
>> +static int ov9640_enum_mbus_code(struct v4l2_subdev *sd,
>> +struct v4l2_subdev_pad_config *cfg,
>> +struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +if (code->pad || code->index >= ARRAY_SIZE(ov9640_codes))
>> +return -EINVAL;
>> +
>> +code->code = ov9640_codes[code->index];
>
> And here.
>
np
>> +/* Request bus settings on camera side */
>> +static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
>> +struct v4l2_mbus_config *cfg)
>> +{
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> +
>> +cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>> +V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>> +V4L2_MBUS_DATA_ACTIVE_HIGH;
>
> This should come from DT instead. Could you add DT binding documentation
> for the sensor, please? There's already some for ov9650; I wonder how
> similar that one is.
The platform doesn't support it yet, so I have no way to test any DT
patches.
>> +cfg->type = V4L2_MBUS_PARALLEL;
>> +cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops ov9640_video_ops = {
>> +.s_stream = ov9640_s_stream,
>> +.g_mbus_config = ov9640_g_mbus_config,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops ov9640_pad_ops = {
>> +.enum_mbus_code = ov9640_enum_mbus_code,
>> +.get_selection = ov9640_get_selection,
>> +.set_fmt= ov9640_set_fmt,
>
> Please add an operating to get the format as well.
OK, but it will be tested on a preliminary hacked pxa-camera :-).
>
>> +};
>> +
>> +static const struct v4l2_subdev_ops ov9640_subdev_ops = {
>> +.core = _core_ops,
>> +.video = _video_ops,
>> +.pad= _pad_ops,
>> +};
>> +
>> +/*
>> + * i2c_driver function
>> + */
>> +static int ov9640_probe(struct i2c_client *client,
>> +const struct i2c_device_id *did)
>> +{
>> +struct ov9640_priv *priv;
>> +struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> +int ret;
>> +
>> +if (!ssdd) {
>> +dev_err(>dev, "Missing platform_data for driver\n");
>> +return -EINVAL;
>> +}
>> +
>> +priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
>> +if (!priv)
>> +return -ENOMEM;
>> +
>> +v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
>> +
>> +v4l2_ctrl_handler_init(>hdl, 2);
>> +v4l2_ctrl_new_std(>hdl, _ctrl_ops,
>> +V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +v4l2_ctrl_new_std(>hdl, _ctrl_ops,
>> +V4L2_CID_HFLIP, 0, 1, 1, 0);
>> +priv->subdev.ctrl_handler = >hdl;
>> +if
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Dne 16.8.2018 v 12:23 jacopo mondi napsal(a): > Hi Petr, > > On Wed, Aug 15, 2018 at 03:35:39PM +0200, Petr Cvek wrote: >> BTW from the v1 discussion: >> >> Dne 10.8.2018 v 09:32 jacopo mondi napsal(a): >>> When I've been recently doing the same for ov772x and other sensor >>> driver I've been suggested to first copy the driver into >>> drivers/media/i2c/ and leave the original soc_camera one there, so >>> they can be bulk removed or moved to staging. I'll let Hans confirm >>> this, as he's about to take care of this process. >> >> I would rather used git mv for preserve the git history, but if a simple >> copy is fine then I'm fine too ;-). > > Well, 'git mv' removes the soc_camera version of this driver, and my > understanding was that when those driver will be obsoleted they will > be bulk removed or moved to staging by Hans. > Nevermind I've probably used the wrong command :-/ Petr
Re: [BUG, RFC] media: Wrong module gets acquired
Dne 14.8.2018 v 15:03 Mauro Carvalho Chehab napsal(a): > Em Tue, 14 Aug 2018 11:35:01 +0300 > Sakari Ailus escreveu: > >> Hi Pert, >> >> On Mon, Aug 13, 2018 at 06:33:12PM +0200, petrcve...@gmail.com wrote: >>> From: Petr Cvek >>> >>> When transferring a media sensor driver from the soc_camera I've found >>> the controller module can get removed (which will cause a stack dump >>> because the sensor driver depends on resources from the controller driver). >>> >> >> There may be a kernel oops if a resource used by another driver goes away. >> But the right fix isn't to prevent unloading that module. Instead, one way >> to address the problem would be to have persistent clock objects that would >> not be dependent on the driver that provides them. >> >>> >>> When I've tried to remove the driver module of the sensor it said the >>> resource was busy (without a reference name) though is should be >>> possible to remove the sensor driver because it is at the end of >>> the dependency list and not to remove the controller driver. >> >> That might be one day possible but it is not today. >> >> You'll still need to acquire the sensor module as well as it registers a >> media entity as well as a sub-device. > > Let put my 2 cents here. > > Usually, the same problem of removing modules happen if you just > ask the driver's core to unbind a module (with can be done via > sysfs). > > Removing/unbinding a driver that uses media controller should work, > if the unbinding code at the driver (e. g. i2c_driver::remove field) > would delete the media controller entities, and the caller driver > doesn't cache it. > Yeah I assume it would be same as removing my sensor module from v4l2_async and unregistering v4l2 clocks. Petr
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
BTW from the v1 discussion: Dne 10.8.2018 v 09:32 jacopo mondi napsal(a): > When I've been recently doing the same for ov772x and other sensor > driver I've been suggested to first copy the driver into > drivers/media/i2c/ and leave the original soc_camera one there, so > they can be bulk removed or moved to staging. I'll let Hans confirm > this, as he's about to take care of this process. I would rather used git mv for preserve the git history, but if a simple copy is fine then I'm fine too ;-). Petr
Re: [BUG, RFC] media: Wrong module gets acquired
Dne 14.8.2018 v 10:35 Sakari Ailus napsal(a): > Hi Pert, > Hello, thanks for answering > On Mon, Aug 13, 2018 at 06:33:12PM +0200, petrcve...@gmail.com wrote: >> From: Petr Cvek >> >> When transferring a media sensor driver from the soc_camera I've found >> the controller module can get removed (which will cause a stack dump >> because the sensor driver depends on resources from the controller driver). > > There may be a kernel oops if a resource used by another driver goes away. > But the right fix isn't to prevent unloading that module. Instead, one way > to address the problem would be to have persistent clock objects that would > not be dependent on the driver that provides them. > Yeah it is a hack, but it allows me to rmmod the sensor driver. Otherwise it has a refcount "deadlock" as it holds the refcount on itself. The _only_ way to unload the driver is to reboot. ad clocks: Well the sensor driver and the controller are using v4l2_clk_* so I was poking around that. Anyway the independent clock object would be a problem, the clock control is inside the capture driver register set (divider, enable) (I think) and it gets gated out completely when the capture driver is unloaded. And when the clock is stopped the sensor isn't responding to i2c commands. The clock control is really dependent on the driver unless there is some callback to the sensor which interrupts it from non-responding i2c transfers. >> >> When I've tried to remove the driver module of the sensor it said the >> resource was busy (without a reference name) though is should be >> possible to remove the sensor driver because it is at the end of >> the dependency list and not to remove the controller driver. > > That might be one day possible but it is not today. > > You'll still need to acquire the sensor module as well as it registers a > media entity as well as a sub-device. > The ported sensor driver doesn't contain any media entity code. best regards, Petr
Re: [PATCH v1 4/5] [media] i2c: drop soc_camera code from ov9640 and switch to v4l2_async
Dne 10.8.2018 v 09:51 jacopo mondi napsal(a):
> Hi Petr,
>
> On Thu, Aug 09, 2018 at 03:39:48AM +0200, petrcve...@gmail.com wrote:
>> From: Petr Cvek
>>
>> This patch removes the dependency on an obsoleted soc_camera from ov9640
>> driver and changes the code to be a standalone v4l2 async subdevice.
>> It also adds GPIO allocations for power and reset signals (as they are not
>> handled by soc_camera now).
>>
>> The patch should make ov9640 again compatible with the pxa_camera driver.
>
> Are there board files using this driverin mainline ? (git grep says so)
> Care to port them to use the new driver if necessary? You can have a
> look at the SH4 Migo-R board, which recently underwent the same
> process (arch/sh/boards/mach-migor/setup.c)
>
Yes there are Magician and Palm Zire72 which are directly using ov9640
(and few others which are using pxa_camera with a different sensor). I'm
working on HTC magician (pxa_camera is not a soc_camera subdev anymore,
ov9640 still is).
> I also suggest to adjust the build system in a single patch with this
> changes, but that's not a big deal...
>
OK (at the end of the patchset I suppose?)
>
>> +ret = v4l2_clk_enable(priv->clk);
>
> Is this required by the pxa camera driver using v4l2_clk_ APIs?
> Otherwise you should use the clk API directly.
>
Yes the clock is registered by pxa camera with v4l2_clk_register(). I
will probably get to that in the future, but there is stuff (bugs, dead
code from soc_camera removal, ...) with more priority in the driver for now.
>> +mdelay(1);
>> +gpiod_set_value(priv->gpio_reset, 0);
>> +} else {
>> +gpiod_set_value(priv->gpio_reset, 1);
>> +mdelay(1);
>> +v4l2_clk_disable(priv->clk);
>> +mdelay(1);
>> +gpiod_set_value(priv->gpio_power, 0);
>> +}
>> +return ret;
>> }
>>
>> /* select nearest higher resolution for capture */
>> @@ -631,14 +648,10 @@ static const struct v4l2_subdev_core_ops
>> ov9640_core_ops = {
>> static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
>> struct v4l2_mbus_config *cfg)
>
> g_mbus/s_mbus are deprecated. Unless the pxa camera driver wants them
> all format negotiation should go through s_fmt/g_fmt pad operations
>
Yeah it does:
ret = sensor_call(pcdev, video, g_mbus_config, );
>> {
>> -struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> -
>> cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>> V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>> V4L2_MBUS_DATA_ACTIVE_HIGH;
>> cfg->type = V4L2_MBUS_PARALLEL;
>> -cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>>
>> return 0;
>> }
>> @@ -667,18 +680,27 @@ static int ov9640_probe(struct i2c_client *client,
>> const struct i2c_device_id *did)
>> {
>> struct ov9640_priv *priv;
>> -struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> int ret;
>>
>> -if (!ssdd) {
>> -dev_err(>dev, "Missing platform_data for driver\n");
>> -return -EINVAL;
>> -}
>> -
>> -priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
>> +priv = devm_kzalloc(>dev, sizeof(*priv),
>> +GFP_KERNEL);
>> if (!priv)
>> return -ENOMEM;
>>
>> +priv->gpio_power = devm_gpiod_get(>dev, "Camera power",
>> + GPIOD_OUT_LOW);
>> +if (IS_ERR_OR_NULL(priv->gpio_power)) {
>> +ret = PTR_ERR(priv->gpio_power);
>> +return ret;
>> +}
>> +
>> +priv->gpio_reset = devm_gpiod_get(>dev, "Camera reset",
>> + GPIOD_OUT_HIGH);
>> +if (IS_ERR_OR_NULL(priv->gpio_reset)) {
>> +ret = PTR_ERR(priv->gpio_reset);
>> +return ret;
>> +}
>> +
>> v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
>>
>> v4l2_ctrl_handler_init(>hdl, 2);
>> @@ -692,17 +714,25 @@ static int ov9640_probe(struct i2c_client *client,
>>
>> priv->clk = v4l2_clk_get(>dev, "mclk");
>> if (IS_ERR(priv->clk)) {
>> -ret = PTR_ERR(priv->clk);
>> +ret = -EPROBE_DEFER;
>
> Why are you forcing EPROBE_DEFER instead of returning the clk_get()
> return value?
>
That may be residue from testing, I will fix that.
Petr
Re: [RFC] [PATCH 0/4] [media] pxa_camera: Fixing bugs and missing colorformats
Dne 13.5.2017 v 08:40 Petr Cvek napsal(a): > The second problem seems to be a same problem. When playing/encoding the data > from the sensor (with or without previous fix) and calling (probably) > anything on v4l2 the drivers stops in a same way. I discovered it by trying > to use the CONFIG_VIDEO_ADV_DEBUG interface to realtime poking the sensor. > > You should be able to recreate that by starting the stream (mplayer or > ffmpeg) and run: > v4l2-ctl -n > or reading registers, running v4l2-compliance etc... > I get it now. The second problem is likely in pxac_fops_camera_release(). At the closing of the device the function calls sensor_call → s_power and sets the sensor to OFF regardless if somebody is using it somewhere else. There should be reference counter (as was in the original soc_camera_close() function). This bug makes impossible to debug pxa camera and its sensors. cheers, Petr
Re: [RFC] [PATCH 0/4] [media] pxa_camera: Fixing bugs and missing colorformats
Dne 2.5.2017 v 16:57 Robert Jarzmik napsal(a): > Petr Cvek <petr.c...@tul.cz> writes: > >> Dne 1.5.2017 v 06:20 Petr Cvek napsal(a): >>> This patchset is just a grouping of a few bugfixes I've found during >>> the ov9640 sensor support re-adding. >> >> P.S. I've manually calculated every format variant for the image size >> calculation functions, but still these functions are not too robust (for >> every >> hypothetical bps/packing/layout combination). For example: >> >> MEDIA_BUS_FMT_Y8_1X8 >> .name = "Grey", >> .bits_per_sample= 8, >> .packing= PXA_MBUS_PACKING_NONE, >> .order = PXA_MBUS_ORDER_LE, >> .layout = PXA_MBUS_LAYOUT_PACKED, >> >> seems to me as a little bit misleading. The better solution would be to have >> something like bytes_per_line and image_size coefficients. Is my idea worth >> a try? >> >> Anyway the .order field seems to be unused (it is a pxa_camera defined >> structure). I'm for removing it (I can create a patch and test it on the >> real hardware). Unless there are plans for it. >> >> The pxa_camera_get_formats() could be probably simplified even up to the >> point >> of a removal of the soc_camera_format_xlate structure. If no one works on it >> (in >> like 2 months) I can try to simplify it. > > Let's see what new ideas can provide, new blood etc ... > OK a problem, probably some kind of race condition. I was trying to add RGB888 mode (and make patchset v2) and wanted to debug it on the runtime (OT: sensor probably does not support it) but I've found probably some race condition :-/. It seems that buffer DMA chaining works only until all buffers are filled for the first time (ffmpeg allocates 32 buffers BTW, for Magician it is more than RAM size, I've forced it to 4 so the problem is more often). pxa27x-camera pxa27x-camera.0: (omitted from log) pxac_vb2_queue_setup(vq=c39472d0 nbufs=2 num_planes=0 size=153600) pxac_vb2_init(nb_channels=1) pxac_vb2_init(nb_channels=1) pxac_vb2_prepare (vb=c1a4dbe0) nb_channels=1 size=153600 pxac_vb2_prepare (vb=c0f33ce0) nb_channels=1 size=153600 pxac_vb2_queue(vb=c1a4dbe0) nb_channels=1 size=153600 active= (null) pxa_dma_add_tail_buf (channel=0) : submit vb=c1a4dbe0 cookie=18 pxac_vb2_queue(vb=c0f33ce0) nb_channels=1 size=153600 active= (null) pxa_dma_add_tail_buf (channel=0) : submit vb=c0f33ce0 cookie=19 pxac_vb2_start_streaming(count=2) active= (null) pxa_camera_start_capture S:8358 0:83ff Camera interrupt status 0x9119 Camera interrupt status 0x0 pxa_dma_start_channels (channel=0) camera dma irq, cisr=0x8318 dma=1 pxa_camera_wakeup dequeued buffer (buf=0xc1a4dbe0) pxa_camera_check_link_miss : top queued buffer=c0f33ce0, is_dma_stopped=0 19 19 //I added two parameters: "19" "19" are last_submitted and last_issued (DMA cookies) pxac_vb2_prepare (vb=c1a4dbe0) nb_channels=1 size=153600 pxac_vb2_queue(vb=c1a4dbe0) nb_channels=1 size=153600 active=c0f33ce0 pxa_dma_add_tail_buf (channel=0) : submit vb=c1a4dbe0 cookie=20 camera dma irq, cisr=0x8318 dma=1 pxa_camera_wakeup dequeued buffer (buf=0xc0f33ce0) pxa_camera_check_link_miss : top queued buffer=c1a4dbe0, is_dma_stopped=0 20 20 pxac_vb2_prepare (vb=c0f33ce0) nb_channels=1 size=153600 pxac_vb2_queue(vb=c0f33ce0) nb_channels=1 size=153600 active=c1a4dbe0 pxa_dma_add_tail_buf (channel=0) : submit vb=c0f33ce0 cookie=21 camera dma irq, cisr=0x8318 dma=1 pxa_camera_wakeup dequeued buffer (buf=0xc1a4dbe0) pxa_camera_check_link_miss : top queued buffer=c0f33ce0, is_dma_stopped=0 21 21 pxac_vb2_prepare (vb=c1a4dbe0) nb_channels=1 size=153600 pxac_vb2_queue(vb=c1a4dbe0) nb_channels=1 size=153600 active=c0f33ce0 pxa_dma_add_tail_buf (channel=0) : submit vb=c1a4dbe0 cookie=22 camera dma irq, cisr=0x8318 dma=1 pxa_camera_wakeup dequeued buffer (buf=0xc0f33ce0) pxa_camera_check_link_miss : top queued buffer=c1a4dbe0, is_dma_stopped=0 22 22 pxac_vb2_prepare (vb=c0f33ce0) nb_channels=1 size=153600 pxac_vb2_queue(vb=c0f33ce0) nb_channels=1 size=153600 active=c1a4dbe0 pxa_dma_add_tail_buf (channel=0) : submit vb=c0f33ce0 cookie=23 camera dma irq, cisr=0x8318 dma=1 pxa_camera_wakeup dequeued buffer (buf=0xc1a4dbe0) pxa_camera_check_link_miss : top queued buffer=c0f33ce0, is_dma_stopped=0 23 23 camera dma irq, cisr=0x8318 dma=1 pxa_camera_wakeup dequeued buffer (buf=0xc0f33ce0) pxa_dma_st
Re: [PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation
Dne 2.5.2017 v 16:43 Robert Jarzmik napsal(a): > Petr Cvek <petr.c...@tul.cz> writes: > >> During the transfer from the soc_camera a test in pxa_mbus_image_size() >> got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either >> the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles >> the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and >> PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg >> (for some of the YUYV subvariants). >> >> This patch re-adds the same test as in soc_camera version. >> >> Signed-off-by: Petr Cvek <petr.c...@tul.cz> > Did you test that with YUV422P format ? > If yes, then you can have my ack. > I was trying to add RGB888 and then I've noticed that "YVYU 16bit" formats (and permutation) and "Bayer 12" formats have incorrect values in the bits_per_sample field. I didn't notice it in the patch creation, because it doesn't affect the computations of the image size. And later in the code it just defaults to 8. A comment for a switch in the pxa_camera_setup_cicr() function: "Datawidth is now guaranteed to be equal to one of the three values..." Values are 10, 9 and 8 and they describe a bit-vector length of the interface for a sensor. So I will include a fix for the patchset v2 for this. I will test the YUYV formats, but my sensor does not support the Bayer 12 formats. In the addition I propose a patch for an enum type (or define) of the interface configuration field. Something like: #define PXA_MBUS_BUSWIDTH_8 8 #define PXA_MBUS_BUSWIDTH_9 9 #define PXA_MBUS_BUSWIDTH_1010 and a patch for something like: s/bits_per_sample/buswidth/g and a formatting patch for excessive tabulators in the mbus_fmt structure initialization ;-). Petr
Re: [PATCH 4/4] [media] pxa_camera: Fix a call with an uninitialized device pointer
Dne 2.5.2017 v 16:53 Robert Jarzmik napsal(a):
> Petr Cvek <petr.c...@tul.cz> writes:
>
>> In 'commit 295ab497d6357 ("[media] media: platform: pxa_camera: make
>> printk consistent")' a pointer to the device structure in
>> mclk_get_divisor() was changed to pcdev_to_dev(pcdev). The pointer used
>> by pcdev_to_dev() is still uninitialized during the call to
>> mclk_get_divisor() as it happens in v4l2_device_register() at the end
>> of the probe. The dev_warn and dev_dbg caused a line in the log:
>>
>> (NULL device *): Limiting master clock to 2600
>>
>> Fix this by using an initialized pointer from the platform_device
>> (as before the old patch).
>>
>> Signed-off-by: Petr Cvek <petr.c...@tul.cz>
> Right, would be good to add to the commit message :
> Fixes: 295ab497d635 ("[media] media: platform: pxa_camera: make printk
> consistent")
>
OK I will add it in v2.
Re: [PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation
Dne 2.5.2017 v 16:43 Robert Jarzmik napsal(a): > Petr Cvek <petr.c...@tul.cz> writes: > >> During the transfer from the soc_camera a test in pxa_mbus_image_size() >> got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either >> the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles >> the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and >> PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg >> (for some of the YUYV subvariants). >> >> This patch re-adds the same test as in soc_camera version. >> >> Signed-off-by: Petr Cvek <petr.c...@tul.cz> > Did you test that with YUV422P format ? > If yes, then you can have my ack. pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=320x240:50323234) And mplayer to framebuffer "somewhat" works (it timeouts after some time but it does regardless on format, ffmpeg is fine). Anyway the patch does not affect V4L2_PIX_FMT_YUV422P in any way as the .layout field is PXA_MBUS_LAYOUT_PLANAR_2Y_U_V and test is only for "== PXA_MBUS_LAYOUT_PACKED" > > And you should add Hans to the reviewers list, it's his call ultimately, and > his > tree which should carry it on. > > Cheers. > > -- > Robert > Petr
Re: [PATCH] [media] pxa_camera: fix module remove codepath for v4l2 clock
Dne 28.4.2017 v 08:31 Robert Jarzmik napsal(a): > Petr Cvek <petr.c...@tul.cz> writes: > >> I will post some other bugfixes (and feature adding) for pxa_camera soon. Do >> you wish to be CC'd? >> >> P.S. Who is the the maintainer of pxa_camera BTW? Still Guennadi >> Liakhovetski? > Euh no, that's me. OK ... so when I remove the ov9640 driver from soc_camera (palmz72 and magician used it with soc_camera+pxa_camera) does that mean I will be its maintainer? Petr
Re: [RFC] [PATCH 0/4] [media] pxa_camera: Fixing bugs and missing colorformats
Dne 1.5.2017 v 06:20 Petr Cvek napsal(a): > This patchset is just a grouping of a few bugfixes I've found during > the ov9640 sensor support re-adding. P.S. I've manually calculated every format variant for the image size calculation functions, but still these functions are not too robust (for every hypothetical bps/packing/layout combination). For example: MEDIA_BUS_FMT_Y8_1X8 .name = "Grey", .bits_per_sample= 8, .packing= PXA_MBUS_PACKING_NONE, .order = PXA_MBUS_ORDER_LE, .layout = PXA_MBUS_LAYOUT_PACKED, seems to me as a little bit misleading. The better solution would be to have something like bytes_per_line and image_size coefficients. Is my idea worth a try? Anyway the .order field seems to be unused (it is a pxa_camera defined structure). I'm for removing it (I can create a patch and test it on the real hardware). Unless there are plans for it. The pxa_camera_get_formats() could be probably simplified even up to the point of a removal of the soc_camera_format_xlate structure. If no one works on it (in like 2 months) I can try to simplify it. best regards, Petr Cvek
[PATCH 4/4] [media] pxa_camera: Fix a call with an uninitialized device pointer
In 'commit 295ab497d6357 ("[media] media: platform: pxa_camera: make
printk consistent")' a pointer to the device structure in
mclk_get_divisor() was changed to pcdev_to_dev(pcdev). The pointer used
by pcdev_to_dev() is still uninitialized during the call to
mclk_get_divisor() as it happens in v4l2_device_register() at the end
of the probe. The dev_warn and dev_dbg caused a line in the log:
(NULL device *): Limiting master clock to 2600
Fix this by using an initialized pointer from the platform_device
(as before the old patch).
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
drivers/media/platform/pxa_camera.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/pxa_camera.c
b/drivers/media/platform/pxa_camera.c
index 79fd7269d1e6..c8466c63be22 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -1124,7 +1124,7 @@ static u32 mclk_get_divisor(struct platform_device *pdev,
/* mclk <= ciclk / 4 (27.4.2) */
if (mclk > lcdclk / 4) {
mclk = lcdclk / 4;
- dev_warn(pcdev_to_dev(pcdev),
+ dev_warn(>dev,
"Limiting master clock to %lu\n", mclk);
}
@@ -1135,7 +1135,7 @@ static u32 mclk_get_divisor(struct platform_device *pdev,
if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
pcdev->mclk = lcdclk / (2 * (div + 1));
- dev_dbg(pcdev_to_dev(pcdev), "LCD clock %luHz, target freq %luHz,
divisor %u\n",
+ dev_dbg(>dev, "LCD clock %luHz, target freq %luHz, divisor %u\n",
lcdclk, mclk, div);
return div;
--
2.11.0
[PATCH 3/4] [media] pxa_camera: Add (un)subscribe_event ioctl
The v4l2-compliance complains about nonexistent vidioc_subscribe_event
and vidioc_unsubscribe_event calls. Add them to fix the complaints.
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
drivers/media/platform/pxa_camera.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/media/platform/pxa_camera.c
b/drivers/media/platform/pxa_camera.c
index f71e7e0a652b..79fd7269d1e6 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -37,7 +37,9 @@
#include
#include
#include
+#include
#include
+#include
#include
#include
@@ -2089,6 +2091,8 @@ static const struct v4l2_ioctl_ops pxa_camera_ioctl_ops =
{
.vidioc_g_register = pxac_vidioc_g_register,
.vidioc_s_register = pxac_vidioc_s_register,
#endif
+ .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
+ .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
};
static struct v4l2_clk_ops pxa_camera_mclk_ops = {
--
2.11.0
[PATCH 2/4] [media] pxa_camera: Fix incorrect test in the image size generation
During the transfer from the soc_camera a test in pxa_mbus_image_size()
got removed. Without it any PXA_MBUS_LAYOUT_PACKED format causes either
the return of a wrong value (PXA_MBUS_PACKING_2X8_PADHI doubles
the correct value) or EINVAL (PXA_MBUS_PACKING_NONE and
PXA_MBUS_PACKING_EXTEND16). This was observed in an error from the ffmpeg
(for some of the YUYV subvariants).
This patch re-adds the same test as in soc_camera version.
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
drivers/media/platform/pxa_camera.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/pxa_camera.c
b/drivers/media/platform/pxa_camera.c
index 5f007713417e..f71e7e0a652b 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -575,6 +575,9 @@ static s32 pxa_mbus_bytes_per_line(u32 width, const struct
pxa_mbus_pixelfmt *mf
static s32 pxa_mbus_image_size(const struct pxa_mbus_pixelfmt *mf,
u32 bytes_per_line, u32 height)
{
+ if (mf->layout == PXA_MBUS_LAYOUT_PACKED)
+ return bytes_per_line * height;
+
switch (mf->packing) {
case PXA_MBUS_PACKING_2X8_PADHI:
return bytes_per_line * height * 2;
--
2.11.0
[PATCH 1/4] [media] pxa_camera: Add remaining Bayer 8 formats
This patch adds Bayer 8 GBRG and RGGB support and move GRBG definition
close to BGGR (so all Bayer 8 variants are together). No other changes are
needed as the driver handles them as RAW data stream.
The RGGB variant was tested in a modified OV9640 driver.
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
drivers/media/platform/pxa_camera.c | 40 +++--
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/media/platform/pxa_camera.c
b/drivers/media/platform/pxa_camera.c
index 6615f80fe059..5f007713417e 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -345,6 +345,36 @@ static const struct pxa_mbus_lookup mbus_fmt[] = {
.layout = PXA_MBUS_LAYOUT_PACKED,
},
}, {
+ .code = MEDIA_BUS_FMT_SGBRG8_1X8,
+ .fmt = {
+ .fourcc = V4L2_PIX_FMT_SGBRG8,
+ .name = "Bayer 8 GBRG",
+ .bits_per_sample= 8,
+ .packing= PXA_MBUS_PACKING_NONE,
+ .order = PXA_MBUS_ORDER_LE,
+ .layout = PXA_MBUS_LAYOUT_PACKED,
+ },
+}, {
+ .code = MEDIA_BUS_FMT_SGRBG8_1X8,
+ .fmt = {
+ .fourcc = V4L2_PIX_FMT_SGRBG8,
+ .name = "Bayer 8 GRBG",
+ .bits_per_sample= 8,
+ .packing= PXA_MBUS_PACKING_NONE,
+ .order = PXA_MBUS_ORDER_LE,
+ .layout = PXA_MBUS_LAYOUT_PACKED,
+ },
+}, {
+ .code = MEDIA_BUS_FMT_SRGGB8_1X8,
+ .fmt = {
+ .fourcc = V4L2_PIX_FMT_SRGGB8,
+ .name = "Bayer 8 RGGB",
+ .bits_per_sample= 8,
+ .packing= PXA_MBUS_PACKING_NONE,
+ .order = PXA_MBUS_ORDER_LE,
+ .layout = PXA_MBUS_LAYOUT_PACKED,
+ },
+}, {
.code = MEDIA_BUS_FMT_SBGGR10_1X10,
.fmt = {
.fourcc = V4L2_PIX_FMT_SBGGR10,
@@ -445,16 +475,6 @@ static const struct pxa_mbus_lookup mbus_fmt[] = {
.layout = PXA_MBUS_LAYOUT_PACKED,
},
}, {
- .code = MEDIA_BUS_FMT_SGRBG8_1X8,
- .fmt = {
- .fourcc = V4L2_PIX_FMT_SGRBG8,
- .name = "Bayer 8 GRBG",
- .bits_per_sample= 8,
- .packing= PXA_MBUS_PACKING_NONE,
- .order = PXA_MBUS_ORDER_LE,
- .layout = PXA_MBUS_LAYOUT_PACKED,
- },
-}, {
.code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8,
.fmt = {
.fourcc = V4L2_PIX_FMT_SGRBG10DPCM8,
--
2.11.0
[PATCH 0/4] [media] pxa_camera: Fixing bugs and missing colorformats
This patchset is just a grouping of a few bugfixes I've found during the ov9640 sensor support re-adding. The remaining Bayer 8 formats are just a permutation of the channels (pxa_camera treats them as raw data). The missing/incorrect test in the image size generation was just re-added from the soc_camera. The (un)subscribe_event ioctl calls were added in a same way as other media drivers. The call with an uninitialized pointer is a revert of a part of the patch, which broke it. The series are based on the kernel commit after [media] pxa_camera: fix module remove codepath for v4l2 clock Petr Cvek (4): [media] pxa_camera: Add remaining Bayer 8 formats [media] pxa_camera: Fix incorrect test in the image size generation [media] pxa_camera: Add (un)subscribe_event ioctl [media] pxa_camera: Fix a call with an uninitialized device pointer drivers/media/platform/pxa_camera.c | 51 - 1 file changed, 39 insertions(+), 12 deletions(-) -- 2.11.0
Re: [PATCH] [media] pxa_camera: fix module remove codepath for v4l2 clock
Dne 27.4.2017 v 21:20 Robert Jarzmik napsal(a): > Petr Cvek <petr.c...@tul.cz> writes: > >> The conversion from soc_camera omitted a correct handling of the clock >> gating for a sensor. When the pxa_camera driver module was removed it >> tried to unregister clk, but this caused a similar warning: >> >> WARNING: CPU: 0 PID: 6740 at drivers/media/v4l2-core/v4l2-clk.c:278 >> v4l2_clk_unregister(): Refusing to unregister ref-counted 0-0030 clock! >> >> The clock was at time still refcounted by the sensor driver. Before >> the removing of the pxa_camera the clock must be dropped by the sensor >> driver. This should be triggered by v4l2_async_notifier_unregister() call >> which removes sensor driver module too, calls unbind() function and then >> tries to probe sensor driver again. Inside unbind() we can safely >> unregister the v4l2 clock as the sensor driver got removed. The original >> v4l2_clk_unregister() should be put inside test as the clock can be >> already unregistered from unbind(). If there was not any bound sensor >> the clock is still present. >> >> The codepath is practically a copy from the old soc_camera. The bug was >> tested with a pxa_camera+ov9640 combination during the conversion >> of the ov9640 from the soc_camera. >> >> Signed-off-by: Petr Cvek <petr.c...@tul.cz> > > Yeah, it's way better with this patch, especially the > insmod/rmmod/insmod/rmmod > test. I will post some other bugfixes (and feature adding) for pxa_camera soon. Do you wish to be CC'd? P.S. Who is the the maintainer of pxa_camera BTW? Still Guennadi Liakhovetski? Basically pxa_camera is no longer part of the soc_camera and MAINTAINERS file does not exactly specify pxa_camera.c (I'm asking because I will send the patch for ov9640 soc_camera removal too :-D). Best regards, Petr
[PATCH] [media] pxa_camera: fix module remove codepath for v4l2 clock
The conversion from soc_camera omitted a correct handling of the clock
gating for a sensor. When the pxa_camera driver module was removed it
tried to unregister clk, but this caused a similar warning:
WARNING: CPU: 0 PID: 6740 at drivers/media/v4l2-core/v4l2-clk.c:278
v4l2_clk_unregister(): Refusing to unregister ref-counted 0-0030 clock!
The clock was at time still refcounted by the sensor driver. Before
the removing of the pxa_camera the clock must be dropped by the sensor
driver. This should be triggered by v4l2_async_notifier_unregister() call
which removes sensor driver module too, calls unbind() function and then
tries to probe sensor driver again. Inside unbind() we can safely
unregister the v4l2 clock as the sensor driver got removed. The original
v4l2_clk_unregister() should be put inside test as the clock can be
already unregistered from unbind(). If there was not any bound sensor
the clock is still present.
The codepath is practically a copy from the old soc_camera. The bug was
tested with a pxa_camera+ov9640 combination during the conversion
of the ov9640 from the soc_camera.
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
drivers/media/platform/pxa_camera.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/pxa_camera.c
b/drivers/media/platform/pxa_camera.c
index 929006f65cc7..6615f80fe059 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2177,6 +2177,12 @@ static void pxa_camera_sensor_unbind(struct
v4l2_async_notifier *notifier,
pxa_dma_stop_channels(pcdev);
pxa_camera_destroy_formats(pcdev);
+
+ if (pcdev->mclk_clk) {
+ v4l2_clk_unregister(pcdev->mclk_clk);
+ pcdev->mclk_clk = NULL;
+ }
+
video_unregister_device(>vdev);
pcdev->sensor = NULL;
@@ -2501,7 +2507,13 @@ static int pxa_camera_remove(struct platform_device
*pdev)
dma_release_channel(pcdev->dma_chans[1]);
dma_release_channel(pcdev->dma_chans[2]);
- v4l2_clk_unregister(pcdev->mclk_clk);
+ v4l2_async_notifier_unregister(>notifier);
+
+ if (pcdev->mclk_clk) {
+ v4l2_clk_unregister(pcdev->mclk_clk);
+ pcdev->mclk_clk = NULL;
+ }
+
v4l2_device_unregister(>v4l2_dev);
dev_info(>dev, "PXA Camera driver unloaded\n");
--
2.11.0
Re: [PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support
Dne 21.8.2015 v 19:36 Robert Jarzmik napsal(a): I shrunk a bit the mailing list. Arnd Bergmann a...@arndb.de writes: On Friday 21 August 2015 00:39:30 Petr Cvek wrote: Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: Petr Cvek petr.c...@tul.cz writes: Datasheet says: tS:RESETSetting time after software/hardware reset 1 ms So at least one ~1 ms should be left there. Are msleep less than 20ms valid? (checkpatch: msleep 20ms can sleep for up to 20ms) On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds (but could be much longer), while an mdelay(1) tries to sleep around one millisecond, more or less. I have rethought of that a bit more. I'm convinced a delay of at least 1ms is necessary according to the specification, it can also be more. Moreover, I came to the conclusion this reset sequence is not something that is magician specific (see palmz72_camera_reset() in .../mach-pxa/palmz72.c). Actually it's not even mach-pxa specific, it's ov9640 specific. Now add this to the fact that it would be good to have a solution working for devicetree as well, and on any board, and the conclusion I came to was that this handling deserves to be in ov9640 driver (please correct me if I'm wrong). The idea behind it is have the reset handled in ov9640, and the gpio provided by platform data or devicetree. So Guennadi, is it possible to add a gpio through platform data to ov9640 driver, does it make sense, and would you accept to have the reset handled there ? And if yes, would you, Petr, accept to revamp your patch to have the reset and power handled in ov9640 ? OK, why not, so power and reset gpio with polarity settings? Please note that it is a proposal, I'm not forcing anybody, I'd like to choose a path that agrees with the future push to remove as many machine files as possible. Anyway I'm planning to send patch for OV9640 in future too (color correction matrix is not complete and some registers too). Cheers. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
