diff mbox

[v3] drm/exynos: let drm handle edid allocations

Message ID 1357128090-3781-1-git-send-email-rahul.sharma@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rahul Sharma Jan. 2, 2013, 12:01 p.m. UTC
There's no need to allocate edid twice and do a memcpy when drm helpers
exist to do just that. This patch cleans that interaction up, and
doesn't keep the edid hanging around in the connector.

v3:
- removed MAX_EDID as it is not used anymore.

v2:
- changed vidi_get_edid callback inside vidi driver.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
This patch is based on branch "exynos-drm-next" at
http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git

 drivers/gpu/drm/exynos/exynos_drm_connector.c | 37 ++++++++++++++-------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h       |  4 +--
 drivers/gpu/drm/exynos/exynos_drm_hdmi.c      |  9 +++----
 drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |  4 +--
 drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 19 +++++++++-----
 drivers/gpu/drm/exynos/exynos_hdmi.c          | 25 ++++++++----------
 6 files changed, 50 insertions(+), 48 deletions(-)

Comments

Inki Dae Jan. 4, 2013, 7:31 a.m. UTC | #1
Hi Rahul,

With this patch, I got a error like below,

# echo 1 > /sys/devices/vidi.5/connection
[  149.485000] [drm:exynos_drm_connector_get_modes] *ERROR* update
edid property failed(-22)

There is something wrong. Please check it out again.

