[PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Liu Ying]

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;
 
-- 
2.37.1

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Sui Jingfeng]

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;
>   

-- 
Best regards
Sui

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Sui Jingfeng]

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;
>   

-- 
Best regards
Sui

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Adam Ford]

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
>

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Dmitry Baryshkov]

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?

-- 
With best wishes
Dmitry

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Liu Ying]

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

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Dmitry Baryshkov]

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
>
>
>
>
>


-- 
With best wishes
Dmitry

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[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?

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Sui Jingfeng]

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.

-- 
Best regards
Sui

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Dmitry Baryshkov]

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().

-- 
With best wishes
Dmitry

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Sui Jingfeng]

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.

-- 
Best regards
Sui

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Dmitry Baryshkov]

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]

-- 
With best wishes
Dmitry

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Adam Ford]

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

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary

[Adam Ford]

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