[PATCH v2 0/2] adv7604: .get_selection support

[PATCH v2 0/2] adv7604: .get_selection support

[Ulrich Hecht]

Hi!

The rcar_vin driver relies on this method.  The second patch makes sure that
they return up-to-date data if the input signal has changed since
initialization.

This revision implements .get_selection instead of .g_crop/.cropcap and
includes the suggested style changes.

It has been tested with the rcar_vin driver together with Hans Verkuil's
"v4l2: remove g/s_crop and cropcap from video ops" patch:

https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=rmcrop&idŸf32166c29d1323db090d638da27ea652d1d4d8

CU
Uli


Ulrich Hecht (2):
  media: adv7604: implement get_selection
  media: adv7604: update timings on change of input signal

 drivers/media/i2c/adv7604.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

-- 
2.6.3

[PATCH v2 1/2] media: adv7604: implement get_selection

[Ulrich Hecht]

The rcar_vin driver relies on this.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/media/i2c/adv7604.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index be5980c..8ad5c28 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1885,6 +1885,26 @@ static int adv76xx_get_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int adv76xx_get_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_selection *sel)
+{
+	struct adv76xx_state *state = to_state(sd);
+
+	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+		return -EINVAL;
+	/* Only CROP, CROP_DEFAULT and CROP_BOUNDS are supported */
+	if (sel->target > V4L2_SEL_TGT_CROP_BOUNDS)
+		return -EINVAL;
+
+	sel->r.left	= 0;
+	sel->r.top	= 0;
+	sel->r.width	= state->timings.bt.width;
+	sel->r.height	= state->timings.bt.height;
+
+	return 0;
+}
+
 static int adv76xx_set_format(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_pad_config *cfg,
 			      struct v4l2_subdev_format *format)
@@ -2415,6 +2435,7 @@ static const struct v4l2_subdev_video_ops adv76xx_video_ops = {
 
 static const struct v4l2_subdev_pad_ops adv76xx_pad_ops = {
 	.enum_mbus_code = adv76xx_enum_mbus_code,
+	.get_selection = adv76xx_get_selection,
 	.get_fmt = adv76xx_get_format,
 	.set_fmt = adv76xx_set_format,
 	.get_edid = adv76xx_get_edid,
-- 
2.6.3

Re: [PATCH v2 1/2] media: adv7604: implement get_selection

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Tuesday 22 December 2015 15:22:01 Ulrich Hecht wrote:
> The rcar_vin driver relies on this.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/i2c/adv7604.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index be5980c..8ad5c28 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1885,6 +1885,26 @@ static int adv76xx_get_format(struct v4l2_subdev *sd,
> return 0;
>  }
> 
> +static int adv76xx_get_selection(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_selection *sel)
> +{
> +	struct adv76xx_state *state = to_state(sd);
> +
> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +		return -EINVAL;
> +	/* Only CROP, CROP_DEFAULT and CROP_BOUNDS are supported */
> +	if (sel->target > V4L2_SEL_TGT_CROP_BOUNDS)
> +		return -EINVAL;
> +
> +	sel->r.left	= 0;
> +	sel->r.top	= 0;
> +	sel->r.width	= state->timings.bt.width;
> +	sel->r.height	= state->timings.bt.height;

This probably requires locking, but as the driver lacks locking support 
completely I can't blame you.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	return 0;
> +}
> +
>  static int adv76xx_set_format(struct v4l2_subdev *sd,
>  			      struct v4l2_subdev_pad_config *cfg,
>  			      struct v4l2_subdev_format *format)
> @@ -2415,6 +2435,7 @@ static const struct v4l2_subdev_video_ops
> adv76xx_video_ops = {
> 
>  static const struct v4l2_subdev_pad_ops adv76xx_pad_ops = {
>  	.enum_mbus_code = adv76xx_enum_mbus_code,
> +	.get_selection = adv76xx_get_selection,
>  	.get_fmt = adv76xx_get_format,
>  	.set_fmt = adv76xx_set_format,
>  	.get_edid = adv76xx_get_edid,

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/2] media: adv7604: update timings on change of input signal

[Ulrich Hecht]

Without this, .get_selection will always return the boot-time state.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/media/i2c/adv7604.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 8ad5c28..dcd659b 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1945,6 +1945,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 	u8 fmt_change_digital;
 	u8 fmt_change;
 	u8 tx_5v;
+	int ret;
 
 	if (irq_reg_0x43)
 		io_write(sd, 0x44, irq_reg_0x43);
@@ -1968,6 +1969,14 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 
 		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
 
+		/* update timings */
+		ret = adv76xx_query_dv_timings(sd, &state->timings);
+		if (ret = -ENOLINK) {
+			/* no signal, fall back to default timings */
+			state->timings = (struct v4l2_dv_timings)
+				V4L2_DV_BT_CEA_640X480P59_94;
+		}
+
 		if (handled)
 			*handled = true;
 	}
-- 
2.6.3

Re: [PATCH v2 2/2] media: adv7604: update timings on change of input signal

[Laurent Pinchart]

Hi Ulrich,

(With a question for Hans below)

Thank you for the patch.

On Tuesday 22 December 2015 15:22:02 Ulrich Hecht wrote:
> Without this, .get_selection will always return the boot-time state.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/i2c/adv7604.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 8ad5c28..dcd659b 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1945,6 +1945,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32
> status, bool *handled) u8 fmt_change_digital;
>  	u8 fmt_change;
>  	u8 tx_5v;
> +	int ret;
> 
>  	if (irq_reg_0x43)
>  		io_write(sd, 0x44, irq_reg_0x43);
> @@ -1968,6 +1969,14 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32
> status, bool *handled)
> 
>  		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
> 
> +		/* update timings */
> +		ret = adv76xx_query_dv_timings(sd, &state->timings);

