Received-SPF: pass (google.com: domain of linux-kernel+bounces-185672-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <7bb4d582-5d90-465e-a241-1ee8439a5057@nxp.com>
Date: Wed, 22 May 2024 09:58:09 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
To: Adam Ford <aford173@gmail.com>, dri-devel@lists.freedesktop.org
Cc: dmitry.baryshkov@linaro.org, sui.jingfeng@linux.dev,
 aford@beaconembedded.com, Andrzej Hajda <andrzej.hajda@intel.com>,
 Neil Armstrong <neil.armstrong@linaro.org>, Robert Foss <rfoss@kernel.org>,
 Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
 Jonas Karlman <jonas@kwiboo.se>, Jernej Skrabec <jernej.skrabec@gmail.com>,
 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>,
 David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
 linux-kernel@vger.kernel.org
References: <20240521011614.496421-1-aford173@gmail.com>
 <CAHCN7xLekU9u0auzB+bt7cRgv48qxH8bRY2e-_nK0nUhaLJagA@mail.gmail.com>
Content-Language: en-US
From: Liu Ying <victor.liu@nxp.com>
In-Reply-To: <CAHCN7xLekU9u0auzB+bt7cRgv48qxH8bRY2e-_nK0nUhaLJagA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
 =?utf-8?B?YjNWc0lHaXBrclFoMEJyRnIyRFV0dEJpeVRxL2Vqbyt0aXBGQmt5bDdKMm9F?=
 =?utf-8?B?Q2trVEZZbVlkUDdoM3RObkZtUzlDL1dVNWdFc2xTYkZkMDRTNit1ZW43WXRI?=
 =?utf-8?B?UXlBMjRxaVIydTNhdnIyN1dhQ3ZlOGNTWU1waVBwRHFmWTF1RXVZK0wzQWo1?=
 =?utf-8?B?bWc2QmEySy9lcFUxNEk4QXVtenpGVDNsVHlyYXBYcHBNYUhGU1d2MmwrbWxh?=
 =?utf-8?B?ZjUzOHpuOTZpZncwNVIvQ1dtODQ5Q2MzRGtwak1qRXh3ZkxGVkpOcXBRWjBQ?=
 =?utf-8?B?NGVjT2d6d1JZQWQ4RWp2dzBRWkRTOUZTVEUzTTNVUjd3TEY5WDJNZ2R6OFJv?=
 =?utf-8?B?c1BEVVhJSHRKTEFEYWRFbTh6T2hEVTloeVg0bVIrVGU0czZlOGxza3VaV2Nt?=
 =?utf-8?B?ZW1BUnBXUCtKOVYwcTFWeUp0QXBCTi9kb1IvemtXZW9tU251S04vdXk1cWxO?=
 =?utf-8?B?blVXcVdLU1BXc0tUMGdtYzJZbGp6UDhsZ3ZZSmhUcTJ1Z1FaWUtJSFU1bkRw?=
 =?utf-8?B?OWlBL2JJQXdXaXRyTmJsOXNncjdIanNxb211dXkvaEZ2S3g2bmJwWXl0eGJn?=
 =?utf-8?B?WVhkcUxaa3J1NmxPTm9VL09yRUdmVkQzYUcxK3c5NU0zY0ZCRE1JQzNyZi8r?=
 =?utf-8?B?M01EbHEzMUFrV2ZXQ29iakt2NnYwUHZWOElJNmRzZGRQK0ErVFljd1RzTng1?=
 =?utf-8?B?cEVmL2NBU0dVbitBeEdHd0lBV3ZhcXZjTUpvSFJ4aE1lQjVFL2gyZ0ZYandn?=
 =?utf-8?B?d3l4QWo3U0tERzc5bVNXUTJnVjNnUGlaK252NDFlaWZiQ3BEUW4wQ1RLUXR0?=
 =?utf-8?B?UTViTDVnblFOaGR2YUNPalQ0M3p2NkFRQlFXWm93c1F4MEZ1K3Bpa1lrM1Br?=
 =?utf-8?B?NElOZ3MxZ0E1Sk5uNmo5MU8zbzNzVURjMy9TZXFwcHlFTUdYVGNaRHlMSzcv?=
 =?utf-8?B?M0tsenNNTG1jMGUxKzN1L1ViMUx3UVlFcUpzM3lWUGM0TFh4ekZiRGhYNlFI?=
 =?utf-8?B?NnRWcmVuQjZoRTJtVHF5UjZERmFxQ1IvTG52amtCRVhtUkJmc3J0QTlWTS82?=
 =?utf-8?B?TGt6ZWRtSjVsSkFvb0g4SVdDd1dpeG5xdHVRMHlydTF4bURxVXBjaW9CYXZV?=
 =?utf-8?B?bUNBSUpXMmp5bXRKcHQyLy9zcVJEZWdJOHVPeFhxUXRTbnZiMSsxZFk5dTIw?=
 =?utf-8?B?NnFnc1JzT3U4RU9ObnI1Nk9PNmFDU0M5cU5CUFRaOGRDMTFUZUI4MkswNktp?=
 =?utf-8?B?ZmlQU0xXTjhDb2lvbmJTenE2TW9SNlU4RHY1U1VaRzRQTmlSeW1kYlhRSkRI?=
 =?utf-8?B?RzkxWEdhSlB2NXpxVkt6aUs0c1dra2NUUkJPWFQ1bnlqMXB0aXhZd0YyY090?=
 =?utf-8?B?RGdkMENEeE9FTC81Y2hobkVMejZOY2lWaVlFRmdwOUsvM3NjalNxZFdpbk9Z?=
 =?utf-8?B?NE9DQitmOHl0TjR1V0Zhd3Z0ejJ5Y1ZJQ1UrNGNaN0RVVkh4bkRVZ0pGd3Fw?=
 =?utf-8?B?QnY1WEhJQzJIaHRjS2JaQUoyZHVkZnJIVEdnRk9LSXJyWko0U21qYWFCVkRz?=
 =?utf-8?B?SzV2aHNrZGpXSWJSNkt5enZpWk40Ylc5QjJIeTh6QmRkcVhMSDFvL0tiRGJI?=
 =?utf-8?B?RXpSbHoyTnI0TTcra1NrMDRrWmc0WERaem5Id3JjNm55RHNVZ0NHZCtreXh2?=
 =?utf-8?B?UFlZbXlYMElzT2llUDlHckt4QjFCWnZwQnpHOGNRY0lOSStCQzMyeDl5VjVJ?=
 =?utf-8?B?SGlPSDRrMFo5Lzhmc0FXNmx4ZXIydmdocGdtWGw2cGxiOVE5QTluN1orV3E1?=
 =?utf-8?B?RXRJYU4xV1ZJOW1jL3hBZHZTZ2Z6SjFmS0hINXk1a0U3ZGw0WjFRWFlJT1Zp?=
 =?utf-8?B?NUttNklmTVJnR1huU2tKbUV1RUxVSXJxSFJQSEtlTENsazUyamc0WFA0aXFq?=
 =?utf-8?B?aGZEU2NzVkhLT0ZENlU1U2hFMDRRaGw0a2VwMkVXUk1BdWVFQkI1WTRjM3lz?=
 =?utf-8?B?bFZmM25waXdTM0lsT1h0VjdTckZweVczTC9rNFIvN3pKcHErMjg0R1dlYlNO?=
 =?utf-8?B?bUd2SktmeFZsaEpuWnhhdzEzVitIL1orWUk4SFpoanFTekgwOVpCc2ljcHhz?=
 =?utf-8?Q?IgIzQZoTT3a3T5rAThptHGwtt?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5d4064f6-d518-47fa-3c22-08dc7a0295d0
X-MS-Exchange-CrossTenant-AuthSource: AM7PR04MB7046.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2024 01:57:50.2952
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lG+461iEr3i3M+0mRHKJiFecm0nTljMQ9/mOfKG9l8BggyyUhPUDJG1hp7ysQzE/gBlKU+1YtUhtAI+52gZrDQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS1PR04MB9584

On 5/22/24 05:51, Adam Ford wrote:
> On Mon, May 20, 2024 at 8:16 PM Adam Ford <aford173@gmail.com> 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@nxp.com>

s/Reported by/Reported-by/

>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
>> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> + Liu
> 
> Sorry about the e-mail address copy-paste error.

No worries.

With this patch, it looks EDID retrieval works ok for me without
interrupt requested.

Tested-by: Liu Ying <victor.liu@nxp.com> # i.MX8MP EVK ADV7535 EDID retrieval w/o IRQ

>>
>> 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..4efb2cabf1b5 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 ret;
>>
>>         /*
>>          * 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..56dd2d5a0376 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;
>> +       int irq_status = IRQ_NONE;
>>
>>         ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
>>         if (ret < 0)
>> @@ -478,38 +480,41 @@ 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);
>> +
>> +       if (cec_status < 0)
>> +               return cec_status;
>>  #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)
>>  {
>>         struct adv7511 *adv7511 = devid;
>> -       int ret;
>>
>> -       ret = adv7511_irq_process(adv7511, true);
>> -       return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
>> +       return adv7511_irq_process(adv7511, true);
>>  }
>>
>>  /* -----------------------------------------------------------------------------
>> --
>> 2.43.0
>>