Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755005AbcK2Gqi (ORCPT ); Tue, 29 Nov 2016 01:46:38 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:59744 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752429AbcK2Gqb (ORCPT ); Tue, 29 Nov 2016 01:46:31 -0500 From: Laurent Pinchart To: John Stultz Cc: lkml , David Airlie , Archit Taneja , Wolfram Sang , Lars-Peter Clausen , dri-devel@lists.freedesktop.org Subject: Re: [RFC][PATCH 1/5 v2] drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context Date: Tue, 29 Nov 2016 08:46:44 +0200 Message-ID: <3570409.tKEQBRfa3m@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <1480395884-5471-2-git-send-email-john.stultz@linaro.org> References: <1480395884-5471-1-git-send-email-john.stultz@linaro.org> <1480395884-5471-2-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3152 Lines: 100 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 > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) > { > unsigned int irq0, irq1; > @@ -419,7 +427,7 @@ static int adv7511_irq_process(struct adv7511 *adv7511, > bool process_hpd) regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1); > > if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) > - drm_helper_hpd_irq_event(adv7511->connector.dev); > + schedule_work(&adv7511->irq_work); > > if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) { > adv7511->edid_read = true; > @@ -1006,6 +1014,8 @@ static int adv7511_probe(struct i2c_client *i2c, const > struct i2c_device_id *id) goto err_i2c_unregister_edid; > } > > + INIT_WORK(&adv7511->irq_work, adv7511_irq_work); > + > if (i2c->irq) { > init_waitqueue_head(&adv7511->wq); -- Regards, Laurent Pinchart