Hi Hans,
On 2017-11-06 08:19 AM, Hans Verkuil wrote:
> Hi Helen,
>
> On 09/27/2017 08:30 PM, Helen Koike wrote:
>> Hi Hans,
>>
>> Thanks for your patch and sorry for my late reply.
>
> Sorry for my late reply to your reply :-)
>
>> Please see my comments and questions below
>>
>> On 2017-07-28 07:23 AM, Hans Verkuil wrote:
>>> Add support for the test_pattern control and the h/vflip controls.
>>>
>>> This makes it possible to switch to more interesting test patterns and to
>>> test control handling in v4l-subdevs.
>>>
>>> There are more tpg-related controls that can be added, but this is a good
>>> start.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>> ---
>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>>> index dca528a316e7..2e9981b18166 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.h
>>> +++ b/drivers/media/platform/vimc/vimc-common.h
>>> @@ -22,6 +22,11 @@
>>> #include <media/media-device.h>
>>> #include <media/v4l2-device.h>
>>>
>>> +/* VIMC-specific controls */
>>> +#define VIMC_CID_VIMC_BASE (0x00f00000 | 0xf000)
>>> +#define VIMC_CID_VIMC_CLASS (0x00f00000 | 1)
>>
>> Why this values, shouldn't we use a derivative from
>> V4L2_CID_PRIVATE_BASE for custom controls? Or can we use random values?
>
> The values are taken from vivid which uses the same scheme. These controls
> deal with how the virtual driver emulates things, and I prefer not to make
> these control IDs part of the public API so we can be a bit more flexible in
> the future. It's a design choice which worked well for vivid.
>
>>
>>> +#define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0)
>>> +
>>> #define VIMC_FRAME_MAX_WIDTH 4096
>>> #define VIMC_FRAME_MAX_HEIGHT 2160
>>> #define VIMC_FRAME_MIN_WIDTH 16
>>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
>>> index 615c2b18dcfc..532097566b27 100644
>>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>>> @@ -22,6 +22,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/v4l2-mediabus.h>
>>> #include <linux/vmalloc.h>
>>> +#include <media/v4l2-ctrls.h>
>>> #include <media/v4l2-subdev.h>
>>> #include <media/v4l2-tpg.h>
>>>
>>> @@ -38,6 +39,7 @@ struct vimc_sen_device {
>>> u8 *frame;
>>> /* The active format */
>>> struct v4l2_mbus_framefmt mbus_format;
>>> + struct v4l2_ctrl_handler hdl;
>>> };
>>>
>>> static const struct v4l2_mbus_framefmt fmt_default = {
>>> @@ -291,6 +293,31 @@ static const struct v4l2_subdev_ops vimc_sen_ops = {
>>> .video = &vimc_sen_video_ops,
>>> };
>>>
>>> +static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl)
>>> +{
>>> + struct vimc_sen_device *vsen =
>>> + container_of(ctrl->handler, struct vimc_sen_device, hdl);
>>> +
>>> + switch (ctrl->id) {
>>> + case VIMC_CID_TEST_PATTERN:
>>> + tpg_s_pattern(&vsen->tpg, ctrl->val);
>>> + break;
>>> + case V4L2_CID_HFLIP:
>>> + tpg_s_hflip(&vsen->tpg, ctrl->val);
>>> + break;
>>> + case V4L2_CID_VFLIP:
>>> + tpg_s_vflip(&vsen->tpg, ctrl->val);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
>>> + .s_ctrl = vimc_sen_s_ctrl,
>>> +};
>>> +
>>> static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
>>> void *master_data)
>>> {
>>> @@ -299,10 +326,29 @@ static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
>>> container_of(ved, struct vimc_sen_device, ved);
>>>
>>> vimc_ent_sd_unregister(ved, &vsen->sd);
>>> + v4l2_ctrl_handler_free(&vsen->hdl);
>>> tpg_free(&vsen->tpg);
>>> kfree(vsen);
>>> }
>>>
>>> +/* Image Processing Controls */
>>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_class = {
>>> + .ops = &vimc_sen_ctrl_ops,
>>> + .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY,
>>
>> I was wondering if it is really necessary to specify the ops and flags
>> in the class, as this is seems to me a meta control for the other
>> controls to be grouped in a class.
>
> ops can be dropped, but flags needs to be there.
>
>>
>>> + .id = VIMC_CID_VIMC_CLASS,
>>> + .name = "VIMC Controls",
>>> + .type = V4L2_CTRL_TYPE_CTRL_CLASS,
>>> +};
>>> +
>>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
>>> + .ops = &vimc_sen_ctrl_ops,
>>> + .id = VIMC_CID_TEST_PATTERN,
>>> + .name = "Test Pattern",
>>> + .type = V4L2_CTRL_TYPE_MENU,
>>> + .max = TPG_PAT_NOISE,
>>> + .qmenu = tpg_pattern_strings,
>>> +};
>>> +
>>> static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>> void *master_data)
>>> {
>>> @@ -316,6 +362,20 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>> if (!vsen)
>>> return -ENOMEM;
>>>
>>> + v4l2_ctrl_handler_init(&vsen->hdl, 4);
>>> +
>>> + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_class, NULL);
>>> + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_test_pattern, NULL);
>>> + v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops,
>>> + V4L2_CID_VFLIP, 0, 1, 1, 0);
>>> + v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops,
>>> + V4L2_CID_HFLIP, 0, 1, 1, 0);
>>
>> Shouldn't we test the return values of the above functions? Or maybe not
>> because we should know what we are doing and this doesn't depend on the
>> user space.
>>
>>> + vsen->sd.ctrl_handler = &vsen->hdl;
>>> + if (vsen->hdl.error) {
>
> The error check happens here. These control functions do nothing if vsen->hdl.error
> is non-0, and set it if an error occurs.
>
> This simplifies the code since you have to check for an error only once at the end.
>
>>> + ret = vsen->hdl.error;
>>> + goto err_free_vsen;
>>> + }
>>> +
>>> /* Initialize ved and sd */
>>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
>>> pdata->entity_name,
>>> @@ -323,7 +383,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>> (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
>>> &vimc_sen_ops);
>>> if (ret)
>>> - goto err_free_vsen;
>>> + goto err_free_hdl;
>>>
>>> dev_set_drvdata(comp, &vsen->ved);
>>> vsen->dev = comp;
>>> @@ -342,6 +402,8 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>>
>>> err_unregister_ent_sd:
>>> vimc_ent_sd_unregister(&vsen->ved, &vsen->sd);
>>> +err_free_hdl:
>>> + v4l2_ctrl_handler_free(&vsen->hdl);
>>> err_free_vsen:
>>> kfree(vsen);
>>>
>>
>>
>> This conflicts a bit in the way I was preparing the optimization to
>> generate the pattern directly from the capture device as it will need to
>> propagate the changes from the controls in the sensor as well, but it
>> shouldn't be a problem to let the sensor to configure the tpg used in
>> the capture, I'll re-work my patch to include this.
>
> OK. I'll post a v2 dropping the ops field for the 'control class' control.
with this change,
Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
>
> Regards,
>
> Hans
>
