| Message ID | 1469471939-25393-1-git-send-email-aospan@netup.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 25, 2016 at 2:38 PM, Abylay Ospan <aospan@netup.ru> wrote: > inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. > This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. > > Signed-off-by: Abylay Ospan <aospan@netup.ru> > --- > drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c > index 179c26e..dad7ad3 100644 > --- a/drivers/media/dvb-frontends/lgdt3306a.c > +++ b/drivers/media/dvb-frontends/lgdt3306a.c > @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, > static int lgdt3306a_search(struct dvb_frontend *fe) > { > enum fe_status status = 0; > - int i, ret; > + int ret; > > /* set frontend */ > ret = lgdt3306a_set_parameters(fe); > if (ret) > goto error; > > - /* wait frontend lock */ > - for (i = 20; i > 0; i--) { > - dbg_info(": loop=%d\n", i); > - msleep(50); > - ret = lgdt3306a_read_status(fe, &status); > - if (ret) > - goto error; > - > - if (status & FE_HAS_LOCK) > - break; > - } > + ret = lgdt3306a_read_status(fe, &status); > + if (ret) > + goto error; > > /* check if we have a valid signal */ > if (status & FE_HAS_LOCK) Your patch removes a loop that was purposefully written here to handle conditions that are not ideal. Are you sure this change is best for all users? I would disagree with merging this patch. Best regards, Michael Ira Krufky -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michael, thanks for update. ok, I will investigate this more heavily later. Please, do not merge this patch. Seems like we need more consultation/testing (as I understand especially on 'weak' ATSC signals). 2016-07-25 14:55 GMT-04:00 Michael Ira Krufky <mkrufky@linuxtv.org>: > On Mon, Jul 25, 2016 at 2:38 PM, Abylay Ospan <aospan@netup.ru> wrote: >> inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. >> This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. >> >> Signed-off-by: Abylay Ospan <aospan@netup.ru> >> --- >> drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ >> 1 file changed, 4 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c >> index 179c26e..dad7ad3 100644 >> --- a/drivers/media/dvb-frontends/lgdt3306a.c >> +++ b/drivers/media/dvb-frontends/lgdt3306a.c >> @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, >> static int lgdt3306a_search(struct dvb_frontend *fe) >> { >> enum fe_status status = 0; >> - int i, ret; >> + int ret; >> >> /* set frontend */ >> ret = lgdt3306a_set_parameters(fe); >> if (ret) >> goto error; >> >> - /* wait frontend lock */ >> - for (i = 20; i > 0; i--) { >> - dbg_info(": loop=%d\n", i); >> - msleep(50); >> - ret = lgdt3306a_read_status(fe, &status); >> - if (ret) >> - goto error; >> - >> - if (status & FE_HAS_LOCK) >> - break; >> - } >> + ret = lgdt3306a_read_status(fe, &status); >> + if (ret) >> + goto error; >> >> /* check if we have a valid signal */ >> if (status & FE_HAS_LOCK) > > Your patch removes a loop that was purposefully written here to handle > conditions that are not ideal. Are you sure this change is best for > all users? > > I would disagree with merging this patch. > > Best regards, > > Michael Ira Krufky
Hi Michael, Em Mon, 25 Jul 2016 14:55:51 -0400 Michael Ira Krufky <mkrufky@linuxtv.org> escreveu: > On Mon, Jul 25, 2016 at 2:38 PM, Abylay Ospan <aospan@netup.ru> wrote: > > inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. > > This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. > > > > Signed-off-by: Abylay Ospan <aospan@netup.ru> > > --- > > drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c > > index 179c26e..dad7ad3 100644 > > --- a/drivers/media/dvb-frontends/lgdt3306a.c > > +++ b/drivers/media/dvb-frontends/lgdt3306a.c > > @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, > > static int lgdt3306a_search(struct dvb_frontend *fe) > > { > > enum fe_status status = 0; > > - int i, ret; > > + int ret; > > > > /* set frontend */ > > ret = lgdt3306a_set_parameters(fe); > > if (ret) > > goto error; > > > > - /* wait frontend lock */ > > - for (i = 20; i > 0; i--) { > > - dbg_info(": loop=%d\n", i); > > - msleep(50); > > - ret = lgdt3306a_read_status(fe, &status); > > - if (ret) > > - goto error; > > - > > - if (status & FE_HAS_LOCK) > > - break; > > - } Could you please explain why lgdt3306a needs the above ugly hack? > > + ret = lgdt3306a_read_status(fe, &status); > > + if (ret) > > + goto error; > > > > /* check if we have a valid signal */ > > if (status & FE_HAS_LOCK) > > Your patch removes a loop that was purposefully written here to handle > conditions that are not ideal. Are you sure this change is best for > all users? > > I would disagree with merging this patch. > > Best regards, > > Michael Ira Krufky
On Mon, Jul 25, 2016 at 3:28 PM, Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote: > Hi Michael, > > Em Mon, 25 Jul 2016 14:55:51 -0400 > Michael Ira Krufky <mkrufky@linuxtv.org> escreveu: > >> On Mon, Jul 25, 2016 at 2:38 PM, Abylay Ospan <aospan@netup.ru> wrote: >> > inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. >> > This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. >> > >> > Signed-off-by: Abylay Ospan <aospan@netup.ru> >> > --- >> > drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ >> > 1 file changed, 4 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c >> > index 179c26e..dad7ad3 100644 >> > --- a/drivers/media/dvb-frontends/lgdt3306a.c >> > +++ b/drivers/media/dvb-frontends/lgdt3306a.c >> > @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, >> > static int lgdt3306a_search(struct dvb_frontend *fe) >> > { >> > enum fe_status status = 0; >> > - int i, ret; >> > + int ret; >> > >> > /* set frontend */ >> > ret = lgdt3306a_set_parameters(fe); >> > if (ret) >> > goto error; >> > >> > - /* wait frontend lock */ >> > - for (i = 20; i > 0; i--) { >> > - dbg_info(": loop=%d\n", i); >> > - msleep(50); >> > - ret = lgdt3306a_read_status(fe, &status); >> > - if (ret) >> > - goto error; >> > - >> > - if (status & FE_HAS_LOCK) >> > - break; >> > - } > > Could you please explain why lgdt3306a needs the above ugly hack? > > >> > + ret = lgdt3306a_read_status(fe, &status); >> > + if (ret) >> > + goto error; > > >> > >> > /* check if we have a valid signal */ >> > if (status & FE_HAS_LOCK) >> >> Your patch removes a loop that was purposefully written here to handle >> conditions that are not ideal. Are you sure this change is best for >> all users? >> >> I would disagree with merging this patch. >> >> Best regards, >> >> Michael Ira Krufky Mauro, I cannot speak for the LG DT3306a part itself, but based on my past experience I can say the following: To my understanding, the hardware might not report a lock on the first read_status request, so the driver author chose to include a loop to retry a few times before giving up. In real life scenarios, there are marginal signals that may take a longer time to lock onto, but once locked, the demod will deliver a reliable stream. Most applications will only issue a single tune request when trying to tune to a given program. The application does not retry the tune request if the driver reports no lock. Applying this patch will have the potential to cause userspace to appear broken. Some users will not be able to receive some weaker channels anymore, and they will have no way to diagnose the problem from within their application. -Mike -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Mon, 25 Jul 2016 15:37:14 -0400 Michael Ira Krufky <mkrufky@linuxtv.org> escreveu: > On Mon, Jul 25, 2016 at 3:28 PM, Mauro Carvalho Chehab > <mchehab@osg.samsung.com> wrote: > > Hi Michael, > > > > Em Mon, 25 Jul 2016 14:55:51 -0400 > > Michael Ira Krufky <mkrufky@linuxtv.org> escreveu: > > > >> On Mon, Jul 25, 2016 at 2:38 PM, Abylay Ospan <aospan@netup.ru> wrote: > >> > inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. > >> > This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. > >> > > >> > Signed-off-by: Abylay Ospan <aospan@netup.ru> > >> > --- > >> > drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ > >> > 1 file changed, 4 insertions(+), 12 deletions(-) > >> > > >> > diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c > >> > index 179c26e..dad7ad3 100644 > >> > --- a/drivers/media/dvb-frontends/lgdt3306a.c > >> > +++ b/drivers/media/dvb-frontends/lgdt3306a.c > >> > @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, > >> > static int lgdt3306a_search(struct dvb_frontend *fe) > >> > { > >> > enum fe_status status = 0; > >> > - int i, ret; > >> > + int ret; > >> > > >> > /* set frontend */ > >> > ret = lgdt3306a_set_parameters(fe); > >> > if (ret) > >> > goto error; > >> > > >> > - /* wait frontend lock */ > >> > - for (i = 20; i > 0; i--) { > >> > - dbg_info(": loop=%d\n", i); > >> > - msleep(50); > >> > - ret = lgdt3306a_read_status(fe, &status); > >> > - if (ret) > >> > - goto error; > >> > - > >> > - if (status & FE_HAS_LOCK) > >> > - break; > >> > - } > > > > Could you please explain why lgdt3306a needs the above ugly hack? > > > > > >> > + ret = lgdt3306a_read_status(fe, &status); > >> > + if (ret) > >> > + goto error; > > > > > >> > > >> > /* check if we have a valid signal */ > >> > if (status & FE_HAS_LOCK) > >> > >> Your patch removes a loop that was purposefully written here to handle > >> conditions that are not ideal. Are you sure this change is best for > >> all users? > >> > >> I would disagree with merging this patch. > >> > >> Best regards, > >> > >> Michael Ira Krufky > > Mauro, > > I cannot speak for the LG DT3306a part itself, but based on my past > experience I can say the following: > > To my understanding, the hardware might not report a lock on the first > read_status request, so the driver author chose to include a loop to > retry a few times before giving up. A one second wait, trying 50 times is not a "few times". It is a lot! > In real life scenarios, there are marginal signals that may take a > longer time to lock onto, but once locked, the demod will deliver a > reliable stream. > > Most applications will only issue a single tune request when trying to > tune to a given program. The application does not retry the tune > request if the driver reports no lock. I don't know a single application that would give up after a single status request with FE_READ_STATUS. Not even simple applications like the legacy dvb-tools do that. If such application exits, it is already broken, as it would fail with most drivers, as almost no drivers wait for frontend locks. Also, the frontend thread assumes that the lock will take some polls to happen, and it keep polling the status for some time, using the status return to do frequency zig-zag, on tuners that don't have hardware zig-zag, and to try bandwidth inversion. Please notice that some legacy DVBv3 applications might want to be bothered only after lock. In such case, they would be calling FE_GET_EVENT with the device opened in blocking mode: https://linuxtv.org/downloads/v4l-dvb-apis-new/media/uapi/dvb/fe-get-event.html In such case, the frontend's kthread will keep the ioctl blocked until the device is locked, or will keep returning -EWOULDBLOCK in non-blocking mode. > Applying this patch will have the potential to cause userspace to > appear broken. Some users will not be able to receive some weaker > channels anymore, and they will have no way to diagnose the problem > from within their application. This is not how it is supposed to work. An ioctl should not block for that long time for no reason, specially since the file descriptor could be opened in no blocking mode. The only possible reason to block would be on really broken hardware that would stop working if the status is called before a certain number of milliseconds. Even so, the proper implementation would be add some logic at the driver level to ensure that the hardware won't be receiving the status command when it is not ready to answer to it. Some drivers do that. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 25, 2016 at 9:36 PM, Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote: > Em Mon, 25 Jul 2016 15:37:14 -0400 > Michael Ira Krufky <mkrufky@linuxtv.org> escreveu: > >> On Mon, Jul 25, 2016 at 3:28 PM, Mauro Carvalho Chehab >> <mchehab@osg.samsung.com> wrote: >> > Hi Michael, >> > >> > Em Mon, 25 Jul 2016 14:55:51 -0400 >> > Michael Ira Krufky <mkrufky@linuxtv.org> escreveu: >> > >> >> On Mon, Jul 25, 2016 at 2:38 PM, Abylay Ospan <aospan@netup.ru> wrote: >> >> > inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. >> >> > This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. >> >> > >> >> > Signed-off-by: Abylay Ospan <aospan@netup.ru> >> >> > --- >> >> > drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ >> >> > 1 file changed, 4 insertions(+), 12 deletions(-) >> >> > >> >> > diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c >> >> > index 179c26e..dad7ad3 100644 >> >> > --- a/drivers/media/dvb-frontends/lgdt3306a.c >> >> > +++ b/drivers/media/dvb-frontends/lgdt3306a.c >> >> > @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, >> >> > static int lgdt3306a_search(struct dvb_frontend *fe) >> >> > { >> >> > enum fe_status status = 0; >> >> > - int i, ret; >> >> > + int ret; >> >> > >> >> > /* set frontend */ >> >> > ret = lgdt3306a_set_parameters(fe); >> >> > if (ret) >> >> > goto error; >> >> > >> >> > - /* wait frontend lock */ >> >> > - for (i = 20; i > 0; i--) { >> >> > - dbg_info(": loop=%d\n", i); >> >> > - msleep(50); >> >> > - ret = lgdt3306a_read_status(fe, &status); >> >> > - if (ret) >> >> > - goto error; >> >> > - >> >> > - if (status & FE_HAS_LOCK) >> >> > - break; >> >> > - } >> > >> > Could you please explain why lgdt3306a needs the above ugly hack? >> > >> > >> >> > + ret = lgdt3306a_read_status(fe, &status); >> >> > + if (ret) >> >> > + goto error; >> > >> > >> >> > >> >> > /* check if we have a valid signal */ >> >> > if (status & FE_HAS_LOCK) >> >> >> >> Your patch removes a loop that was purposefully written here to handle >> >> conditions that are not ideal. Are you sure this change is best for >> >> all users? >> >> >> >> I would disagree with merging this patch. >> >> >> >> Best regards, >> >> >> >> Michael Ira Krufky >> >> Mauro, >> >> I cannot speak for the LG DT3306a part itself, but based on my past >> experience I can say the following: >> >> To my understanding, the hardware might not report a lock on the first >> read_status request, so the driver author chose to include a loop to >> retry a few times before giving up. > > A one second wait, trying 50 times is not a "few times". It is a lot! > >> In real life scenarios, there are marginal signals that may take a >> longer time to lock onto, but once locked, the demod will deliver a >> reliable stream. >> >> Most applications will only issue a single tune request when trying to >> tune to a given program. The application does not retry the tune >> request if the driver reports no lock. > > I don't know a single application that would give up after a > single status request with FE_READ_STATUS. Not even simple > applications like the legacy dvb-tools do that. If such application > exits, it is already broken, as it would fail with most drivers, > as almost no drivers wait for frontend locks. > > Also, the frontend thread assumes that the lock will take some > polls to happen, and it keep polling the status for some time, > using the status return to do frequency zig-zag, on tuners that > don't have hardware zig-zag, and to try bandwidth inversion. > > Please notice that some legacy DVBv3 applications might want to > be bothered only after lock. In such case, they would be calling > FE_GET_EVENT with the device opened in blocking mode: > https://linuxtv.org/downloads/v4l-dvb-apis-new/media/uapi/dvb/fe-get-event.html > > In such case, the frontend's kthread will keep the ioctl blocked > until the device is locked, or will keep returning -EWOULDBLOCK > in non-blocking mode. > >> Applying this patch will have the potential to cause userspace to >> appear broken. Some users will not be able to receive some weaker >> channels anymore, and they will have no way to diagnose the problem >> from within their application. > > This is not how it is supposed to work. An ioctl should not block > for that long time for no reason, specially since the file > descriptor could be opened in no blocking mode. > > The only possible reason to block would be on really broken hardware > that would stop working if the status is called before a certain > number of milliseconds. Even so, the proper implementation would be > add some logic at the driver level to ensure that the hardware won't > be receiving the status command when it is not ready to answer to > it. Some drivers do that. Your argument seems reasonable. You can include my ack if you decide to merge the patch. Acked-by: Michael Ira Krufky <mkrufky@linuxtv.org> Cheers, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-media" 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/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c index 179c26e..dad7ad3 100644 --- a/drivers/media/dvb-frontends/lgdt3306a.c +++ b/drivers/media/dvb-frontends/lgdt3306a.c @@ -1737,24 +1737,16 @@ static int lgdt3306a_get_tune_settings(struct dvb_frontend *fe, static int lgdt3306a_search(struct dvb_frontend *fe) { enum fe_status status = 0; - int i, ret; + int ret; /* set frontend */ ret = lgdt3306a_set_parameters(fe); if (ret) goto error; - /* wait frontend lock */ - for (i = 20; i > 0; i--) { - dbg_info(": loop=%d\n", i); - msleep(50); - ret = lgdt3306a_read_status(fe, &status); - if (ret) - goto error; - - if (status & FE_HAS_LOCK) - break; - } + ret = lgdt3306a_read_status(fe, &status); + if (ret) + goto error; /* check if we have a valid signal */ if (status & FE_HAS_LOCK)
inside lgdt3306a_search we reading demod status 20 times with 50 msec sleep after each read. This gives us more than 1 sec of delay. Removing this delay should not affect demod functionality. Signed-off-by: Abylay Ospan <aospan@netup.ru> --- drivers/media/dvb-frontends/lgdt3306a.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)