Re: [PATCH 18/21] drm/omap: Don't call EDID read operation recursively

Hi,

On Wed, Jun 06, 2018 at 12:36:47PM +0300, Laurent Pinchart wrote:

Instead of calling the EDID read operation (.read_edid()) recursively from the display device back to the first device that provides EDID read support, iterate over the devices manually in the DRM connector code. This moves the complexity to a single central location and simplifies the logic in omap_dss_device drivers.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Reviewed-by: Sebastian Reichel sebastian.reichel@collabora.co.uk

-- Sebastian

drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 15 +-- drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 11 --- .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c | 13 --- drivers/gpu/drm/omapdrm/dss/hdmi4.c | 1 + drivers/gpu/drm/omapdrm/dss/hdmi5.c | 1 + drivers/gpu/drm/omapdrm/omap_connector.c | 101 ++++++++++++--------- 6 files changed, 65 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c index 6be260ff6458..eae4108330f1 100644 --- a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c +++ b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c @@ -166,12 +166,6 @@ static int dvic_read_edid(struct omap_dss_device *dssdev, struct panel_drv_data *ddata = to_panel_data(dssdev); int r, l, bytes_read;

• if (ddata->hpd_gpio && !gpiod_get_value_cansleep(ddata->hpd_gpio))

• return -ENODEV;
  

• if (!ddata->i2c_adapter)

• return -ENODEV;
  

• l = min(EDID_LENGTH, len); r = dvic_ddc_read(ddata->i2c_adapter, edid, l, 0); if (r)

@@ -341,10 +335,11 @@ static int dvic_probe(struct platform_device *pdev) dssdev->of_ports = BIT(0);

if (ddata->hpd_gpio)

• dssdev->ops_flags = OMAP_DSS_DEVICE_OP_DETECT
  

• 		  | OMAP_DSS_DEVICE_OP_HPD;
  

• else if (ddata->i2c_adapter)

• dssdev->ops_flags = OMAP_DSS_DEVICE_OP_DETECT;
  

• dssdev->ops_flags |= OMAP_DSS_DEVICE_OP_DETECT
  

• 		  |  OMAP_DSS_DEVICE_OP_HPD;
  

• if (ddata->i2c_adapter)

• dssdev->ops_flags |= OMAP_DSS_DEVICE_OP_DETECT
  

• 		  |  OMAP_DSS_DEVICE_OP_EDID;
  
  

  omapdss_display_init(dssdev); omapdss_device_register(dssdev);

diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c index 6f2364afb14a..16dc22edcb8e 100644 --- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c +++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c @@ -15,8 +15,6 @@ #include <linux/platform_device.h> #include <linux/slab.h>

-#include <drm/drm_edid.h>

#include "../dss/omapdss.h"

static const struct videomode hdmic_default_vm = { @@ -126,14 +124,6 @@ static int hdmic_check_timings(struct omap_dss_device *dssdev, return src->ops->check_timings(src, vm); }

-static int hdmic_read_edid(struct omap_dss_device *dssdev,

• u8 *edid, int len)
  

-{

• struct omap_dss_device *src = dssdev->src;
• return src->ops->read_edid(src, edid, len);

-}

static bool hdmic_detect(struct omap_dss_device *dssdev) { struct panel_drv_data *ddata = to_panel_data(dssdev); @@ -190,7 +180,6 @@ static const struct omap_dss_device_ops hdmic_ops = { .get_timings = hdmic_get_timings, .check_timings = hdmic_check_timings,

• .read_edid = hdmic_read_edid, .detect = hdmic_detect, .register_hpd_cb = hdmic_register_hpd_cb, .unregister_hpd_cb = hdmic_unregister_hpd_cb,

diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c index 2772af84531a..dbbf9683bd51 100644 --- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c +++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c @@ -115,18 +115,6 @@ static int tpd_check_timings(struct omap_dss_device *dssdev, return src->ops->check_timings(src, vm); }

-static int tpd_read_edid(struct omap_dss_device *dssdev,

• u8 *edid, int len)
  

-{

• struct panel_drv_data *ddata = to_panel_data(dssdev);
• struct omap_dss_device *src = dssdev->src;
• if (!gpiod_get_value_cansleep(ddata->hpd_gpio))

• return -ENODEV;
  

• return src->ops->read_edid(src, edid, len);

-}

static bool tpd_detect(struct omap_dss_device *dssdev) { struct panel_drv_data *ddata = to_panel_data(dssdev); @@ -180,7 +168,6 @@ static const struct omap_dss_device_ops tpd_ops = { .disable = tpd_disable, .check_timings = tpd_check_timings, .set_timings = tpd_set_timings,

• .read_edid = tpd_read_edid, .detect = tpd_detect, .register_hpd_cb = tpd_register_hpd_cb, .unregister_hpd_cb = tpd_unregister_hpd_cb,

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c index bebce93fed3e..c92564300446 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c @@ -711,6 +711,7 @@ static int hdmi4_init_output(struct omap_hdmi *hdmi) out->ops = &hdmi_ops; out->owner = THIS_MODULE; out->of_ports = BIT(0);

• out->ops_flags = OMAP_DSS_DEVICE_OP_EDID;

  out->next = omapdss_of_find_connected_device(out->dev->of_node, 0); if (IS_ERR(out->next)) {

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c index 7c07e0208107..2aaa8ee61662 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c @@ -703,6 +703,7 @@ static int hdmi5_init_output(struct omap_hdmi *hdmi) out->ops = &hdmi_ops; out->owner = THIS_MODULE; out->of_ports = BIT(0);

• out->ops_flags = OMAP_DSS_DEVICE_OP_EDID;

  out->next = omapdss_of_find_connected_device(out->dev->of_node, 0); if (IS_ERR(out->next)) {

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index 1b3f85a02c85..a1d379816732 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -170,65 +170,80 @@ static void omap_connector_destroy(struct drm_connector *connector)

#define MAX_EDID 512

+static int omap_connector_get_modes_edid(struct drm_connector *connector,

• 			 struct omap_dss_device *dssdev)
  

+{

• struct omap_connector *omap_connector = to_omap_connector(connector);
• enum drm_connector_status status;
• void *edid;
• int n;
• status = omap_connector_detect(connector, false);
• if (status != connector_status_connected)

• goto no_edid;
  

• edid = kzalloc(MAX_EDID, GFP_KERNEL);
• if (!edid)

• goto no_edid;
  

• if (dssdev->ops->read_edid(dssdev, edid, MAX_EDID) <= 0 ||

•    !drm_edid_is_valid(edid)) {
  

• kfree(edid);
  

• goto no_edid;
  

• }
• drm_mode_connector_update_edid_property(connector, edid);
• n = drm_add_edid_modes(connector, edid);
• omap_connector->hdmi_mode = drm_detect_hdmi_monitor(edid);
• kfree(edid);
• return n;

+no_edid:

• drm_mode_connector_update_edid_property(connector, NULL);
• return 0;

+}

static int omap_connector_get_modes(struct drm_connector *connector) { struct omap_connector *omap_connector = to_omap_connector(connector);

• struct omap_dss_device *dssdev = omap_connector->dssdev;
• struct drm_device *dev = connector->dev;
• int n = 0;

• struct omap_dss_device *dssdev;

• struct drm_display_mode *mode;

• struct videomode vm = {0};

  DBG("%s", omap_connector->dssdev->name);

• /* if display exposes EDID, then we parse that in the normal way to

• * build table of supported modes.. otherwise (ie. fixed resolution
  

• /*

• * If display exposes EDID, then we parse that in the normal way to
  

• * build table of supported modes. Otherwise (ie. fixed resolution
  

  • LCD panels) we just return a single mode corresponding to the

• * currently configured timings:
  

• * currently configured timings.
  

  */

• if (dssdev->ops->read_edid) {

• void *edid = kzalloc(MAX_EDID, GFP_KERNEL);
  

• if (!edid)
  

• 	return 0;
  

• if ((dssdev->ops->read_edid(dssdev, edid, MAX_EDID) > 0) &&
  

• 		drm_edid_is_valid(edid)) {
  

• 	drm_mode_connector_update_edid_property(
  

• 			connector, edid);
  

• 	n = drm_add_edid_modes(connector, edid);
  

• 	omap_connector->hdmi_mode =
  

• 		drm_detect_hdmi_monitor(edid);
  

• } else {
  

• 	drm_mode_connector_update_edid_property(
  

• 			connector, NULL);
  

• }
  

• kfree(edid);
  

• } else {

• struct drm_display_mode *mode = drm_mode_create(dev);
  

• struct videomode vm = {0};
  

• dssdev = omap_connector_find_device(connector,

• 			    OMAP_DSS_DEVICE_OP_EDID);
  

• if (dssdev)

• return omap_connector_get_modes_edid(connector, dssdev);
  
  

• if (!mode)
  

• 	return 0;
  

• mode = drm_mode_create(connector->dev);
• if (!mode)

• return 0;
  
  

• dssdev->ops->get_timings(dssdev, &vm);
  

• dssdev = omap_connector->dssdev;
• dssdev->ops->get_timings(dssdev, &vm);

• drm_display_mode_from_videomode(&vm, mode);
  

• drm_display_mode_from_videomode(&vm, mode);

• mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
  

• drm_mode_set_name(mode);
  

• drm_mode_probed_add(connector, mode);
  

• mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
• drm_mode_set_name(mode);
• drm_mode_probed_add(connector, mode);

• if (dssdev->driver && dssdev->driver->get_size) {
  

• 	dssdev->driver->get_size(dssdev,
  

• if (dssdev->driver && dssdev->driver->get_size)

• dssdev->driver->get_size(dssdev,
  		 &connector->display_info.width_mm,
  		 &connector->display_info.height_mm);
  

• }
  
  

• n = 1;
  

• }

• return n;

• return 1;

}

static int omap_connector_mode_valid(struct drm_connector *connector,

Regards,

Laurent Pinchart

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel