| Message ID | 1483472502-16403-3-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <airlied@linux.ie> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > 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)
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 <airlied@linux.ie> > > Cc: Archit Taneja <architt@codeaurora.org> > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Cc: Lars-Peter Clausen <lars@metafoo.de> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > 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 <laurent.pinchart@ideasonboard.com> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > } > > > > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
On Mon, Jan 16, 2017 at 7:56 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > 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 <airlied@linux.ie> >> > Cc: Archit Taneja <architt@codeaurora.org> >> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> >> > Cc: Lars-Peter Clausen <lars@metafoo.de> >> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> > Cc: dri-devel@lists.freedesktop.org >> > Signed-off-by: John Stultz <john.stultz@linaro.org> >> > --- >> > 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 <laurent.pinchart@ideasonboard.com> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks so much for catching this! I'll respin the patches with this fix and resend a v4. thanks again! -john
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; } static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
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 <airlied@linux.ie> Cc: Archit Taneja <architt@codeaurora.org> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz <john.stultz@linaro.org> --- 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(-)