[PATCH V2] drm/bridge: adv7511: Fix Intermittent EDID failures

Linux regression tracking (Thorsten Leemhuis) regressions at leemhuis.info
Mon Jun 17 13:29:49 UTC 2024


On 17.06.24 15:14, Adam Ford wrote:
> On Mon, Jun 17, 2024 at 8:00 AM Linux regression tracking (Thorsten
> Leemhuis) <regressions at leemhuis.info> wrote:
>>
>> [CCing the regression list, as it should be in the loop for regressions:
>> https://docs.kernel.org/admin-guide/reporting-regressions.html]
>>
>> Hi! Top-posting for once, to make this easily accessible to everyone.
>>
>> Hmm, seem nobody took a look at below fix for a regression that seems to
>> be caused by f3d9683346d6b1 ("drm/bridge: adv7511: Allow IRQ to share
>> GPIO pins") [which went into v6.10-rc1].
>>
>> Adam and Dimitry, what are your stances on this patch from Adam? I'm
>> asking, as you authored respectively committed the culprit?
> 
> I learned of the regression from Liu Ying [...]

Ohh, I'm very sorry, stupid me somehow missed that the Adam that was
posting the fix was the same Adam that authored the culprit. :-( Seems I
definitely need more coffee (or green tea in my case) or reduce the
number or regressions on the stack. Please accept my apologies.

Thx for the update anyway.

> Dimitry had given me some suggestions, and from that,  I posted a V1.
> Dmitry had some more followup suggestions [2] which resulted in the
> V2.
>> As far as I know, Liu was satisfied that this addressed the regression
> he reported.

So in that case the main question afaics is why this fix did not make
any progress for more than two weeks now (at least afaics -- or did I
miss something in that area, too?).

Ciao, Thorsten

>> On 01.06.24 15:24, Adam Ford wrote:
>>> In the process of adding support for shared IRQ pins, a scenario
>>> was accidentally created where adv7511_irq_process returned
>>> prematurely causing the EDID to fail randomly.
>>>
>>> Since the interrupt handler is broken up into two main helper functions,
>>> update both of them to treat the helper functions as IRQ handlers. These
>>> IRQ routines process their respective tasks as before, but if they
>>> determine that actual work was done, mark the respective IRQ status
>>> accordingly, and delay the check until everything has been processed.
>>>
>>> This should guarantee the helper functions don't return prematurely
>>> while still returning proper values of either IRQ_HANDLED or IRQ_NONE.
>>>
>>> Reported-by: Liu Ying <victor.liu at nxp.com>
>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
>>> Signed-off-by: Adam Ford <aford173 at gmail.com>
>>> Tested-by: Liu Ying <victor.liu at nxp.com> # i.MX8MP EVK ADV7535 EDID retrieval w/o IRQ
>>> ---
>>> V2:  Fix uninitialized cec_status
>>>      Cut back a little on error handling to return either IRQ_NONE or
>>>      IRQ_HANDLED.
>>>
>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>> index ea271f62b214..ec0b7f3d889c 100644
>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>> @@ -401,7 +401,7 @@ struct adv7511 {
>>>
>>>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>>>  int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
>>> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>>> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>>>  #else
>>>  static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>>>  {
>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>> index 44451a9658a3..651fb1dde780 100644
>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>> @@ -119,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
>>>       cec_received_msg(adv7511->cec_adap, &msg);
>>>  }
>>>
>>> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>>  {
>>>       unsigned int offset = adv7511->info->reg_cec_offset;
>>>       const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
>>> @@ -130,17 +130,21 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>>                               ADV7511_INT1_CEC_RX_READY3;
>>>       unsigned int rx_status;
>>>       int rx_order[3] = { -1, -1, -1 };
>>> -     int i;
>>> +     int i, ret = 0;
>>> +     int irq_status = IRQ_NONE;
>>>
>>> -     if (irq1 & irq_tx_mask)
>>> +     if (irq1 & irq_tx_mask) {
>>>               adv_cec_tx_raw_status(adv7511, irq1);
>>> +             irq_status = IRQ_HANDLED;
>>> +     }
>>>
>>>       if (!(irq1 & irq_rx_mask))
>>> -             return;
>>> +             return irq_status;
>>>
>>> -     if (regmap_read(adv7511->regmap_cec,
>>> -                     ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
>>> -             return;
>>> +     ret = regmap_read(adv7511->regmap_cec,
>>> +                     ADV7511_REG_CEC_RX_STATUS + offset, &rx_status);
>>> +     if (ret < 0)
>>> +             return irq_status;
>>>
>>>       /*
>>>        * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
>>> @@ -172,6 +176,8 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>>
>>>               adv7511_cec_rx(adv7511, rx_buf);
>>>       }
>>> +
>>> +     return IRQ_HANDLED;
>>>  }
>>>
>>>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> index 66ccb61e2a66..c8d2c4a157b2 100644
>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> @@ -469,6 +469,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>>>  {
>>>       unsigned int irq0, irq1;
>>>       int ret;
>>> +     int cec_status = IRQ_NONE;
>>> +     int irq_status = IRQ_NONE;
>>>
>>>       ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
>>>       if (ret < 0)
>>> @@ -478,29 +480,31 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>>>       if (ret < 0)
>>>               return ret;
>>>
>>> -     /* 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))
>>> -             return -ENODATA;
>>> -
>>>       regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
>>>       regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>>>
>>> -     if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
>>> +     if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) {
>>>               schedule_work(&adv7511->hpd_work);
>>> +             irq_status = IRQ_HANDLED;
>>> +     }
>>>
>>>       if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
>>>               adv7511->edid_read = true;
>>>
>>>               if (adv7511->i2c_main->irq)
>>>                       wake_up_all(&adv7511->wq);
>>> +             irq_status = IRQ_HANDLED;
>>>       }
>>>
>>>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>>> -     adv7511_cec_irq_process(adv7511, irq1);
>>> +     cec_status = adv7511_cec_irq_process(adv7511, irq1);
>>>  #endif
>>>
>>> -     return 0;
>>> +     /* If there is no IRQ to handle, exit indicating no IRQ data */
>>> +     if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
>>> +             return IRQ_HANDLED;
>>> +
>>> +     return IRQ_NONE;
>>>  }
>>>
>>>  static irqreturn_t adv7511_irq_handler(int irq, void *devid)
>>> @@ -509,7 +513,7 @@ static irqreturn_t adv7511_irq_handler(int irq, void *devid)
>>>       int ret;
>>>
>>>       ret = adv7511_irq_process(adv7511, true);
>>> -     return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
>>> +     return ret < 0 ? IRQ_NONE : ret;
>>>  }
>>>
>>>  /* -----------------------------------------------------------------------------
> 
> 


More information about the dri-devel mailing list