Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754827AbcK2REa (ORCPT ); Tue, 29 Nov 2016 12:04:30 -0500 Received: from mail-oi0-f48.google.com ([209.85.218.48]:32897 "EHLO mail-oi0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751227AbcK2REX (ORCPT ); Tue, 29 Nov 2016 12:04:23 -0500 MIME-Version: 1.0 In-Reply-To: <3570409.tKEQBRfa3m@avalon> References: <1480395884-5471-1-git-send-email-john.stultz@linaro.org> <1480395884-5471-2-git-send-email-john.stultz@linaro.org> <3570409.tKEQBRfa3m@avalon> From: John Stultz Date: Tue, 29 Nov 2016 09:04:00 -0800 Message-ID: Subject: Re: [RFC][PATCH 1/5 v2] drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context To: Laurent Pinchart Cc: lkml , David Airlie , Archit Taneja , Wolfram Sang , Lars-Peter Clausen , "dri-devel@lists.freedesktop.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2577 Lines: 79 On Mon, Nov 28, 2016 at 10:46 PM, Laurent Pinchart wrote: > Hi John, > > Thank you for the patch. > > On Monday 28 Nov 2016 21:04:40 John Stultz wrote: >> I was recently seeing issues with EDID probing, where >> the logic to wait for the EDID read bit to be set by the >> IRQ wasn't happening and the code would time out and fail. >> >> Digging deeper, I found this was due to the fact that >> IRQs were disabled as we were running in IRQ context from >> the HPD signal. >> >> Thus this patch changes the logic to handle the HPD signal >> via a work_struct so we can be out of irq context. >> >> With this patch, the EDID probing on hotplug does not time >> out. >> >> Cc: David Airlie >> Cc: Archit Taneja >> Cc: Wolfram Sang >> Cc: Lars-Peter Clausen >> Cc: Laurent Pinchart >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: John Stultz >> --- >> v2: Reworked to properly fix the issue rather then >> just delaying for 200ms >> >> drivers/gpu/drm/bridge/adv7511/adv7511.h | 2 ++ >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++++- >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> @@ -317,6 +317,8 @@ struct adv7511 { >> bool edid_read; >> >> wait_queue_head_t wq; >> + struct work_struct irq_work; >> + > > I'd call this hpd_work. > >> struct drm_bridge bridge; >> struct drm_connector connector; >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743 >> 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -402,6 +402,14 @@ static bool adv7511_hpd(struct adv7511 *adv7511) >> return false; >> } >> >> +static void adv7511_irq_work(struct work_struct *work) >> +{ >> + struct adv7511 *adv7511 = container_of(work, struct adv7511, > irq_work); >> + >> + drm_helper_hpd_irq_event(adv7511->connector.dev); >> +} >> + >> + > > One blank line is enough. > > Apart from that, > > Reviewed-by: Laurent Pinchart Thanks so much for the review! I've made your suggested changes! -john