2013/1/2 Rahul Sharma <rahul.sharma@samsung.com>:
> There's no need to allocate edid twice and do a memcpy when drm helpers
> exist to do just that. This patch cleans that interaction up, and
> doesn't keep the edid hanging around in the connector.
>
> v3:
> - removed MAX_EDID as it is not used anymore.
>
> v2:
> - changed vidi_get_edid callback inside vidi driver.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
> This patch is based on branch "exynos-drm-next" at
> http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git
>
>  drivers/gpu/drm/exynos/exynos_drm_connector.c | 37 ++++++++++++++-------------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  4 +--
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      |  9 +++----
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |  4 +--
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 19 +++++++++-----
>  drivers/gpu/drm/exynos/exynos_hdmi.c          | 25 ++++++++----------
>  6 files changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> index ab37437..221ec72 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> @@ -18,7 +18,6 @@
>  #include "exynos_drm_drv.h"
>  #include "exynos_drm_encoder.h"
>
> -#define MAX_EDID 256
>  #define to_exynos_connector(x) container_of(x, struct exynos_drm_connector,\
>                                 drm_connector)
>
> @@ -96,7 +95,9 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector)
>                                         to_exynos_connector(connector);
>         struct exynos_drm_manager *manager = exynos_connector->manager;
>         struct exynos_drm_display_ops *display_ops = manager->display_ops;
> -       unsigned int count;
> +       unsigned int count = 0;
> +       struct edid *edid = NULL;
> +       int ret;
>
>         DRM_DEBUG_KMS("%s\n", __FILE__);
>
> @@ -114,27 +115,25 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector)
>          * because lcd panel has only one mode.
>          */
>         if (display_ops->get_edid) {
> -               int ret;
> -               void *edid;
> -
> -               edid = kzalloc(MAX_EDID, GFP_KERNEL);
> -               if (!edid) {
> -                       DRM_ERROR("failed to allocate edid\n");
> -                       return 0;
> +               edid = display_ops->get_edid(manager->dev, connector);
> +               if (IS_ERR_OR_NULL(edid)) {
> +                       ret = PTR_ERR(edid);
> +                       edid = NULL;
> +                       DRM_ERROR("Panel operation get_edid failed %d\n", ret);
> +                       goto out;
>                 }
>
> -               ret = display_ops->get_edid(manager->dev, connector,
> -                                               edid, MAX_EDID);
> -               if (ret < 0) {
> -                       DRM_ERROR("failed to get edid data.\n");
> -                       kfree(edid);
> -                       edid = NULL;
> -                       return 0;
> +               ret = drm_mode_connector_update_edid_property(connector, edid);
> +               if (ret) {
> +                       DRM_ERROR("update edid property failed(%d)\n", ret);
> +                       goto out;
>                 }
>
> -               drm_mode_connector_update_edid_property(connector, edid);
>                 count = drm_add_edid_modes(connector, edid);
> -               kfree(edid);
> +               if (count < 0) {
> +                       DRM_ERROR("Add edid modes failed %d\n", count);
> +                       goto out;
> +               }
>         } else {
>                 struct exynos_drm_panel_info *panel;
>                 struct drm_display_mode *mode = drm_mode_create(connector->dev);
> @@ -161,6 +160,8 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector)
>                 count = 1;
>         }
>
> +out:
> +       kfree(edid);
>         return count;
>  }
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index b9e51bc..4606fac 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -148,8 +148,8 @@ struct exynos_drm_overlay {
>  struct exynos_drm_display_ops {
>         enum exynos_drm_output_type type;
>         bool (*is_connected)(struct device *dev);
> -       int (*get_edid)(struct device *dev, struct drm_connector *connector,
> -                               u8 *edid, int len);
> +       struct edid *(*get_edid)(struct device *dev,
> +                       struct drm_connector *connector);
>         void *(*get_panel)(struct device *dev);
>         int (*check_timing)(struct device *dev, void *timing);
>         int (*power_on)(struct device *dev, int mode);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> index 55793c4..a17d752 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> @@ -108,18 +108,17 @@ static bool drm_hdmi_is_connected(struct device *dev)
>         return false;
>  }
>
> -static int drm_hdmi_get_edid(struct device *dev,
> -               struct drm_connector *connector, u8 *edid, int len)
> +struct edid *drm_hdmi_get_edid(struct device *dev,
> +                       struct drm_connector *connector)
>  {
>         struct drm_hdmi_context *ctx = to_context(dev);
>
>         DRM_DEBUG_KMS("%s\n", __FILE__);
>
>         if (hdmi_ops && hdmi_ops->get_edid)
> -               return hdmi_ops->get_edid(ctx->hdmi_ctx->ctx, connector, edid,
> -                                         len);
> +               return hdmi_ops->get_edid(ctx->hdmi_ctx->ctx, connector);
>
> -       return 0;
> +       return NULL;
>  }
>
>  static int drm_hdmi_check_timing(struct device *dev, void *timing)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> index 784a7e9..d80516f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> @@ -30,8 +30,8 @@ struct exynos_drm_hdmi_context {
>  struct exynos_hdmi_ops {
>         /* display */
>         bool (*is_connected)(void *ctx);
> -       int (*get_edid)(void *ctx, struct drm_connector *connector,
> -                       u8 *edid, int len);
> +       struct edid *(*get_edid)(void *ctx,
> +                       struct drm_connector *connector);
>         int (*check_timing)(void *ctx, void *timing);
>         int (*power_on)(void *ctx, int mode);
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 99bfc38..3260035 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -98,10 +98,12 @@ static bool vidi_display_is_connected(struct device *dev)
>         return ctx->connected ? true : false;
>  }
>
> -static int vidi_get_edid(struct device *dev, struct drm_connector *connector,
> -                               u8 *edid, int len)
> +static struct edid *vidi_get_edid(struct device *dev,
> +                       struct drm_connector *connector)
>  {
>         struct vidi_context *ctx = get_vidi_context(dev);
> +       struct edid *edid;
> +       int edid_len;
>
>         DRM_DEBUG_KMS("%s\n", __FILE__);
>
> @@ -111,13 +113,18 @@ static int vidi_get_edid(struct device *dev, struct drm_connector *connector,
>          */
>         if (!ctx->raw_edid) {
>                 DRM_DEBUG_KMS("raw_edid is null.\n");
> -               return -EFAULT;
> +               return ERR_PTR(-EFAULT);
>         }
>
> -       memcpy(edid, ctx->raw_edid, min((1 + ctx->raw_edid->extensions)
> -                                       * EDID_LENGTH, len));
> +       edid_len = (1 + ctx->raw_edid->extensions) * EDID_LENGTH;
> +       edid = kzalloc(edid_len, GFP_KERNEL);
> +       if (!edid) {
> +               DRM_DEBUG_KMS("failed to allocate edid\n");
> +               return ERR_PTR(-ENOMEM);
> +       }
>
> -       return 0;
> +       memcpy(edid, ctx->raw_edid, edid_len);
> +       return edid;
>  }
>
>  static void *vidi_get_panel(struct device *dev)
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 2c46b6c..36e9214 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -1391,8 +1391,7 @@ static bool hdmi_is_connected(void *ctx)
>         return hdata->hpd;
>  }
>
> -static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
> -                               u8 *edid, int len)
> +static struct edid *hdmi_get_edid(void *ctx, struct drm_connector *connector)
>  {
>         struct edid *raw_edid;
>         struct hdmi_context *hdata = ctx;
> @@ -1400,22 +1399,18 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
>         DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>
>         if (!hdata->ddc_port)
> -               return -ENODEV;
> +               return ERR_PTR(-ENODEV);
>
>         raw_edid = drm_get_edid(connector, hdata->ddc_port->adapter);
> -       if (raw_edid) {
> -               hdata->dvi_mode = !drm_detect_hdmi_monitor(raw_edid);
> -               memcpy(edid, raw_edid, min((1 + raw_edid->extensions)
> -                                       * EDID_LENGTH, len));
> -               DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
> -                       (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
> -                       raw_edid->width_cm, raw_edid->height_cm);
> -               kfree(raw_edid);
> -       } else {
> -               return -ENODEV;
> -       }
> +       if (!raw_edid)
> +               return ERR_PTR(-ENODEV);
>
> -       return 0;
> +       hdata->dvi_mode = !drm_detect_hdmi_monitor(raw_edid);
> +       DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
> +               (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
> +               raw_edid->width_cm, raw_edid->height_cm);
> +
> +       return raw_edid;
>  }
>
>  static int hdmi_v13_check_timing(struct fb_videomode *check_timing)
> --
> 1.8.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c
index ab37437..221ec72 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
@@ -18,7 +18,6 @@ 
 #include "exynos_drm_drv.h"
 #include "exynos_drm_encoder.h"
 
-#define MAX_EDID 256
 #define to_exynos_connector(x)	container_of(x, struct exynos_drm_connector,\
 				drm_connector)
 
