linux-kernel - Re: [PATCH 1/3] drm/omap: Support for HDMI hot plug detection

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <817d3ce9-f189-f8d3-1764-7701e8412af6@ti.com>
Date:   Mon, 22 May 2017 14:59:09 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...com>
To:     Peter Ujfalusi <peter.ujfalusi@...com>,
        <laurent.pinchart@...asonboard.com>
CC:     <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

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?

> +
>  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?

 Tomi



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)