I'm not too familiar with the DV timings API, but I'm not sure this is 
correct. This would result in g_dv_timings returning the detected timings, 
while we have the dedicated query_dv_timings operation for that. Hans, could 
you comment on this ? How do query_dv_timings and g_dv_timings interact ? The 
API documentation isn't very clear about that.

> +		if (ret = -ENOLINK) {
> +			/* no signal, fall back to default timings */
> +			state->timings = (struct v4l2_dv_timings)
> +				V4L2_DV_BT_CEA_640X480P59_94;
> +		}
> +
>  		if (handled)
>  			*handled = true;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] media: adv7604: update timings on change of input signal

[hverkuil]

On 2015-12-24 12:47, Laurent Pinchart wrote:
> Hi Ulrich,
> 
> (With a question for Hans below)
> 
> Thank you for the patch.
> 
> On Tuesday 22 December 2015 15:22:02 Ulrich Hecht wrote:
>> Without this, .get_selection will always return the boot-time state.
>> 
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> ---
>>  drivers/media/i2c/adv7604.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>> index 8ad5c28..dcd659b 100644
>> --- a/drivers/media/i2c/adv7604.c
>> +++ b/drivers/media/i2c/adv7604.c
>> @@ -1945,6 +1945,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, 
>> u32
>> status, bool *handled) u8 fmt_change_digital;
>>  	u8 fmt_change;
>>  	u8 tx_5v;
>> +	int ret;
>> 
>>  	if (irq_reg_0x43)
>>  		io_write(sd, 0x44, irq_reg_0x43);
>> @@ -1968,6 +1969,14 @@ static int adv76xx_isr(struct v4l2_subdev *sd, 
>> u32
>> status, bool *handled)
>> 
>>  		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
>> 
>> +		/* update timings */
>> +		ret = adv76xx_query_dv_timings(sd, &state->timings);
> 
> I'm not too familiar with the DV timings API, but I'm not sure this is
> correct. This would result in g_dv_timings returning the detected 
> timings,
> while we have the dedicated query_dv_timings operation for that. Hans, 
> could
> you comment on this ? How do query_dv_timings and g_dv_timings interact 
> ? The
> API documentation isn't very clear about that.

This code is wrong. If a format/timings change is detected the driver 
can send
an event, but it should never change the current timings. The 
application has
to call the query function first, then set the detected new timings. You 
can't
just change timings on the fly since that often means changes to buffer 
sizes
as well, and changing buffer sizes on the fly is very dangerous.

So if a change is detected, send the V4L2_EVENT_SOURCE_CHANGE event. In 
response
the application will call QUERY_DV_TIMINGS and, if valid new timings are 
detected,
S_DV_TIMINGS. Usually this will require the stop streaming, set the new 
timings,
allocate new buffers and start streaming sequence.

Regards,

     Hans

> 
>> +		if (ret = -ENOLINK) {
>> +			/* no signal, fall back to default timings */
>> +			state->timings = (struct v4l2_dv_timings)
>> +				V4L2_DV_BT_CEA_640X480P59_94;
>> +		}
>> +
>>  		if (handled)
>>  			*handled = true;
>>  	}

Re: [PATCH v2 2/2] media: adv7604: update timings on change of input signal

[Hans Verkuil]

On 12/22/2015 03:22 PM, Ulrich Hecht wrote:
> Without this, .get_selection will always return the boot-time state.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/i2c/adv7604.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 8ad5c28..dcd659b 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1945,6 +1945,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>  	u8 fmt_change_digital;
>  	u8 fmt_change;
>  	u8 tx_5v;
> +	int ret;
>  
>  	if (irq_reg_0x43)
>  		io_write(sd, 0x44, irq_reg_0x43);
> @@ -1968,6 +1969,14 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>  
>  		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
>  
> +		/* update timings */
> +		ret = adv76xx_query_dv_timings(sd, &state->timings);
> +		if (ret = -ENOLINK) {
> +			/* no signal, fall back to default timings */
> +			state->timings = (struct v4l2_dv_timings)
> +				V4L2_DV_BT_CEA_640X480P59_94;
> +		}

Nack.

If you detect a change in timings, then send the V4L2_EVENT_SOURCE_CHANGE event and leave
it to the app to query the new timings. Never do this in the driver itself.

The reason is that this will pull the rug out from under the application: the app thinks
it is using timings A but the driver is using timings B. Instead, tell the app that the
timings have changed and let the app handle this.

Regards,

	Hans

> +
>  		if (handled)
>  			*handled = true;
>  	}
> 

Re: [PATCH v2 2/2] media: adv7604: update timings on change of input signal

[Ulrich Hecht]

On Mon, Jan 11, 2016 at 11:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> If you detect a change in timings, then send the V4L2_EVENT_SOURCE_CHANGE event and leave
> it to the app to query the new timings. Never do this in the driver itself.
>
> The reason is that this will pull the rug out from under the application: the app thinks
> it is using timings A but the driver is using timings B. Instead, tell the app that the
> timings have changed and let the app handle this.

Thank you for your review.  It appears that code to that effect is in
the driver already, and all that is necessary to make it work (on the
adv7604 side) is the interrupt enablement ("[PATCH v2] adv7604: add
direct interrupt handling"), which I will resend RSN.

CU
Uli