@@ -96,7 +95,9 @@  static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 					to_exynos_connector(connector);
 	struct exynos_drm_manager *manager = exynos_connector->manager;
 	struct exynos_drm_display_ops *display_ops = manager->display_ops;
-	unsigned int count;
+	unsigned int count = 0;
+	struct edid *edid = NULL;
+	int ret;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
@@ -114,27 +115,25 @@  static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 	 * because lcd panel has only one mode.
 	 */
 	if (display_ops->get_edid) {
-		int ret;
-		void *edid;
-
-		edid = kzalloc(MAX_EDID, GFP_KERNEL);
-		if (!edid) {
-			DRM_ERROR("failed to allocate edid\n");
-			return 0;
+		edid = display_ops->get_edid(manager->dev, connector);
+		if (IS_ERR_OR_NULL(edid)) {
+			ret = PTR_ERR(edid);
+			edid = NULL;
+			DRM_ERROR("Panel operation get_edid failed %d\n", ret);
+			goto out;
 		}
 
-		ret = display_ops->get_edid(manager->dev, connector,
-						edid, MAX_EDID);
-		if (ret < 0) {
-			DRM_ERROR("failed to get edid data.\n");
-			kfree(edid);
-			edid = NULL;
-			return 0;
+		ret = drm_mode_connector_update_edid_property(connector, edid);
+		if (ret) {
+			DRM_ERROR("update edid property failed(%d)\n", ret);
+			goto out;
 		}
 
-		drm_mode_connector_update_edid_property(connector, edid);
 		count = drm_add_edid_modes(connector, edid);
-		kfree(edid);
+		if (count < 0) {
+			DRM_ERROR("Add edid modes failed %d\n", count);
+			goto out;
+		}
 	} else {
 		struct exynos_drm_panel_info *panel;
 		struct drm_display_mode *mode = drm_mode_create(connector->dev);
@@ -161,6 +160,8 @@  static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 		count = 1;
 	}
 
+out:
+	kfree(edid);
 	return count;
 }
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index b9e51bc..4606fac 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -148,8 +148,8 @@  struct exynos_drm_overlay {
 struct exynos_drm_display_ops {
 	enum exynos_drm_output_type type;
 	bool (*is_connected)(struct device *dev);
-	int (*get_edid)(struct device *dev, struct drm_connector *connector,
-				u8 *edid, int len);
+	struct edid *(*get_edid)(struct device *dev,
+			struct drm_connector *connector);
 	void *(*get_panel)(struct device *dev);
 	int (*check_timing)(struct device *dev, void *timing);
 	int (*power_on)(struct device *dev, int mode);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index 55793c4..a17d752 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
@@ -108,18 +108,17 @@  static bool drm_hdmi_is_connected(struct device *dev)
 	return false;
 }
 
-static int drm_hdmi_get_edid(struct device *dev,
-		struct drm_connector *connector, u8 *edid, int len)
+struct edid *drm_hdmi_get_edid(struct device *dev,
+			struct drm_connector *connector)
 {
 	struct drm_hdmi_context *ctx = to_context(dev);
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
 	if (hdmi_ops && hdmi_ops->get_edid)
-		return hdmi_ops->get_edid(ctx->hdmi_ctx->ctx, connector, edid,
-					  len);
+		return hdmi_ops->get_edid(ctx->hdmi_ctx->ctx, connector);
 
-	return 0;
+	return NULL;
 }
 
 static int drm_hdmi_check_timing(struct device *dev, void *timing)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
index 784a7e9..d80516f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
@@ -30,8 +30,8 @@  struct exynos_drm_hdmi_context {
 struct exynos_hdmi_ops {
 	/* display */
 	bool (*is_connected)(void *ctx);
-	int (*get_edid)(void *ctx, struct drm_connector *connector,
-			u8 *edid, int len);
+	struct edid *(*get_edid)(void *ctx,
+			struct drm_connector *connector);
 	int (*check_timing)(void *ctx, void *timing);
 	int (*power_on)(void *ctx, int mode);
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 99bfc38..3260035 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -98,10 +98,12 @@  static bool vidi_display_is_connected(struct device *dev)
 	return ctx->connected ? true : false;
 }
 
-static int vidi_get_edid(struct device *dev, struct drm_connector *connector,
-				u8 *edid, int len)
+static struct edid *vidi_get_edid(struct device *dev,
+			struct drm_connector *connector)
 {
 	struct vidi_context *ctx = get_vidi_context(dev);
+	struct edid *edid;
+	int edid_len;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
@@ -111,13 +113,18 @@  static int vidi_get_edid(struct device *dev, struct drm_connector *connector,
 	 */
 	if (!ctx->raw_edid) {
 		DRM_DEBUG_KMS("raw_edid is null.\n");
-		return -EFAULT;
+		return ERR_PTR(-EFAULT);
 	}
 
-	memcpy(edid, ctx->raw_edid, min((1 + ctx->raw_edid->extensions)
-					* EDID_LENGTH, len));
+	edid_len = (1 + ctx->raw_edid->extensions) * EDID_LENGTH;
+	edid = kzalloc(edid_len, GFP_KERNEL);
+	if (!edid) {
+		DRM_DEBUG_KMS("failed to allocate edid\n");
+		return ERR_PTR(-ENOMEM);
+	}
 
-	return 0;
+	memcpy(edid, ctx->raw_edid, edid_len);
+	return edid;
 }
 
 static void *vidi_get_panel(struct device *dev)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 2c46b6c..36e9214 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -1391,8 +1391,7 @@  static bool hdmi_is_connected(void *ctx)
 	return hdata->hpd;
 }
 
-static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
-				u8 *edid, int len)
+static struct edid *hdmi_get_edid(void *ctx, struct drm_connector *connector)
 {
 	struct edid *raw_edid;
 	struct hdmi_context *hdata = ctx;
@@ -1400,22 +1399,18 @@  static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
 	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
 
 	if (!hdata->ddc_port)
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 
 	raw_edid = drm_get_edid(connector, hdata->ddc_port->adapter);
-	if (raw_edid) {
-		hdata->dvi_mode = !drm_detect_hdmi_monitor(raw_edid);
-		memcpy(edid, raw_edid, min((1 + raw_edid->extensions)
-					* EDID_LENGTH, len));
-		DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
-			(hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
-			raw_edid->width_cm, raw_edid->height_cm);
-		kfree(raw_edid);
-	} else {
-		return -ENODEV;
-	}
+	if (!raw_edid)
+		return ERR_PTR(-ENODEV);
 
-	return 0;
+	hdata->dvi_mode = !drm_detect_hdmi_monitor(raw_edid);
+	DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
+		(hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
+		raw_edid->width_cm, raw_edid->height_cm);
+
+	return raw_edid;
 }
 
 static int hdmi_v13_check_timing(struct fb_videomode *check_timing)