| Message ID | 1450794122-31293-3-git-send-email-ulrich.hecht+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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; > }
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; >> } -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(+)