Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Mon, May 20, 2024 at 4:16 PM Dmitry Baryshkov
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
> > wrote:
> > >
> > > On Mon, 20 May 2024 at 14:48, Sui Jingfeng wrote:
> > > >
> > > > Hi,
> > > >
> > > >
> > > > On 5/20/24 19:13, Dmitry Baryshkov wrote:
> > > > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng
> > > > > 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
> > > > ---
> > > > 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_
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Mon, May 20, 2024 at 07:46:05AM -0500, Adam Ford wrote:
> On Mon, May 20, 2024 at 7:00 AM Dmitry Baryshkov
> wrote:
> >
> > On Mon, 20 May 2024 at 14:48, Sui Jingfeng wrote:
> > >
> > > Hi,
> > >
> > >
> > > On 5/20/24 19:13, Dmitry Baryshkov wrote:
> > > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng
> > > > 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
> > > ---
> > > 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
>
>
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Mon, May 20, 2024 at 7:00 AM Dmitry Baryshkov
wrote:
>
> On Mon, 20 May 2024 at 14:48, Sui Jingfeng wrote:
> >
> > Hi,
> >
> >
> > On 5/20/24 19:13, Dmitry Baryshkov wrote:
> > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng 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
> > ---
> > 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 wo
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Mon, 20 May 2024 at 14:48, Sui Jingfeng wrote:
>
> Hi,
>
>
> On 5/20/24 19:13, Dmitry Baryshkov wrote:
> > On Mon, 20 May 2024 at 14:11, Sui Jingfeng 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
> ---
> 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]
--
With best wishes
Dmitry
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
Hi,
On 5/20/24 19:13, Dmitry Baryshkov wrote:
On Mon, 20 May 2024 at 14:11, Sui Jingfeng 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
---
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.
--
Best regards
Sui
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Mon, 20 May 2024 at 14:11, Sui Jingfeng 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
> >> ---
> >> 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().
--
With best wishes
Dmitry
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
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
---
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.
--
Best regards
Sui
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On 5/20/24 17:08, Dmitry Baryshkov wrote:
> On Mon, 20 May 2024 at 06:29, Liu Ying 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
---
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?
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Mon, 20 May 2024 at 06:29, Liu Ying 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
> >> ---
> >> 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
>
>
>
>
>
--
With best wishes
Dmitry
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
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
>> ---
>> 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
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
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
> ---
> 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?
--
With best wishes
Dmitry
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
On Thu, May 16, 2024 at 5:01 AM 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.
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
Acked-by: Adam Ford
> ---
> 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
>
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
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
Acked-by: Sui Jingfeng
---
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;
--
Best regards
Sui
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
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
---
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;
--
Best regards
Sui
