From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751229AbdAPP4Q (ORCPT ); Mon, 16 Jan 2017 10:56:16 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:51136 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750886AbdAPP4O (ORCPT ); Mon, 16 Jan 2017 10:56:14 -0500 From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: John Stultz , lkml , Wolfram Sang Subject: Re: [PATCH 2/5 v3] drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event() Date: Mon, 16 Jan 2017 17:56:29 +0200 Message-ID: <7359021.xs7IcXVQ9z@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <2543020.qSshgutoB3@avalon> References: <1483472502-16403-1-git-send-email-john.stultz@linaro.org> <1483472502-16403-3-git-send-email-john.stultz@linaro.org> <2543020.qSshgutoB3@avalon> 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 On Monday 16 Jan 2017 17:47:46 Laurent Pinchart wrote: > 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(). With if (adv7511->connector.status != status) { adv7511->connector.status = status; drm_kms_helper_hotplug_event(adv7511->connector.dev); } Reviewed-by: Laurent Pinchart Tested-by: Laurent Pinchart > > } > > > > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd) -- Regards, Laurent Pinchart