Hi Marek,
Thanks for explanation, I understand now.
Please note that dcmi is not atmel-isi.c has same code structure, hence
same problem:
static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
struct v4l2_subdev_state pad_state = {
.pads = &pad_cfg
};
[...]
ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
drivers/media/platform/renesas/vsp1/vsp1_entity.c
/*
* FIXME: Drop this call, drivers are not supposed to use
* __v4l2_subdev_state_alloc().
*/
entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
"vsp1:config->lock", &key);
drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
/*
* FIXME: Drop this call, drivers are not supposed to use
* __v4l2_subdev_state_alloc().
*/
sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
So I wonder about introducing this new change in dcmi while it is marked
as "FIXME" in other camera interface drivers ?
Best regards,
Hugues.
On 6/27/22 13:30, Marek Vasut wrote:
On 6/27/22 11:14, Hugues FRUCHET wrote:
Hi Marek,
Hi,
On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
Any local subdev state should be allocated and free'd using
__v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
takes care of calling .init_cfg() subdev op. Without this,
subdev internal state might be uninitialized by the time
any other subdev op is called.
Does this fix a bug you have?
Yes
Which bug did you encounter exactly ?
The DCMI driver does set_fmt subdev call on the sensor driver instance.
The mt9p031 sensor driver set_fmt depends on crop rectangle to be
initialized _before_ set_fmt subdev call is made. Currently this
initialization is done in open callback, which is too late, so when the
DCMI does set_fmt subdev call, it operates on uninitialized private data.
There is patch to mt9p031 driver which move the initialization to the
right place in .init_cfg:
[PATCH v2] media: mt9p031: Move open subdev op init code into init_cfg
However, the .init_cfg is not called by DCMI right now. For that to be
called in the right place, __v4l2_subdev_state_alloc() must be added,
hence this patch.
You won't trigger the problem on OV5640 because that one driver does not
implement .init_cfg v4l2_subdev_ops .
This is strange that we have not yet encounter any problems around
that through our tests campaigns... or we have to enforce with a new
test, so better to know what your problem was exactly.
You need a sensor driver which implements struct v4l2_subdev_ops
.init_cfg and then have something in set_fmt depend on the
initialization done in the .init_cfg callback . Then you would see the
problem.
Wasn't this broken even before the active state, as init_cfg was not
called?
Yes, this was always broken. I suspect nobody tested this mode of
operation before. In my case, I have this DCMI driver connected
directly to MT9P006 sensor.
As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
I don't see the difference with our OV5640 used in parallel mode which
is a mainline config on our side, so one more time I wonder what the
problem was.
See above.
