From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751388AbdAPPri (ORCPT ); Mon, 16 Jan 2017 10:47:38 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:51101 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751210AbdAPPrf (ORCPT ); Mon, 16 Jan 2017 10:47:35 -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: [PATCH 2/5 v3] drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event() Date: Mon, 16 Jan 2017 17:47:46 +0200 Message-ID: <2543020.qSshgutoB3@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <1483472502-16403-3-git-send-email-john.stultz@linaro.org> References: <1483472502-16403-1-git-send-email-john.stultz@linaro.org> <1483472502-16403-3-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 Hi John, Thank you for the patch. On Tuesday 03 Jan 2017 11:41:39 John Stultz wrote: > In chasing down a previous issue with EDID probing from calling > drm_helper_hpd_irq_event() from irq context, Laurent noticed > that the DRM documentation suggests that > drm_kms_helper_hotplug_event() should be used instead. > > Thus this patch replaces drm_helper_hpd_irq_event() with > drm_kms_helper_hotplug_event(), which requires we update the > connector.status entry and only call _hotplug_event() when the > status changes. > > 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 > --- > v3: Update connector.status value and only call __hotplug_event() > when that status changes, as suggested by Laurent. > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 4fcea44..d93d66f > 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -405,8 +405,22 @@ static bool adv7511_hpd(struct adv7511 *adv7511) > static void adv7511_hpd_work(struct work_struct *work) > { > struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work); > + enum drm_connector_status status; > + unsigned int val; > + int ret; > + > + ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val); > + if (ret < 0) > + status = connector_status_disconnected; > + else if (val & ADV7511_STATUS_HPD) > + status = connector_status_connected; > + else > + status = connector_status_disconnected; > + > + if (adv7511->connector.status != status) > + drm_kms_helper_hotplug_event(adv7511->connector.dev); > > - drm_helper_hpd_irq_event(adv7511->connector.dev); > + adv7511->connector.status = status; Shouldn't you update the status before calling drm_kms_helper_hotplug_event() ? Doing it after not only creates a small race condition as drm_kms_helper_hotplug_event() sends an event to userspace that could result in an ioctl call to retrieve the status, but the status is also checked by drm_setup_crtcs() called by drm_fb_helper_hotplug_event(). > } > > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) -- Regards, Laurent Pinchart