Received-SPF: pass (google.com: domain of linux-kernel+bounces-183531-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <2f41a890-915b-4859-8ac9-5436da14fe22@nxp.com>
Date: Mon, 20 May 2024 17:26:05 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when
 necessary
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, aford173@gmail.com
Cc: 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, sui.jingfeng@linux.dev
References: <20240516101006.2388767-1-victor.liu@nxp.com>
 <evnxopqt5be56vxuxqdj4l3mcd5nrrvucio7jhwgg4emraai3t@gherubdynyip>
 <5efe29a7-441f-4d70-b17c-5327c0a9f600@nxp.com>
 <CAA8EJpqZPQUcqm2U+T7AK74qxo9J0kggS4g484OfoNPAEZPNBw@mail.gmail.com>
Content-Language: en-US
From: Liu Ying <victor.liu@nxp.com>
In-Reply-To: <CAA8EJpqZPQUcqm2U+T7AK74qxo9J0kggS4g484OfoNPAEZPNBw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
 =?utf-8?B?V0hzTDM2L1RCRktPZjRjQ1E3aUh4amhwcThzaG5Ba1JVcnB5NkZwM01HWmVy?=
 =?utf-8?B?UitxWit0UDFLV3d4QzBZRHI0UWEvS1BwaU1FQms4NGhvUVluZGpqOGR6cVZx?=
 =?utf-8?B?Uko4WjZjanJheWloLytsRFBvVCs5NVhhMzIrdytFNTZJcm1rUDVKT3lKZzdv?=
 =?utf-8?B?dThpejRNTHNtcEpkN0c5SHQ5bjgwdjVvR3l0RTNyZTZud1AzWWJnV3hUbGll?=
 =?utf-8?B?a0ZpWkJnZm43K3pxaTh0MDMrY2RPclcrTmwyRlBvT2FGR280TU1KdXhDZTk5?=
 =?utf-8?B?VkNrNXRHdThmV3d0ZmV3aUNVMXRFd2g3V0Mzcllmam84ZzZab2ZCNlVrbFdo?=
 =?utf-8?B?YXM3SWd5dUhmcGpaL0xweWV3QkttVmpYY3NRYmtxMDkwNllwUmNHNm1Pa21H?=
 =?utf-8?B?YTAyYVBicFl6S05oYWRtallkdkNYM3VVV1FNQmdISXZraU04TUpXT0ZWUHd5?=
 =?utf-8?B?TzZSQTI5SEt4ZXVJU2pMVlRrdmxkUm5oYWxxUC9FTlZkQ1BjRUNGY3dMa3VY?=
 =?utf-8?B?N081dTNGeElUZGhiZTR5ek9nWXFrdit1c1N1ZkV3VWdLS2ZnY2ZPNlhndXZS?=
 =?utf-8?B?WHNpRnoyTGRrMXFRUWg4ek84SlhUYXM0TUs3U2g5SFlNSGpYMjloOUhlTzRw?=
 =?utf-8?B?UHNtN2NnUEtlK3U3U29ZbW9peFBtQmxuOU8vdlZBQkJxMHAwNnBCOVQ3QVgz?=
 =?utf-8?B?UVFIMWUyaktEZG9haG56WUFWbW1IMXJ0TkdMSHJBL1JqQzdLUzdxL3U5Qlg2?=
 =?utf-8?B?Vmh3b3FsenRUUVRYWEFsVHNlRGlub0JrbnB5bTg1V2xObDFZUFFEQzZQWTQ3?=
 =?utf-8?B?ZGt6NEFvQzR1dFBtTmdNbEczNGRZSk1GaWJwQ0VudVp6NHlpSHk3eVFmY3JY?=
 =?utf-8?B?RTFHWGp2ZklGTVVkOTRmWUdKeTdpS3VSU1dzZHl4cU9lV1gwc0I4STZJVmhB?=
 =?utf-8?B?UmlaaXlwVVhuQko1bW1JSy9tbFdIdmtUbjJxLzlJaVMzZVV6WTJFSnJYMyty?=
 =?utf-8?B?T0Z4WHVYdDhHeDhVRzVOSVBaRlpNWVVLMisxMXd3VE9YV3IvdiszRUNtdTF5?=
 =?utf-8?B?WFJabE9WdXB2a0ovRU5ETlVQeFp3N20yMmFaQ2R0S0IzcnRxL2ZkUStoNVFi?=
 =?utf-8?B?eDNWOHBMbEtBM2NxRmlEbXZVMUgweW9Bd2l6d05ySXVBbXlMNXMwaVV0d1ZI?=
 =?utf-8?B?dTNoOVB6ZHZNZnhieGg2eWVmaVdMS2c5K3JycG8vQ3NVY3BlZlQxRERvblU0?=
 =?utf-8?B?KzJOSVhCd29JUUpLcGVydVV4VWwvamNXY2R0NkpWSlpSanIrY3B4SG1LMDFP?=
 =?utf-8?B?NThUbm9XVlNuZXZwMmVkbGVONzhzb0NkMjhHcHo3ZjN0alBKTG5vZHZsaURZ?=
 =?utf-8?B?RnVhUFBlbW5mQ1h1ZlhyNmVNVThyaHVRVjg1K3hxbHA5TUFFOWdNSmR4T2tl?=
 =?utf-8?B?b2M1VENsbURBSFBGbUwvSDNOdzFpQ1VsVjNhR0VxM3U4MVVHalhTZ3Bxcklk?=
 =?utf-8?B?SXZNQ1VkYkhSNEFSejR1Z2pmUU5FSDlPVlRYbDFpVDZXWUwvZ2RxenAxWHlM?=
 =?utf-8?B?MmNyNTRDaFNNQ3BXMmR1TkEybW0zM256K2Z1T3pObEhEaEN1V3dDZWNSd3Bu?=
 =?utf-8?B?OXJ5U2pjU3B5MC82cjA5MndTUlV3Qm1FeCtXUUtRRW92c0VWdEcwYUI4QW5r?=
 =?utf-8?B?aG9DM1BJekljMDZHS002dE1uRFVNQXBjRkdqUm1KMjdlY2N5M2h2WHdya0lj?=
 =?utf-8?B?bFIwSEVUM244eElQU0QyZ2tLVGFwb2NCWE5obENUWkQ2Ry9ERjNKN2RpM0hV?=
 =?utf-8?B?OFNnazRLRVNxcWE1S1R1a05LMzZZSFg2VnBHWHc4dWd1QnFSZjJFR0c4ZHZW?=
 =?utf-8?B?aStmQmk4M09FWElZaFc2T1J4N3ZGc3pMS3RSdno5RlVsZFNMdHNxVHlEbEIz?=
 =?utf-8?B?aEhZdVpuTTMzZGs0aDEwZzNEWWErUi83cDAyclhFa1lHOVhwenNiN0pvOHkz?=
 =?utf-8?B?dXB5TVFwYUxQMFNOb0ptQzlwYWlwdUVFLzducXdYMW9TOTA3cXUvZktJVmRX?=
 =?utf-8?B?WmYvcjBNZ2VZK3RYVCtJWE1oSlo4SWkvQ2phMGdad3hJMWRuUFkvSk5tV3JP?=
 =?utf-8?Q?NjwQhIt1laMvvH0D+e0ULX3Ze?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6d589888-911b-4276-36ef-08dc78aed49f
X-MS-Exchange-CrossTenant-AuthSource: AM7PR04MB7046.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2024 09:25:46.9755
 (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: CbkWxNxCd3b+bE1oNa1XoeCMPiIgiV/A6mnaTc/lLif0cRNMxMXbF47SzjMtJkg+AXtiLqMcDr9KJqx+byQ1iA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB7691

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?