From: Adam Ford <aford173@gmail.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Sui Jingfeng <sui.jingfeng@linux.dev>,
Liu Ying <victor.liu@nxp.com>,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
andrzej.hajda@intel.com, neil.armstrong@linaro.org,
rfoss@kernel.org, Laurent.pinchart@ideasonboard.com,
jonas@kwiboo.se, jernej.skrabec@gmail.com,
maarten.lankhorst@linux.intel.com, mripard@kernel.org,
tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch,
biju.das.jz@bp.renesas.com, u.kleine-koenig@pengutronix.de,
jani.nikula@intel.com, bli@bang-olufsen.dk
Subject: Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
Date: Mon, 20 May 2024 07:46:05 -0500 [thread overview]
Message-ID: <CAHCN7x+5L+fjkDgR_mJ2BQ1M52oBZyU0nUca1Uvhyh1pFSJChw@mail.gmail.com> (raw)
In-Reply-To: <CAA8EJpowLnrnV8ezXYj=oWBFFTa6LqkPFFHc9F49un6AYgZvzw@mail.gmail.com>
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
next prev parent reply other threads:[~2024-05-20 12:46 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-16 10:10 [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary Liu Ying
2024-05-16 11:07 ` Sui Jingfeng
2024-05-16 12:21 ` Sui Jingfeng
2024-05-16 12:22 ` Adam Ford
2024-05-19 22:11 ` Dmitry Baryshkov
2024-05-20 3:29 ` Liu Ying
2024-05-20 9:08 ` Dmitry Baryshkov
2024-05-20 9:26 ` Liu Ying
2024-05-20 11:11 ` Sui Jingfeng
2024-05-20 11:13 ` Dmitry Baryshkov
2024-05-20 11:48 ` Sui Jingfeng
2024-05-20 12:00 ` Dmitry Baryshkov
2024-05-20 12:46 ` Adam Ford [this message]
2024-05-20 21:16 ` Dmitry Baryshkov
2024-05-21 21:49 ` Adam Ford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHCN7x+5L+fjkDgR_mJ2BQ1M52oBZyU0nUca1Uvhyh1pFSJChw@mail.gmail.com \
--to=aford173@gmail.com \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=airlied@gmail.com \
--cc=andrzej.hajda@intel.com \
--cc=biju.das.jz@bp.renesas.com \
--cc=bli@bang-olufsen.dk \
--cc=daniel@ffwll.ch \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=jernej.skrabec@gmail.com \
--cc=jonas@kwiboo.se \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=rfoss@kernel.org \
--cc=sui.jingfeng@linux.dev \
--cc=tzimmermann@suse.de \
--cc=u.kleine-koenig@pengutronix.de \
--cc=victor.liu@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).