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@xxxxxxxxxx> wrote:
> >
> > On Mon, 20 May 2024 at 14:48, Sui Jingfeng <sui.jingfeng@xxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > >
> > > On 5/20/24 19:13, Dmitry Baryshkov wrote:
> > > > On Mon, 20 May 2024 at 14:11, Sui Jingfeng <sui.jingfeng@xxxxxxxxx> 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@xxxxxxx>
> > > >>>> ---
> > > >>>> 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
--
With best wishes
Dmitry
