| Message ID | 20240516101006.2388767-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: adv7511: Exit interrupt handling when necessary | expand |
Hi, On 5/16/24 18:10, Liu Ying wrote: > Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > no interrupt requested at all. > > Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > because it polls adv7511->edid_read flag by calling adv7511_irq_process() > a few times, but adv7511_irq_process() happens to refuse to handle > interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > > Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > handling from adv7511_irq_process(). > > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 6089b0bb9321..2074fa3c1b7b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > return ret; > > /* If there is no IRQ to handle, exit indicating no IRQ data */ > - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > + if (adv7511->i2c_main->irq && Maybe you could use 'if (process_hpd)' here. > + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > !(irq1 & ADV7511_INT1_DDC_ERROR)) > return -ENODATA; >
Hi, On 5/16/24 18:10, Liu Ying wrote: > Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > no interrupt requested at all. > > Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > because it polls adv7511->edid_read flag by calling adv7511_irq_process() > a few times, but adv7511_irq_process() happens to refuse to handle > interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > > Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > handling from adv7511_irq_process(). > > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > Signed-off-by: Liu Ying <victor.liu@nxp.com> Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 6089b0bb9321..2074fa3c1b7b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > return ret; > > /* If there is no IRQ to handle, exit indicating no IRQ data */ > - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > + if (adv7511->i2c_main->irq && > + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > !(irq1 & ADV7511_INT1_DDC_ERROR)) > return -ENODATA; >
On Thu, May 16, 2024 at 5:01 AM Liu Ying <victor.liu@nxp.com> wrote: > > Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > no interrupt requested at all. Sorry about that. > > Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > because it polls adv7511->edid_read flag by calling adv7511_irq_process() > a few times, but adv7511_irq_process() happens to refuse to handle > interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > > Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > handling from adv7511_irq_process(). > > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > Signed-off-by: Liu Ying <victor.liu@nxp.com> Acked-by: Adam Ford <aford173@gmail.com> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 6089b0bb9321..2074fa3c1b7b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > return ret; > > /* If there is no IRQ to handle, exit indicating no IRQ data */ > - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > + if (adv7511->i2c_main->irq && > + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > !(irq1 & ADV7511_INT1_DDC_ERROR)) > return -ENODATA; > > -- > 2.37.1 >
On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > no interrupt requested at all. > > Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > because it polls adv7511->edid_read flag by calling adv7511_irq_process() > a few times, but adv7511_irq_process() happens to refuse to handle > interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > > Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > handling from adv7511_irq_process(). > > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 6089b0bb9321..2074fa3c1b7b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > return ret; > > /* If there is no IRQ to handle, exit indicating no IRQ data */ > - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > + if (adv7511->i2c_main->irq && > + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > !(irq1 & ADV7511_INT1_DDC_ERROR)) > return -ENODATA; I think it might be better to handle -ENODATA in adv7511_wait_for_edid() instead. WDYT?
On 5/20/24 06:11, Dmitry Baryshkov wrote: > On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: >> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., >> no interrupt requested at all. >> >> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, >> because it polls adv7511->edid_read flag by calling adv7511_irq_process() >> a few times, but adv7511_irq_process() happens to refuse to handle >> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. >> >> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt >> handling from adv7511_irq_process(). >> >> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> --- >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index 6089b0bb9321..2074fa3c1b7b 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) >> return ret; >> >> /* If there is no IRQ to handle, exit indicating no IRQ data */ >> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >> + if (adv7511->i2c_main->irq && >> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >> !(irq1 & ADV7511_INT1_DDC_ERROR)) >> return -ENODATA; > > I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > instead. WDYT? Then, adv7511_cec_irq_process() will have less chance to be called from adv7511_irq_process() (assuming CONFIG_DRM_I2C_ADV7511_CEC is defined) if adv7511->i2c_main->irq is zero. But, anyway, it seems that commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") is even more broken to handle the CEC case, as adv7511_cec_adap_enable() may enable some interrupts for CEC. This is a bit complicated. Thoughts? Regards, Liu Ying
On Mon, 20 May 2024 at 06:29, Liu Ying <victor.liu@nxp.com> wrote: > > On 5/20/24 06:11, Dmitry Baryshkov wrote: > > On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > >> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > >> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > >> no interrupt requested at all. > >> > >> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > >> because it polls adv7511->edid_read flag by calling adv7511_irq_process() > >> a few times, but adv7511_irq_process() happens to refuse to handle > >> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > >> > >> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > >> handling from adv7511_irq_process(). > >> > >> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > >> Signed-off-by: Liu Ying <victor.liu@nxp.com> > >> --- > >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> index 6089b0bb9321..2074fa3c1b7b 100644 > >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > >> return ret; > >> > >> /* If there is no IRQ to handle, exit indicating no IRQ data */ > >> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > >> + if (adv7511->i2c_main->irq && > >> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > >> !(irq1 & ADV7511_INT1_DDC_ERROR)) > >> return -ENODATA; > > > > I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > > instead. WDYT? > > Then, adv7511_cec_irq_process() will have less chance to be called from > adv7511_irq_process() (assuming CONFIG_DRM_I2C_ADV7511_CEC is defined) > if adv7511->i2c_main->irq is zero. > > But, anyway, it seems that commit f3d9683346d6 ("drm/bridge: adv7511: > Allow IRQ to share GPIO pins") is even more broken to handle the CEC case, > as adv7511_cec_adap_enable() may enable some interrupts for CEC. > > This is a bit complicated. Thoughts? Send a revert and do it properly? > > Regards, > Liu Ying > > > > >
On 5/20/24 17:08, Dmitry Baryshkov wrote: > On Mon, 20 May 2024 at 06:29, Liu Ying <victor.liu@nxp.com> wrote: >> >> On 5/20/24 06:11, Dmitry Baryshkov wrote: >>> On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: >>>> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >>>> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., >>>> no interrupt requested at all. >>>> >>>> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, >>>> because it polls adv7511->edid_read flag by calling adv7511_irq_process() >>>> a few times, but adv7511_irq_process() happens to refuse to handle >>>> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. >>>> >>>> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt >>>> handling from adv7511_irq_process(). >>>> >>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >>>> --- >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> index 6089b0bb9321..2074fa3c1b7b 100644 >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) >>>> return ret; >>>> >>>> /* If there is no IRQ to handle, exit indicating no IRQ data */ >>>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >>>> + if (adv7511->i2c_main->irq && >>>> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >>>> !(irq1 & ADV7511_INT1_DDC_ERROR)) >>>> return -ENODATA; >>> >>> I think it might be better to handle -ENODATA in adv7511_wait_for_edid() >>> instead. WDYT? >> >> Then, adv7511_cec_irq_process() will have less chance to be called from >> adv7511_irq_process() (assuming CONFIG_DRM_I2C_ADV7511_CEC is defined) >> if adv7511->i2c_main->irq is zero. >> >> But, anyway, it seems that commit f3d9683346d6 ("drm/bridge: adv7511: >> Allow IRQ to share GPIO pins") is even more broken to handle the CEC case, >> as adv7511_cec_adap_enable() may enable some interrupts for CEC. >> >> This is a bit complicated. Thoughts? > > Send a revert and do it properly? Good idea. Adam, can you do that?
Hi, On 5/20/24 06:11, Dmitry Baryshkov wrote: > On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: >> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., >> no interrupt requested at all. >> >> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, >> because it polls adv7511->edid_read flag by calling adv7511_irq_process() >> a few times, but adv7511_irq_process() happens to refuse to handle >> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. >> >> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt >> handling from adv7511_irq_process(). >> >> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> --- >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index 6089b0bb9321..2074fa3c1b7b 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) >> return ret; >> >> /* If there is no IRQ to handle, exit indicating no IRQ data */ >> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >> + if (adv7511->i2c_main->irq && >> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >> !(irq1 & ADV7511_INT1_DDC_ERROR)) >> return -ENODATA; > > I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > instead. WDYT? > I think this is may deserve another patch.
On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > On 5/20/24 06:11, Dmitry Baryshkov wrote: > > On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > >> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > >> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > >> no interrupt requested at all. > >> > >> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > >> because it polls adv7511->edid_read flag by calling adv7511_irq_process() > >> a few times, but adv7511_irq_process() happens to refuse to handle > >> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > >> > >> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > >> handling from adv7511_irq_process(). > >> > >> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > >> Signed-off-by: Liu Ying <victor.liu@nxp.com> > >> --- > >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> index 6089b0bb9321..2074fa3c1b7b 100644 > >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > >> return ret; > >> > >> /* If there is no IRQ to handle, exit indicating no IRQ data */ > >> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > >> + if (adv7511->i2c_main->irq && > >> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > >> !(irq1 & ADV7511_INT1_DDC_ERROR)) > >> return -ENODATA; > > > > I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > > instead. WDYT? > > > > I think this is may deserve another patch. My point is that the IRQ handler is fine to remove -ENODATA here, there is no pending IRQ that can be handled. So instead of continuing the execution when we know that IRQ bits are not set, it's better to ignore -ENODATA in the calling code and go on with msleep().
Hi, On 5/20/24 19:13, Dmitry Baryshkov wrote: > On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> >> Hi, >> >> On 5/20/24 06:11, Dmitry Baryshkov wrote: >>> On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: >>>> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >>>> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., >>>> no interrupt requested at all. >>>> >>>> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, >>>> because it polls adv7511->edid_read flag by calling adv7511_irq_process() >>>> a few times, but adv7511_irq_process() happens to refuse to handle >>>> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. >>>> >>>> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt >>>> handling from adv7511_irq_process(). >>>> >>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >>>> --- >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> index 6089b0bb9321..2074fa3c1b7b 100644 >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) >>>> return ret; >>>> >>>> /* If there is no IRQ to handle, exit indicating no IRQ data */ >>>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >>>> + if (adv7511->i2c_main->irq && >>>> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && >>>> !(irq1 & ADV7511_INT1_DDC_ERROR)) >>>> return -ENODATA; >>> >>> I think it might be better to handle -ENODATA in adv7511_wait_for_edid() >>> instead. WDYT? >>> >> >> I think this is may deserve another patch. > > My point is that the IRQ handler is fine to remove -ENODATA here, [...] > there is no pending IRQ that can be handled. But there may has other things need to do in the adv7511_irq_process() function. So instead of continuing > the execution when we know that IRQ bits are not set, Even when IRQ bits are not set, it just means that there is no HPD and no EDID ready-to-read signal. HDMI CEC interrupts still need to process. it's better to > ignore -ENODATA in the calling code and go on with msleep(). > So, It's confusing to ignore the -ENODATA here.
On Mon, 20 May 2024 at 14:48, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 5/20/24 19:13, Dmitry Baryshkov wrote: > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >> > >> Hi, > >> > >> On 5/20/24 06:11, Dmitry Baryshkov wrote: > >>> On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > >>>> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > >>>> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > >>>> no interrupt requested at all. > >>>> > >>>> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > >>>> because it polls adv7511->edid_read flag by calling adv7511_irq_process() > >>>> a few times, but adv7511_irq_process() happens to refuse to handle > >>>> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > >>>> > >>>> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > >>>> handling from adv7511_irq_process(). > >>>> > >>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> > >>>> --- > >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >>>> index 6089b0bb9321..2074fa3c1b7b 100644 > >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >>>> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > >>>> return ret; > >>>> > >>>> /* If there is no IRQ to handle, exit indicating no IRQ data */ > >>>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > >>>> + if (adv7511->i2c_main->irq && > >>>> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > >>>> !(irq1 & ADV7511_INT1_DDC_ERROR)) > >>>> return -ENODATA; > >>> > >>> I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > >>> instead. WDYT? > >>> > >> > >> I think this is may deserve another patch. > > > > My point is that the IRQ handler is fine to remove -ENODATA here, > > [...] > > > there is no pending IRQ that can be handled. > > But there may has other things need to do in the adv7511_irq_process() > function. But the function returns anyway. So, we know that the condition is broken. > > > So instead of continuing > > the execution when we know that IRQ bits are not set, > > Even when IRQ bits are not set, it just means that there is no HPD > and no EDID ready-to-read signal. HDMI CEC interrupts still need > to process. Yes. Let's get the CEC fixed. Then maybe we won't need this commit at all. > > > > it's better to > > ignore -ENODATA in the calling code and go on with msleep(). > > > > So, It's confusing to ignore the -ENODATA here. [BTW: you had quotation levels wrong in two places, I've fixed them]
On Mon, May 20, 2024 at 7:00 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 20 May 2024 at 14:48, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > > > Hi, > > > > > > On 5/20/24 19:13, Dmitry Baryshkov wrote: > > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > >> > > >> Hi, > > >> > > >> On 5/20/24 06:11, Dmitry Baryshkov wrote: > > >>> On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > > >>>> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > >>>> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > > >>>> no interrupt requested at all. > > >>>> > > >>>> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > > >>>> because it polls adv7511->edid_read flag by calling adv7511_irq_process() > > >>>> a few times, but adv7511_irq_process() happens to refuse to handle > > >>>> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. Sorry about that. I did some testing and didn't see any regressions, but if it was random, it's likely I just was lucky to not see it. > > >>>> > > >>>> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > > >>>> handling from adv7511_irq_process(). > > >>>> > > >>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> > > >>>> --- > > >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > >>>> index 6089b0bb9321..2074fa3c1b7b 100644 > > >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > >>>> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > > >>>> return ret; > > >>>> > > >>>> /* If there is no IRQ to handle, exit indicating no IRQ data */ > > >>>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > >>>> + if (adv7511->i2c_main->irq && > > >>>> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > >>>> !(irq1 & ADV7511_INT1_DDC_ERROR)) > > >>>> return -ENODATA; > > >>> > > >>> I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > > >>> instead. WDYT? > > >>> > > >> > > >> I think this is may deserve another patch. > > > > > > My point is that the IRQ handler is fine to remove -ENODATA here, > > > > [...] > > > > > there is no pending IRQ that can be handled. > > > > But there may has other things need to do in the adv7511_irq_process() > > function. > > But the function returns anyway. So, we know that the condition is broken. When I originally submitted the patch, I only added the shared IRQ flag without any IRQ condition checks, IRQ because I didn't want to break anything. The feedback I got was to check to see if the IRQ was intended by the device. My focus was the adv7511_drv.c file because that appears to be what the registered IRQ hander was, but after looking through adv7511_cec, I see that adv7511_cec_irq_process checks adv_cec_tx_raw_status and returns if there is nothing to do. Would it make sense to move the if statement did the following things: - Make adv7511_cec_irq_process return an int and modify it to return 0 in normal operation or return -ENODATA where there is nothing to do. It already has the checks in place to determine if there is work to do, so the impact here should be minimal. - Move the check I added on whether or not there is an interrupt to the very end of adv7511_irq_process just before the return 0. - Instead of blindly returning 0, modify the if statement to read the state of the return code of adv7511_cec_irq_process and the IRQ flags it already checks. If ADV7511_INT0_HPD, ADV7511_INT0_EDID_READY and ADV7511_INT1_DDC_ERROR are all not true and adv7511_cec_irq_process returned early, return ENODATA, but if any of the interrupts was present and adv7511_cec_irq_process did work, it would return 0. I think that would cover the situation where adv7511_cec_irq_process would get called, and also prevent a false return of the IRQ being handled when this part didn't handle anything. It would look something like: cec_irq = adv7511_cec_irq_process(adv7511, irq1); /* If there is no IRQ to handle, exit indicating no IRQ data */) if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && !(irq1 & ADV7511_INT1_DDC_ERROR) && cec_irq == -ENODATA) return -ENODATA; else return 0 OR... Another alternative to all this is to modify the check that I added to verify all the following flags which are currently checked in adv7511_cec_irq_process : ADV7511_INT1_CEC_TX_READY ADV7511_INT1_CEC_TX_ARBIT_LOST ADV7511_INT1_CEC_TX_RETRY_TIMEOUT ADV7511_INT1_CEC_RX_READY1 ADV7511_INT1_CEC_RX_READY2 ADV7511_INT1_CEC_RX_READY3 It would look something like: /* If there is no IRQ to handle, exit indicating no IRQ data */ if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && !(irq1 & ADV7511_INT1_DDC_ERROR) && !(irq1 & ADV7511_INT1_CEC_TX_READY) && !(irq1 & ADV7511_INT1_CEC_TX_ARBIT_LOST) && !(irq1 & ADV7511_INT1_CEC_TX_RETRY_TIMEOUT) && !(irq1 & ADV7511_INT1_CEC_RX_READY1) && !(irq1 & ADV7511_INT1_CEC_RX_READY2) && !(irq1 & ADV7511_INT1_CEC_RX_READY3)) return -ENODATA; Please let me know what is preferred or if there is a third possible solution. I can write up a patch with a fixes tag later today when I get back to my build machine. adam > > > > > > So instead of continuing > > > the execution when we know that IRQ bits are not set, > > > > Even when IRQ bits are not set, it just means that there is no HPD > > and no EDID ready-to-read signal. HDMI CEC interrupts still need > > to process. > > Yes. Let's get the CEC fixed. Then maybe we won't need this commit at all. > > > > > > > > it's better to > > > ignore -ENODATA in the calling code and go on with msleep(). > > > > > > > So, It's confusing to ignore the -ENODATA here. > > [BTW: you had quotation levels wrong in two places, I've fixed them] > > -- > With best wishes > Dmitry
On Mon, May 20, 2024 at 07:46:05AM -0500, Adam Ford wrote: > On Mon, May 20, 2024 at 7:00 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 20 May 2024 at 14:48, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > > > > > Hi, > > > > > > > > > On 5/20/24 19:13, Dmitry Baryshkov wrote: > > > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > > >> > > > >> Hi, > > > >> > > > >> On 5/20/24 06:11, Dmitry Baryshkov wrote: > > > >>> On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > > > >>>> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > > >>>> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > > > >>>> no interrupt requested at all. > > > >>>> > > > >>>> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > > > >>>> because it polls adv7511->edid_read flag by calling adv7511_irq_process() > > > >>>> a few times, but adv7511_irq_process() happens to refuse to handle > > > >>>> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > > Sorry about that. I did some testing and didn't see any regressions, > but if it was random, it's likely I just was lucky to not see it. > > > > >>>> > > > >>>> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > > > >>>> handling from adv7511_irq_process(). > > > >>>> > > > >>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > > >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > >>>> --- > > > >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > > >>>> > > > >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > >>>> index 6089b0bb9321..2074fa3c1b7b 100644 > > > >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > >>>> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > > > >>>> return ret; > > > >>>> > > > >>>> /* If there is no IRQ to handle, exit indicating no IRQ data */ > > > >>>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > > >>>> + if (adv7511->i2c_main->irq && > > > >>>> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > > >>>> !(irq1 & ADV7511_INT1_DDC_ERROR)) > > > >>>> return -ENODATA; > > > >>> > > > >>> I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > > > >>> instead. WDYT? > > > >>> > > > >> > > > >> I think this is may deserve another patch. > > > > > > > > My point is that the IRQ handler is fine to remove -ENODATA here, > > > > > > [...] > > > > > > > there is no pending IRQ that can be handled. > > > > > > But there may has other things need to do in the adv7511_irq_process() > > > function. > > > > But the function returns anyway. So, we know that the condition is broken. > > When I originally submitted the patch, I only added the shared IRQ > flag without any IRQ condition checks, IRQ because I didn't want to > break anything. The feedback I got was to check to see if the IRQ was > intended by the device. My focus was the adv7511_drv.c file because > that appears to be what the registered IRQ hander was, but after > looking through adv7511_cec, I see that adv7511_cec_irq_process checks > adv_cec_tx_raw_status and returns if there is nothing to do. > > Would it make sense to move the if statement did the following things: > > - Make adv7511_cec_irq_process return an int and modify it to return > 0 in normal operation or return -ENODATA where there is nothing to do. > It already has the checks in place to determine if there is work to > do, so the impact here should be minimal. > > - Move the check I added on whether or not there is an interrupt to > the very end of adv7511_irq_process just before the return 0. > > - Instead of blindly returning 0, modify the if statement to read the > state of the return code of adv7511_cec_irq_process and the IRQ flags > it already checks. If ADV7511_INT0_HPD, ADV7511_INT0_EDID_READY and > ADV7511_INT1_DDC_ERROR are all not true and adv7511_cec_irq_process > returned early, return ENODATA, but if any of the interrupts was > present and adv7511_cec_irq_process did work, it would return 0. > > I think that would cover the situation where adv7511_cec_irq_process > would get called, and also prevent a false return of the IRQ being > handled when this part didn't handle anything. > > It would look something like: > > cec_irq = adv7511_cec_irq_process(adv7511, irq1); > > /* If there is no IRQ to handle, exit indicating no IRQ data */) > if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > !(irq1 & ADV7511_INT1_DDC_ERROR) && > cec_irq == -ENODATA) > return -ENODATA; > else > return 0 > > > OR... > > > Another alternative to all this is to modify the check that I added to > verify all the following flags which are currently checked in > adv7511_cec_irq_process : > > ADV7511_INT1_CEC_TX_READY > ADV7511_INT1_CEC_TX_ARBIT_LOST > ADV7511_INT1_CEC_TX_RETRY_TIMEOUT > ADV7511_INT1_CEC_RX_READY1 > ADV7511_INT1_CEC_RX_READY2 > ADV7511_INT1_CEC_RX_READY3 > > It would look something like: > > /* If there is no IRQ to handle, exit indicating no IRQ data */ > if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > !(irq1 & ADV7511_INT1_DDC_ERROR) && > !(irq1 & ADV7511_INT1_CEC_TX_READY) && > !(irq1 & ADV7511_INT1_CEC_TX_ARBIT_LOST) && > !(irq1 & ADV7511_INT1_CEC_TX_RETRY_TIMEOUT) && > !(irq1 & ADV7511_INT1_CEC_RX_READY1) && > !(irq1 & ADV7511_INT1_CEC_RX_READY2) && > !(irq1 & ADV7511_INT1_CEC_RX_READY3)) > return -ENODATA; This definitely looks ugly. At least it should be a mask. I have a slightly different solution: Make adv7511_irq_process return <0 for error, IRQ_NONE or IRQ_HANDLED. This would also require tracking whether HPD, EDID or CEC processing actually took place (add temp var for the current 'handled' status, make adv7511_cec_irq_process() return IRQ_HANDLED too). > > > Please let me know what is preferred or if there is a third possible solution. > > I can write up a patch with a fixes tag later today when I get back to > my build machine. > > adam > > > > > > > > > So instead of continuing > > > > the execution when we know that IRQ bits are not set, > > > > > > Even when IRQ bits are not set, it just means that there is no HPD > > > and no EDID ready-to-read signal. HDMI CEC interrupts still need > > > to process. > > > > Yes. Let's get the CEC fixed. Then maybe we won't need this commit at all. > > > > > > > > > > > > it's better to > > > > ignore -ENODATA in the calling code and go on with msleep(). > > > > > > > > > > So, It's confusing to ignore the -ENODATA here. > > > > [BTW: you had quotation levels wrong in two places, I've fixed them] > > > > -- > > With best wishes > > Dmitry
On Mon, May 20, 2024 at 4:16 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, May 20, 2024 at 07:46:05AM -0500, Adam Ford wrote: > > On Mon, May 20, 2024 at 7:00 AM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Mon, 20 May 2024 at 14:48, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > > > > > > > Hi, > > > > > > > > > > > > On 5/20/24 19:13, Dmitry Baryshkov wrote: > > > > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > > > >> > > > > >> Hi, > > > > >> > > > > >> On 5/20/24 06:11, Dmitry Baryshkov wrote: > > > > >>> On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote: > > > > >>>> Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > > > >>>> fails to consider the case where adv7511->i2c_main->irq is zero, i.e., > > > > >>>> no interrupt requested at all. > > > > >>>> > > > > >>>> Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, > > > > >>>> because it polls adv7511->edid_read flag by calling adv7511_irq_process() > > > > >>>> a few times, but adv7511_irq_process() happens to refuse to handle > > > > >>>> interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. > > > > Sorry about that. I did some testing and didn't see any regressions, > > but if it was random, it's likely I just was lucky to not see it. > > > > > > >>>> > > > > >>>> Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt > > > > >>>> handling from adv7511_irq_process(). > > > > >>>> > > > > >>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > > > >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > >>>> --- > > > > >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > > > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > > > >>>> > > > > >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > >>>> index 6089b0bb9321..2074fa3c1b7b 100644 > > > > >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > >>>> @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > > > > >>>> return ret; > > > > >>>> > > > > >>>> /* If there is no IRQ to handle, exit indicating no IRQ data */ > > > > >>>> - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > > > >>>> + if (adv7511->i2c_main->irq && > > > > >>>> + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > > > >>>> !(irq1 & ADV7511_INT1_DDC_ERROR)) > > > > >>>> return -ENODATA; > > > > >>> > > > > >>> I think it might be better to handle -ENODATA in adv7511_wait_for_edid() > > > > >>> instead. WDYT? > > > > >>> > > > > >> > > > > >> I think this is may deserve another patch. > > > > > > > > > > My point is that the IRQ handler is fine to remove -ENODATA here, > > > > > > > > [...] > > > > > > > > > there is no pending IRQ that can be handled. > > > > > > > > But there may has other things need to do in the adv7511_irq_process() > > > > function. > > > > > > But the function returns anyway. So, we know that the condition is broken. > > > > When I originally submitted the patch, I only added the shared IRQ > > flag without any IRQ condition checks, IRQ because I didn't want to > > break anything. The feedback I got was to check to see if the IRQ was > > intended by the device. My focus was the adv7511_drv.c file because > > that appears to be what the registered IRQ hander was, but after > > looking through adv7511_cec, I see that adv7511_cec_irq_process checks > > adv_cec_tx_raw_status and returns if there is nothing to do. > > > > Would it make sense to move the if statement did the following things: > > > > - Make adv7511_cec_irq_process return an int and modify it to return > > 0 in normal operation or return -ENODATA where there is nothing to do. > > It already has the checks in place to determine if there is work to > > do, so the impact here should be minimal. > > > > - Move the check I added on whether or not there is an interrupt to > > the very end of adv7511_irq_process just before the return 0. > > > > - Instead of blindly returning 0, modify the if statement to read the > > state of the return code of adv7511_cec_irq_process and the IRQ flags > > it already checks. If ADV7511_INT0_HPD, ADV7511_INT0_EDID_READY and > > ADV7511_INT1_DDC_ERROR are all not true and adv7511_cec_irq_process > > returned early, return ENODATA, but if any of the interrupts was > > present and adv7511_cec_irq_process did work, it would return 0. > > > > I think that would cover the situation where adv7511_cec_irq_process > > would get called, and also prevent a false return of the IRQ being > > handled when this part didn't handle anything. > > > > It would look something like: > > > > cec_irq = adv7511_cec_irq_process(adv7511, irq1); > > > > /* If there is no IRQ to handle, exit indicating no IRQ data */) > > if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > !(irq1 & ADV7511_INT1_DDC_ERROR) && > > cec_irq == -ENODATA) > > return -ENODATA; > > else > > return 0 > > > > > > OR... > > > > > > Another alternative to all this is to modify the check that I added to > > verify all the following flags which are currently checked in > > adv7511_cec_irq_process : > > > > ADV7511_INT1_CEC_TX_READY > > ADV7511_INT1_CEC_TX_ARBIT_LOST > > ADV7511_INT1_CEC_TX_RETRY_TIMEOUT > > ADV7511_INT1_CEC_RX_READY1 > > ADV7511_INT1_CEC_RX_READY2 > > ADV7511_INT1_CEC_RX_READY3 > > > > It would look something like: > > > > /* If there is no IRQ to handle, exit indicating no IRQ data */ > > if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > > !(irq1 & ADV7511_INT1_DDC_ERROR) && > > !(irq1 & ADV7511_INT1_CEC_TX_READY) && > > !(irq1 & ADV7511_INT1_CEC_TX_ARBIT_LOST) && > > !(irq1 & ADV7511_INT1_CEC_TX_RETRY_TIMEOUT) && > > !(irq1 & ADV7511_INT1_CEC_RX_READY1) && > > !(irq1 & ADV7511_INT1_CEC_RX_READY2) && > > !(irq1 & ADV7511_INT1_CEC_RX_READY3)) > > return -ENODATA; > > This definitely looks ugly. At least it should be a mask. > > I have a slightly different solution: > > Make adv7511_irq_process return <0 for error, IRQ_NONE or IRQ_HANDLED. > This would also require tracking whether HPD, EDID or CEC processing > actually took place (add temp var for the current 'handled' status, make > adv7511_cec_irq_process() return IRQ_HANDLED too). Dmitry, I think I have addressed your concerns. I got feedback from a build bot with one warning, so I'll address that, but i wasn't sure if I should wait for feedback from you. I am traveling Friday-Tuesday, so I was hoping to send a V2 on Thursday if there are no other concerns. Liu, I realized that I didn't properly copy-paste your e-mail address, so you didn't get copied on the submission I just did. I'll reply to the thread where I posted a bug fix with your proper e-mail address. If you can try it to see if it addresses your issue, that would be really helpful. thanks, adam > > > > > > > Please let me know what is preferred or if there is a third possible solution. > > > > I can write up a patch with a fixes tag later today when I get back to > > my build machine. > > > > adam > > > > > > > > > > > > So instead of continuing > > > > > the execution when we know that IRQ bits are not set, > > > > > > > > Even when IRQ bits are not set, it just means that there is no HPD > > > > and no EDID ready-to-read signal. HDMI CEC interrupts still need > > > > to process. > > > > > > Yes. Let's get the CEC fixed. Then maybe we won't need this commit at all. > > > > > > > > > > > > > > > > it's better to > > > > > ignore -ENODATA in the calling code and go on with msleep(). > > > > > > > > > > > > > So, It's confusing to ignore the -ENODATA here. > > > > > > [BTW: you had quotation levels wrong in two places, I've fixed them] > > > > > > -- > > > With best wishes > > > Dmitry > > -- > With best wishes > Dmitry
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 6089b0bb9321..2074fa3c1b7b 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) return ret; /* If there is no IRQ to handle, exit indicating no IRQ data */ - if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && + if (adv7511->i2c_main->irq && + !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && !(irq1 & ADV7511_INT1_DDC_ERROR)) return -ENODATA;
Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") fails to consider the case where adv7511->i2c_main->irq is zero, i.e., no interrupt requested at all. Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes, because it polls adv7511->edid_read flag by calling adv7511_irq_process() a few times, but adv7511_irq_process() happens to refuse to handle interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly. Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt handling from adv7511_irq_process(). Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") Signed-off-by: Liu Ying <victor.liu@nxp.com> --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)