| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 23 May 2017 12:36 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@...com>
Cc: Peter Ujfalusi <peter.ujfalusi@...com>, airlied@...ux.ie,
jsarha@...com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] drm/omap: Support for HDMI hot plug detection
Hi Tomi,
On Monday 22 May 2017 14:59:09 Tomi Valkeinen wrote:
> On 15/05/17 12:03, Peter Ujfalusi wrote:
> > The HPD signal can be used for detecting HDMI cable plug and unplug event
> > without the need for polling the status of the line.
> > This will speed up detecting such event because we do not need to wait for
> > the next poll event to notice the state change.
> >
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@...com>
> > ---
> >
> > drivers/gpu/drm/omapdrm/dss/omapdss.h | 17 +++++++++++++++++
> > drivers/gpu/drm/omapdrm/omap_connector.c | 32 ++++++++++++++++++++++++++-
> > drivers/gpu/drm/omapdrm/omap_drv.c | 29 ++++++++++++++++++++++++++
> > 3 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > b/drivers/gpu/drm/omapdrm/dss/omapdss.h index b19dae1fd6c5..1f01669eb610
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > @@ -25,6 +25,7 @@
> >
> > #include <video/videomode.h>
> > #include <linux/platform_data/omapdss.h>
> > #include <uapi/drm/drm_mode.h>
> >
> > +#include <drm/drm_crtc.h>
> >
> > #define DISPC_IRQ_FRAMEDONE (1 << 0)
> > #define DISPC_IRQ_VSYNC (1 << 1)
> >
> > @@ -555,6 +556,14 @@ struct omapdss_hdmi_ops {
> >
> > int (*read_edid)(struct omap_dss_device *dssdev, u8 *buf, int len);
> > bool (*detect)(struct omap_dss_device *dssdev);
> >
> > + int (*register_hpd_cb)(struct omap_dss_device *dssdev,
> > + void (*cb)(void *cb_data,
> > + enum drm_connector_status status),
> > + void *cb_data);
> > + void (*unregister_hpd_cb)(struct omap_dss_device *dssdev);
> > + void (*enable_hpd)(struct omap_dss_device *dssdev);
> > + void (*disable_hpd)(struct omap_dss_device *dssdev);
> > +
> >
> > int (*set_hdmi_mode)(struct omap_dss_device *dssdev, bool hdmi_mode);
> > int (*set_infoframe)(struct omap_dss_device *dssdev,
> >
> > const struct hdmi_avi_infoframe *avi);
> >
> > @@ -761,6 +770,14 @@ struct omap_dss_driver {
> >
> > int (*read_edid)(struct omap_dss_device *dssdev, u8 *buf, int len);
> > bool (*detect)(struct omap_dss_device *dssdev);
> >
> > + int (*register_hpd_cb)(struct omap_dss_device *dssdev,
> > + void (*cb)(void *cb_data,
> > + enum drm_connector_status status),
> > + void *cb_data);
> > + void (*unregister_hpd_cb)(struct omap_dss_device *dssdev);
> > + void (*enable_hpd)(struct omap_dss_device *dssdev);
> > + void (*disable_hpd)(struct omap_dss_device *dssdev);
> > +
> >
> > int (*set_hdmi_mode)(struct omap_dss_device *dssdev, bool hdmi_mode);
> > int (*set_hdmi_infoframe)(struct omap_dss_device *dssdev,
> >
> > const struct hdmi_avi_infoframe *avi);
> >
> > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c
> > b/drivers/gpu/drm/omapdrm/omap_connector.c index
> > c24b6b783e9a..5219e340ed9d 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> > @@ -35,6 +35,18 @@ struct omap_connector {
> >
> > bool hdmi_mode;
> >
> > };
> >
> > +static void omap_connector_hpd_cb(void *cb_data,
> > + enum drm_connector_status status)
> > +{
> > + struct omap_connector *omap_connector = cb_data;
> > + struct drm_connector *connector = &omap_connector->base;
> > +
> > + if (connector->status != status) {
> > + connector->status = status;
> > + drm_kms_helper_hotplug_event(omap_connector->base.dev);
> > + }
> > +}
>
> I'm not sure if this is racy or not... drm_kms_helper_hotplug_event()
> should be called without locks held, but is it safe to touch
> connector->status without any locks?
We should at least protect against internal race conditions if the hpd
callback can be called concurrently (I haven't checked the callers yet). I
think it would be safer to use the mode_config mutex the same way
drm_helper_hpd_irq_event() does. Something like the following should do.
struct omap_connector *omap_connector = cb_data;
struct drm_connector *connector = &omap_connector->base;
struct drm_device *dev = connector->dev;
enum drm_connector_status old_status;
mutex_lock(&dev->mode_config.mutex);
old_status = connector->status;
connector->status = status;
mutex_unlock(&dev->mode_config.mutex);
if (old_status != status)
drm_kms_helper_hotplug_event(dev);
> > + if (connector->status != status) {
> > + connector->status = status;
> > + drm_kms_helper_hotplug_event(omap_connector->base.dev);
> > + }
> > +
> > bool omap_connector_get_hdmi_mode(struct drm_connector *connector)
> > {
> > struct omap_connector *omap_connector = to_omap_connector(connector);
> > @@ -75,6 +87,10 @@ static void omap_connector_destroy(struct drm_connector
> > *connector)
> > struct omap_dss_device *dssdev = omap_connector->dssdev;
> >
> > DBG("%s", omap_connector->dssdev->name);
> > + if (connector->polled == DRM_CONNECTOR_POLL_HPD &&
> > + dssdev->driver->unregister_hpd_cb) {
> > + dssdev->driver->unregister_hpd_cb(dssdev);
> > + }
> > drm_connector_unregister(connector);
> > drm_connector_cleanup(connector);
> > kfree(omap_connector);
> > @@ -216,6 +232,7 @@ struct drm_connector *omap_connector_init(struct
> > drm_device *dev,
> > {
> > struct drm_connector *connector = NULL;
> > struct omap_connector *omap_connector;
> > + bool hpd_supported = false;
> >
> > DBG("%s", dssdev->name);
> >
> > @@ -233,7 +250,20 @@ struct drm_connector *omap_connector_init(struct
> > drm_device *dev,
> > connector_type);
> >
> > drm_connector_helper_add(connector, &omap_connector_helper_funcs);
> >
> > - if (dssdev->driver->detect)
> > + if (dssdev->driver->register_hpd_cb) {
> > + int ret = dssdev->driver->register_hpd_cb(dssdev,
> > +
omap_connector_hpd_cb,
> > + omap_connector);
> > + if (!ret)
> > + hpd_supported = true;
> > + else if (ret != -ENOTSUPP)
> > + DBG("%s: Failed to register HPD callback (%d).",
> > + dssdev->name, ret);
>
> This should be an error print, shouldn't it?
Can it actually happen ? If it can't, I wouldn't bother with a message at all.
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists