Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:31 EET Tomi Valkeinen wrote:

If we have no crtcs/connectors, fbdev init goes fine, but omap_fbdev_create() is never called. This means that omap_fbdev->bo is NULL and omap_fbdev_free() crashes.

Add a check to omap_fbdev_free() to handle the NULL case.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

I wonder whether we shouldn't fail probe if we have no CRTC or connector, but regardless of that, this change looks good to me.

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 1ace63e2ff22..632ebcf2165f 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -303,7 +303,8 @@ void omap_fbdev_free(struct drm_device *dev) fbdev = to_omap_fbdev(priv->fbdev);

/* unpin the GEM object pinned in omap_fbdev_create() */

• omap_gem_unpin(fbdev->bo);

• if (fbdev->bo)

• omap_gem_unpin(fbdev->bo);
  
  

  /* this will free the backing object */ if (fbdev->fb)

On 27/02/18 13:38, Laurent Pinchart wrote:

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:32 EET Tomi Valkeinen wrote:

...

omap_fbdev_init() and omap_fbdev_free() use priv->fbdev directly. However, omap_fbdev_init() returns the fbdev, and omap_drv.c also assigns the return value to priv->fbdev. This is slightly confusing.

Clean this up by removing the omap_fbdev_init() return value, as we don't care whether fbdev init succeeded or not. Also change omap_drv.c to call omap_fbdev_init() always, and omap_fbdev_init() does the check if fbdev was initialized.

I assume you mean omap_fbdev_free() for the last two occurrences (and on a

Right, true.

side note I wonder whether we should rename it to omap_fbdev_cleanup() or omap_fbdev_fini() for symmetry with omap_fbdev_init()).

Yep, I think that makes sense.

...

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++----- drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++----- drivers/gpu/drm/omapdrm/omap_fbdev.h | 5 ++--- 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b2556f5b..485684c637ff 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev) for (i = 0; i < priv->num_crtcs; i++) drm_crtc_vblank_off(priv->crtcs[i]);

• priv->fbdev = omap_fbdev_init(ddev);

• omap_fbdev_init(ddev);

  drm_kms_helper_poll_init(ddev); omap_modeset_enable_external_hpd();

@@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev) err_cleanup_helpers: omap_modeset_disable_external_hpd(); drm_kms_helper_poll_fini(ddev);

• if (priv->fbdev)

• omap_fbdev_free(ddev);
  

• omap_fbdev_free(ddev);

err_cleanup_modeset: drm_mode_config_cleanup(ddev); omap_drm_irq_uninstall(ddev); @@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev) omap_modeset_disable_external_hpd(); drm_kms_helper_poll_fini(ddev);

• if (priv->fbdev)

• omap_fbdev_free(ddev);
  

• omap_fbdev_free(ddev);

  drm_atomic_helper_shutdown(ddev);

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 632ebcf2165f..30ce3d562414 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi) }

/* initialize fbdev helper */ -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) +void omap_fbdev_init(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private; struct omap_fbdev *fbdev = NULL; @@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)

priv->fbdev = helper;

• return helper;

• return;

fini: drm_fb_helper_fini(helper); @@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) kfree(fbdev);

dev_warn(dev->dev, "omap_fbdev_init failed\n");

While at it I'd remove the error message when drm_fb_helper_init() fails, it duplicates this one.

Ok.

...

• /* well, limp along without an fbdev.. maybe X11 will work? */
• return NULL;

}

void omap_fbdev_free(struct drm_device *dev) @@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev)

DBG();

• if (!priv->fbdev)

• return;
  

I'd either write this as if (!helper) and replace the other occurrences of priv->fbdev below, or replace helper with priv->fbdev in the whole function.

Ok.

Tomi

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:33 EET Tomi Valkeinen wrote:

Add HPD support to the DVI connector driver. The code is almost identical to the HPD code in the HDMI connector driver.

Would it make sense to share a single implementation then ? Or is that an exercise left for the reader (that is, me) ? :-)

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 114 ++++++++++++++++++++ 1 file changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c index 391d80364346..f9f8700223c3 100644 --- a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c +++ b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/slab.h> +#include <linux/gpio/consumer.h>

Could you please keep headers alphabetically sorted ?

#include <drm/drm_edid.h>

@@ -44,6 +45,13 @@ struct panel_drv_data { struct videomode vm;

struct i2c_adapter *i2c_adapter;

• struct gpio_desc *hpd_gpio;
• void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
• void *hpd_cb_data;
• bool hpd_enabled;
• struct mutex hpd_lock;

Locks should have a comment that describes which fields they protect.

};

#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev) @@ -189,6 +197,9 @@ 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;

@@ -220,6 +231,9 @@ static bool dvic_detect(struct omap_dss_device *dssdev) unsigned char out; int r;

• if (ddata->hpd_gpio)

• return gpiod_get_value_cansleep(ddata->hpd_gpio);
  

• if (!ddata->i2c_adapter) return true;

@@ -228,6 +242,60 @@ static bool dvic_detect(struct omap_dss_device *dssdev) return r == 0; }

+static int dvic_register_hpd_cb(struct omap_dss_device *dssdev,

• 		 void (*cb)(void *cb_data,
  

• 			    enum drm_connector_status status),
  

• 		 void *cb_data)
  

+{

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

• return -ENOTSUPP;
  

• mutex_lock(&ddata->hpd_lock);
• ddata->hpd_cb = cb;
• ddata->hpd_cb_data = cb_data;
• mutex_unlock(&ddata->hpd_lock);
• return 0;

+}

+static void dvic_unregister_hpd_cb(struct omap_dss_device *dssdev) +{

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

• return;
  

• mutex_lock(&ddata->hpd_lock);
• ddata->hpd_cb = NULL;
• ddata->hpd_cb_data = NULL;
• mutex_unlock(&ddata->hpd_lock);

+}

+static void dvic_enable_hpd(struct omap_dss_device *dssdev) +{

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

• return;
  

• mutex_lock(&ddata->hpd_lock);
• ddata->hpd_enabled = true;
• mutex_unlock(&ddata->hpd_lock);

+}

+static void dvic_disable_hpd(struct omap_dss_device *dssdev) +{

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

• return;
  

• mutex_lock(&ddata->hpd_lock);
• ddata->hpd_enabled = false;
• mutex_unlock(&ddata->hpd_lock);

+}

static struct omap_dss_driver dvic_driver = { .connect = dvic_connect, .disconnect = dvic_disconnect, @@ -241,14 +309,60 @@ static struct omap_dss_driver dvic_driver = {

.read_edid = dvic_read_edid, .detect = dvic_detect,

• .register_hpd_cb = dvic_register_hpd_cb,
• .unregister_hpd_cb = dvic_unregister_hpd_cb,
• .enable_hpd = dvic_enable_hpd,
• .disable_hpd = dvic_disable_hpd,

};

+static irqreturn_t dvic_hpd_isr(int irq, void *data) +{

• struct panel_drv_data *ddata = data;
• mutex_lock(&ddata->hpd_lock);
• if (ddata->hpd_enabled && ddata->hpd_cb) {

• enum drm_connector_status status;
  

• if (dvic_detect(&ddata->dssdev))
  

• 	status = connector_status_connected;
  

• else
  

• 	status = connector_status_disconnected;
  

• ddata->hpd_cb(ddata->hpd_cb_data, status);
  

• }
• mutex_unlock(&ddata->hpd_lock);
• return IRQ_HANDLED;

+}

static int dvic_probe_of(struct platform_device *pdev) { struct panel_drv_data *ddata = platform_get_drvdata(pdev); struct device_node *node = pdev->dev.of_node; struct device_node *adapter_node; struct i2c_adapter *adapter;

• struct gpio_desc *gpio;
• int r;
• gpio = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
• if (IS_ERR(gpio)) {

• dev_err(&pdev->dev, "failed to parse HPD gpio\n");
  

• return PTR_ERR(gpio);
  

• }
• ddata->hpd_gpio = gpio;
• mutex_init(&ddata->hpd_lock);

Shouldn't you also have a mutex_destroy ?

• if (ddata->hpd_gpio) {

• r = devm_request_threaded_irq(&pdev->dev,
  

• 	gpiod_to_irq(ddata->hpd_gpio), NULL, dvic_hpd_isr,
  

• 	IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
  

• 	"DVI HPD", ddata);
  

• if (r)
  

• 	return r;
  

• }

  adapter_node = of_parse_phandle(node, "ddc-i2c-bus", 0); if (adapter_node) {

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:34 EET Tomi Valkeinen wrote:

Add hpd-gpios property to dvi-connector.txt.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: devicetree@vger.kernel.org

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

Documentation/devicetree/bindings/display/connector/dvi-connector.txt | 1 + 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/connector/dvi-connector.txt b/Documentation/devicetree/bindings/display/connector/dvi-connector.txt index fc53f7c60bc6..207e42e9eba0 100644 --- a/Documentation/devicetree/bindings/display/connector/dvi-connector.txt +++ b/Documentation/devicetree/bindings/display/connector/dvi-connector.txt @@ -10,6 +10,7 @@ Optional properties:

• analog: the connector has DVI analog pins
• digital: the connector has DVI digital pins
• dual-link: the connector has pins for DVI dual-link

+- hpd-gpios: HPD GPIO number

Required nodes:

• Video port for DVI input

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:35 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

It makes the cleanup paths a bit cleaner.

But it also goes in the wrong direction. The data structure is accessible after the .remove() handler returns and thus should outlive the probe/remove sequence through proper reference counting. We're of course not doing a good job here as we kfree() it in .remove() instead of reference-counting it properly, and that should be fixed, but this patch makes the fix more complex as we'll have to move back from devm_kzalloc().

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 485684c637ff..3cd9188ab30b 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -529,19 +529,17 @@ static int pdev_probe(struct platform_device *pdev) return ret; }

• priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

• if (!priv)

• return -ENOMEM;
  

• omap_crtc_pre_init();

  ret = omap_connect_dssdevs(); if (ret) goto err_crtc_uninit;

• /* Allocate and initialize the driver private structure. */
• priv = kzalloc(sizeof(*priv), GFP_KERNEL);
• if (!priv) {

• ret = -ENOMEM;
  

• goto err_disconnect_dssdevs;
  

• }

• /* Initialize the driver private structure. */ priv->dispc_ops = dispc_get_ops();

  soc = soc_device_match(omapdrm_soc_devices);

@@ -555,7 +553,7 @@ static int pdev_probe(struct platform_device *pdev) ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev); if (IS_ERR(ddev)) { ret = PTR_ERR(ddev);

• goto err_free_priv;
  

• goto err_destroy_wq;
  

  }

  ddev->dev_private = priv;

@@ -610,10 +608,8 @@ static int pdev_probe(struct platform_device *pdev) err_free_drm_dev: omap_gem_deinit(ddev); drm_dev_unref(ddev); -err_free_priv: +err_destroy_wq: destroy_workqueue(priv->wq);

• kfree(priv);

-err_disconnect_dssdevs: omap_disconnect_dssdevs(); err_crtc_uninit: omap_crtc_pre_uninit(); @@ -644,7 +640,6 @@ static int pdev_remove(struct platform_device *pdev) drm_dev_unref(ddev);

destroy_workqueue(priv->wq);

• kfree(priv);

  omap_disconnect_dssdevs(); omap_crtc_pre_uninit();

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:36 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

If we allocate the drm_device earlier we can just return the error code without the need to use goto. Do the unref of the drm_device as a last step when cleaning up. This will make the drm_device available longer for us and makes sure that we only free up the memory when all other cleanups have been already done.

How about making it even simpler by embedding drm_device inside omap_drm_private ?

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/omap_drv.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 3cd9188ab30b..57d11f9aeead 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -533,6 +533,14 @@ static int pdev_probe(struct platform_device *pdev) if (!priv) return -ENOMEM;

• /* Allocate and initialize the DRM device. */

• ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);

• if (IS_ERR(ddev))

• return PTR_ERR(ddev);
  

• ddev->dev_private = priv;

• platform_set_drvdata(pdev, ddev);

• omap_crtc_pre_init();

  ret = omap_connect_dssdevs();

@@ -549,16 +557,6 @@ static int pdev_probe(struct platform_device *pdev) spin_lock_init(&priv->list_lock); INIT_LIST_HEAD(&priv->obj_list);

• /* Allocate and initialize the DRM device. */
• ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
• if (IS_ERR(ddev)) {

• ret = PTR_ERR(ddev);
  

• goto err_destroy_wq;
  

• }
• ddev->dev_private = priv;
• platform_set_drvdata(pdev, ddev);
• /* Get memory bandwidth limits */ if (priv->dispc_ops->get_memory_bandwidth_limit) priv->max_bandwidth =

@@ -569,7 +567,7 @@ static int pdev_probe(struct platform_device *pdev) ret = omap_modeset_init(ddev); if (ret) { dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);

• goto err_free_drm_dev;
  

• goto err_gem_deinit;
  

  }

  /* Initialize vblank handling, start with all CRTCs disabled. */

@@ -605,14 +603,13 @@ static int pdev_probe(struct platform_device *pdev) err_cleanup_modeset: drm_mode_config_cleanup(ddev); omap_drm_irq_uninstall(ddev); -err_free_drm_dev: +err_gem_deinit: omap_gem_deinit(ddev);

• drm_dev_unref(ddev);

-err_destroy_wq: destroy_workqueue(priv->wq); omap_disconnect_dssdevs(); err_crtc_uninit: omap_crtc_pre_uninit();

• drm_dev_unref(ddev); return ret;

}

@@ -637,13 +634,13 @@ static int pdev_remove(struct platform_device *pdev) omap_drm_irq_uninstall(ddev); omap_gem_deinit(ddev);

• drm_dev_unref(ddev);

• destroy_workqueue(priv->wq);

  omap_disconnect_dssdevs(); omap_crtc_pre_uninit();

• drm_dev_unref(ddev);
• return 0;

}

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:37 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

Instead of reaching back to DSS to iterate through the dss_devices every time, use an internal array where we store the available and usable dss_devices.

At the same time remove the omapdss_device_is_connected() check from omap_modeset_init() as it became irrelevant: We are not adding dssdevs if their connect failed.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/omap_drv.c | 95 ++++++++++++++++++++--------------- drivers/gpu/drm/omapdrm/omap_drv.h | 3 ++ 2 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 57d11f9aeead..869a8ab6aa4e 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -149,18 +149,27 @@ static int get_connector_type(struct omap_dss_device *dssdev) } }

-static void omap_disconnect_dssdevs(void) +static void omap_disconnect_dssdevs(struct drm_device *ddev) {

• struct omap_dss_device *dssdev = NULL;

• struct omap_drm_private *priv = ddev->dev_private;
• unsigned int i;
• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  
  

• for_each_dss_dev(dssdev) dssdev->driver->disconnect(dssdev);

• priv->dssdevs[i] = NULL;
  

• omap_dss_put_device(dssdev);
  

• }
• priv->num_dssdevs = 0;

}

-static int omap_connect_dssdevs(void) +static int omap_connect_dssdevs(struct drm_device *ddev) {

• int r;

• struct omap_drm_private *priv = ddev->dev_private; struct omap_dss_device *dssdev = NULL;

• int r;

  if (!omapdss_stack_is_ready()) return -EPROBE_DEFER;

@@ -173,6 +182,14 @@ static int omap_connect_dssdevs(void) } else if (r) { dev_warn(dssdev->dev, "could not connect display: %s\n", dssdev->name);

• } else {
  

• 	omap_dss_get_device(dssdev);
  

• 	priv->dssdevs[priv->num_dssdevs++] = dssdev;
  

• 	if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
  

• 		/* To balance the 'for_each_dss_dev' loop */
  

• 		omap_dss_put_device(dssdev);
  

• 		break;
  

• 	}
  

  } }

@@ -183,7 +200,7 @@ static int omap_connect_dssdevs(void) * if we are deferring probe, we disconnect the devices we previously * connected */

• omap_disconnect_dssdevs();

• omap_disconnect_dssdevs(ddev);

  return r;

} @@ -208,7 +225,7 @@ static int omap_modeset_init(struct drm_device *dev) int num_ovls = priv->dispc_ops->get_num_ovls(); int num_mgrs = priv->dispc_ops->get_num_mgrs(); int num_crtcs, crtc_idx, plane_idx;

• int ret;

• int ret, i;

i is never negative, you can make it an unsigned int. Same for the other functions below.

Apart from that,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

u32 plane_crtc_mask;

drm_mode_config_init(dev); @@ -225,11 +242,7 @@ static int omap_modeset_init(struct drm_device *dev) * configuration does not match the expectations or exceeds * the available resources, the configuration is rejected. */

• num_crtcs = 0;
• for_each_dss_dev(dssdev)

• if (omapdss_device_is_connected(dssdev))
  

• 	num_crtcs++;
  

• num_crtcs = priv->num_dssdevs; if (num_crtcs > num_mgrs || num_crtcs > num_ovls || num_crtcs > ARRAY_SIZE(priv->crtcs) || num_crtcs > ARRAY_SIZE(priv->planes) ||

@@ -247,15 +260,13 @@ static int omap_modeset_init(struct drm_device *dev)

crtc_idx = 0; plane_idx = 0;

• for_each_dss_dev(dssdev) {

• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  

  struct drm_connector *connector; struct drm_encoder *encoder; struct drm_plane *plane; struct drm_crtc *crtc;

• if (!omapdss_device_is_connected(dssdev))
  

• 	continue;
  

• encoder = omap_encoder_init(dev, dssdev); if (!encoder) return -ENOMEM;

@@ -329,11 +340,14 @@ static int omap_modeset_init(struct drm_device *dev) /*

• Enable the HPD in external components if supported

*/ -static void omap_modeset_enable_external_hpd(void) +static void omap_modeset_enable_external_hpd(struct drm_device *ddev) {

• struct omap_dss_device *dssdev = NULL;

• struct omap_drm_private *priv = ddev->dev_private;
• int i;
• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  
  

• for_each_dss_dev(dssdev) { if (dssdev->driver->enable_hpd) dssdev->driver->enable_hpd(dssdev); }

@@ -342,11 +356,14 @@ static void omap_modeset_enable_external_hpd(void) /*

• Disable the HPD in external components if supported

*/ -static void omap_modeset_disable_external_hpd(void) +static void omap_modeset_disable_external_hpd(struct drm_device *ddev) {

• struct omap_dss_device *dssdev = NULL;

• struct omap_drm_private *priv = ddev->dev_private;
• int i;
• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  
  

• for_each_dss_dev(dssdev) { if (dssdev->driver->disable_hpd) dssdev->driver->disable_hpd(dssdev); }

@@ -543,7 +560,7 @@ static int pdev_probe(struct platform_device *pdev)

omap_crtc_pre_init();

• ret = omap_connect_dssdevs();

• ret = omap_connect_dssdevs(ddev); if (ret) goto err_crtc_uninit;

@@ -583,7 +600,7 @@ static int pdev_probe(struct platform_device *pdev) omap_fbdev_init(ddev);

drm_kms_helper_poll_init(ddev);

• omap_modeset_enable_external_hpd();

• omap_modeset_enable_external_hpd(ddev);

  /*

  • Register the DRM device with the core and the connectors with

@@ -596,7 +613,7 @@ static int pdev_probe(struct platform_device *pdev) return 0;

err_cleanup_helpers:

• omap_modeset_disable_external_hpd();

• omap_modeset_disable_external_hpd(ddev); drm_kms_helper_poll_fini(ddev);

  omap_fbdev_free(ddev);

@@ -606,7 +623,7 @@ static int pdev_probe(struct platform_device *pdev) err_gem_deinit: omap_gem_deinit(ddev); destroy_workqueue(priv->wq);

• omap_disconnect_dssdevs();

• omap_disconnect_dssdevs(ddev);

err_crtc_uninit: omap_crtc_pre_uninit(); drm_dev_unref(ddev); @@ -622,7 +639,7 @@ static int pdev_remove(struct platform_device *pdev)

drm_dev_unregister(ddev);

• omap_modeset_disable_external_hpd();

• omap_modeset_disable_external_hpd(ddev); drm_kms_helper_poll_fini(ddev);

  omap_fbdev_free(ddev);

@@ -636,7 +653,7 @@ static int pdev_remove(struct platform_device *pdev)

destroy_workqueue(priv->wq);

• omap_disconnect_dssdevs();

• omap_disconnect_dssdevs(ddev); omap_crtc_pre_uninit();

  drm_dev_unref(ddev);

@@ -645,11 +662,14 @@ static int pdev_remove(struct platform_device *pdev) }

#ifdef CONFIG_PM_SLEEP -static int omap_drm_suspend_all_displays(void) +static int omap_drm_suspend_all_displays(struct drm_device *ddev) {

• struct omap_dss_device *dssdev = NULL;

• struct omap_drm_private *priv = ddev->dev_private;
• int i;
• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  
  

• for_each_dss_dev(dssdev) { if (!dssdev->driver) continue;

@@ -664,11 +684,14 @@ static int omap_drm_suspend_all_displays(void) return 0; }

-static int omap_drm_resume_all_displays(void) +static int omap_drm_resume_all_displays(struct drm_device *ddev) {

• struct omap_dss_device *dssdev = NULL;

• struct omap_drm_private *priv = ddev->dev_private;
• int i;
• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  
  

• for_each_dss_dev(dssdev) { if (!dssdev->driver) continue;

@@ -688,7 +711,7 @@ static int omap_drm_suspend(struct device *dev) drm_kms_helper_poll_disable(drm_dev);

drm_modeset_lock_all(drm_dev);

• omap_drm_suspend_all_displays();

• omap_drm_suspend_all_displays(drm_dev); drm_modeset_unlock_all(drm_dev);

  return 0;

@@ -699,7 +722,7 @@ static int omap_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev);

drm_modeset_lock_all(drm_dev);

• omap_drm_resume_all_displays();

• omap_drm_resume_all_displays(drm_dev); drm_modeset_unlock_all(drm_dev);

  drm_kms_helper_poll_enable(drm_dev);

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index ba322c519999..c9e433a91cd0 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -50,6 +50,9 @@ struct omap_drm_private {

const struct dispc_ops *dispc_ops;

• unsigned int num_dssdevs;
• struct omap_dss_device *dssdevs[8];
• unsigned int num_crtcs; struct drm_crtc *crtcs[8];

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:38 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

In order to ease up on the logic,

I have some doubts about this :-) I find the logic both larger (40 insertions, 14 deletions) and more complex.

break the current code to gather the dssdevs:

first get all available dssdevs, then call connect on each dssdev. As the last step remove the dssdevs which failed to connect from the available dssdev list.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/omap_drv.c | 54 ++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 869a8ab6aa4e..b5061fc7241a 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -165,34 +165,60 @@ static void omap_disconnect_dssdevs(struct drm_device *ddev) priv->num_dssdevs = 0; }

-static int omap_connect_dssdevs(struct drm_device *ddev) +static void omap_collect_dssdevs(struct drm_device *ddev) { struct omap_drm_private *priv = ddev->dev_private; struct omap_dss_device *dssdev = NULL;

• int r;

• for_each_dss_dev(dssdev) {

• omap_dss_get_device(dssdev);
  

• priv->dssdevs[priv->num_dssdevs++] = dssdev;
  

• if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
  

• 	/* To balance the 'for_each_dss_dev' loop */
  

• 	omap_dss_put_device(dssdev);
  

• 	break;
  

• }
  

• }

+}

+static int omap_connect_dssdevs(struct drm_device *ddev) +{

• struct omap_drm_private *priv = ddev->dev_private;

• u32 working = 0;

• int r, i, j;

  if (!omapdss_stack_is_ready()) return -EPROBE_DEFER;

• for_each_dss_dev(dssdev) {

• omap_collect_dssdevs(ddev);
• for (i = 0; i < priv->num_dssdevs; i++) {

• struct omap_dss_device *dssdev = priv->dssdevs[i];
  

• r = dssdev->driver->connect(dssdev);

• if (r == -EPROBE_DEFER) {
  

• 	omap_dss_put_device(dssdev);
  

• if (r == -EPROBE_DEFER)
  goto cleanup;
  

• } else if (r) {
  

• else if (r)
  dev_warn(dssdev->dev, "could not connect display: %s\n",
  

• 		dssdev->name);
  

• 		 dssdev->name);
  

• else
  

• 	working |= BIT(i);
  

• }
• /* Remove the dssdevs if their connect failed */
• j = 0;
• for (i = 0; i < priv->num_dssdevs; i++) {

• if (working & BIT(i)) {
  

• 	if (j != i)
  

• 		priv->dssdevs[j] = priv->dssdevs[i];
  

• 	j++;
  

  } else {

• 	omap_dss_get_device(dssdev);
  

• 	priv->dssdevs[priv->num_dssdevs++] = dssdev;
  

• 	if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
  

• 		/* To balance the 'for_each_dss_dev' loop */
  

• 		omap_dss_put_device(dssdev);
  

• 		break;
  

• 	}
  

• 	omap_dss_put_device(priv->dssdevs[i]);
  

  } }

• priv->num_dssdevs = j;

• return 0;

cleanup:

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:39 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

Sort the dssdev array based on DT aliases.

With this change we can remove the panel ordering from dss/display.c and have all sorting related to dssdevs in one place.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/dss/display.c | 2 ++ drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 + drivers/gpu/drm/omapdrm/omap_drv.c | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/display.c b/drivers/gpu/drm/omapdrm/dss/display.c index 424143128cd4..3ef99f344bd3 100644 --- a/drivers/gpu/drm/omapdrm/dss/display.c +++ b/drivers/gpu/drm/omapdrm/dss/display.c @@ -52,6 +52,8 @@ int omapdss_register_display(struct omap_dss_device *dssdev) if (id < 0) id = disp_num_counter++;

• dssdev->alias_id = id;

• snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);

  /* Use 'label' property for name, if it exists */

diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 4222661d4c88..bd5f174a3c56 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -478,6 +478,7 @@ struct omap_dss_device {

/* alias in the form of "display%d" */ char alias[16];

• int alias_id;

Can the alias ID be negative ? Apart from that,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

enum omap_display_type type; enum omap_display_type output_type; diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index b5061fc7241a..877c3749f69b 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -15,6 +15,8 @@

• this program. If not, see http://www.gnu.org/licenses/.

*/

+#include <linux/of.h> +#include <linux/sort.h> #include <linux/sys_soc.h>

#include <drm/drm_atomic.h> @@ -165,6 +167,18 @@ static void omap_disconnect_dssdevs(struct drm_device *ddev) priv->num_dssdevs = 0; }

+static int omap_compare_dssdevs(const void *a, const void *b) +{

• const struct omap_dss_device *dssdev1 = *(struct omap_dss_device **)a;
• const struct omap_dss_device *dssdev2 = *(struct omap_dss_device **)b;
• if (dssdev1->alias_id > dssdev2->alias_id)

• return 1;
  

• else if (dssdev1->alias_id < dssdev2->alias_id)

• return -1;
  

• return 0;

+}

static void omap_collect_dssdevs(struct drm_device *ddev) { struct omap_drm_private *priv = ddev->dev_private; @@ -179,6 +193,10 @@ static void omap_collect_dssdevs(struct drm_device *ddev) break; } }

• /* Sort the list by DT aliases */
• sort(priv->dssdevs, priv->num_dssdevs, sizeof(priv->dssdevs[0]),

•     omap_compare_dssdevs, NULL);
  

}

static int omap_connect_dssdevs(struct drm_device *ddev)

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:40 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

The previous patch implements the ordering of the dss_devices based on DT aliases in omap_drm.c, so there is no need to do the ordering in dss/display.c anymore.

At the same time remove the alias member of the omap_dss_device struct since it is no longer needed. The only place it was used is in the omapdss_register_display() function.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/dss/display.c | 15 +++------------ drivers/gpu/drm/omapdrm/dss/omapdss.h | 2 -- 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/display.c b/drivers/gpu/drm/omapdrm/dss/display.c index 3ef99f344bd3..7840837f4325 100644 --- a/drivers/gpu/drm/omapdrm/dss/display.c +++ b/drivers/gpu/drm/omapdrm/dss/display.c @@ -41,7 +41,6 @@ static int disp_num_counter; int omapdss_register_display(struct omap_dss_device *dssdev) { struct omap_dss_driver *drv = dssdev->driver;

• struct list_head *cur; int id;

  /*

@@ -54,26 +53,18 @@ int omapdss_register_display(struct omap_dss_device *dssdev)

dssdev->alias_id = id;

• snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);

• /* Use 'label' property for name, if it exists */ of_property_read_string(dssdev->dev->of_node, "label", &dssdev->name);

  if (dssdev->name == NULL)

• dssdev->name = dssdev->alias;
  

• dssdev->name = devm_kasprintf(dssdev->dev, GFP_KERNEL,
  

• 			      "display%d", id);
  

Given that the size of the name is known, how about turning dssdev->name into a fixed-size char array ? That would remove the need for dynamic allocation.

And shouldn't you use %u instead of %d ?

if (drv && drv->get_timings == NULL) drv->get_timings = omapdss_default_get_timings;

mutex_lock(&panel_list_mutex);

• list_for_each(cur, &panel_list) {

• struct omap_dss_device *ldev = list_entry(cur,
  

• 					 struct omap_dss_device,
  

• 					 panel_list);
  

• if (strcmp(ldev->alias, dssdev->alias) > 0)
  

• 	break;
  

• }
• list_add_tail(&dssdev->panel_list, cur);

• list_add_tail(&dssdev->panel_list, &panel_list); mutex_unlock(&panel_list_mutex); return 0;

} diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index bd5f174a3c56..ac8ca37ff889 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -476,8 +476,6 @@ struct omap_dss_device {

struct list_head panel_list;

• /* alias in the form of "display%d" */

• char alias[16]; int alias_id;

  enum omap_display_type type;

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:41 EET Tomi Valkeinen wrote:

From: Peter Ujfalusi peter.ujfalusi@ti.com

omapdrm.displays (int array) can be used to reorder the displays by id if needed. It can be also used to disable display.

If the board have two active displays: 0 - LCD 1 - HDMI then: omapdrm.displays=0,1 - represents the original order (LCD, HDMI) omapdrm.displays=1,0 - represents reverse order (HDMI, LCD) omapdrm.displays=0 - only the LCD is enabled omapdrm.displays=1 - only the HDMI is enabled omapdrm.displays=-1 - disable all displays

What's the use case for this ?

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/omap_drv.c | 55 ++++++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 877c3749f69b..5a27a47b628e 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -34,6 +34,14 @@ #define DRIVER_MINOR 0 #define DRIVER_PATCHLEVEL 0

+#define MAX_NR_DISPLAYS 8 +static int display_order[MAX_NR_DISPLAYS]; +static int display_order_nelm; +module_param_array_named(displays, display_order, int, &display_order_nelm,

• 	 0444);
  

+MODULE_PARM_DESC(displays,

•  "ID array to specify the order of the active displays");
  

/*

• mode config funcs

*/ @@ -182,12 +190,21 @@ static int omap_compare_dssdevs(const void *a, const void *b) static void omap_collect_dssdevs(struct drm_device *ddev) { struct omap_drm_private *priv = ddev->dev_private;

• struct omap_dss_device *dssdevs[ARRAY_SIZE(priv->dssdevs)]; struct omap_dss_device *dssdev = NULL;
• int num_dssdevs = 0;

unsigned int.

• unsigned long dssdev_mask = 0;
• int i;

unsigned int.

• /* No displays should be enabled */

• if (display_order_nelm == 1 && display_order[0] < 0)

• return;
  
  

  for_each_dss_dev(dssdev) { omap_dss_get_device(dssdev);

• priv->dssdevs[priv->num_dssdevs++] = dssdev;
  

• if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
  

• set_bit(num_dssdevs, &dssdev_mask);
  

• dssdevs[num_dssdevs++] = dssdev;
  

• if (num_dssdevs == ARRAY_SIZE(dssdevs)) {
  /* To balance the 'for_each_dss_dev' loop */
  omap_dss_put_device(dssdev);
  break;
  

@@ -195,8 +212,38 @@ static void omap_collect_dssdevs(struct drm_device *ddev) }

/* Sort the list by DT aliases */

• sort(priv->dssdevs, priv->num_dssdevs, sizeof(priv->dssdevs[0]),

•     omap_compare_dssdevs, NULL);
  

• sort(dssdevs, num_dssdevs, sizeof(dssdevs[0]), omap_compare_dssdevs,

•     NULL);
  

• /* Do ordering based on the display_order parameter array */
• for (i = 0; i < display_order_nelm; i++) {

• int old_index = display_order[i];
  

• if ((old_index >= 0 && old_index < num_dssdevs) &&
  

No need for the inner parentheses.

•     (dssdev_mask & BIT(old_index))) {
  

• 	priv->dssdevs[priv->num_dssdevs++] = dssdevs[old_index];
  

• 	clear_bit(old_index, &dssdev_mask);
  

• } else {
  

• 	dev_err(ddev->dev,
  

• 		"Ignoring invalid displays module parameter\n");
  

• 	priv->num_dssdevs = 0;
  

• 	break;
  

• }
  

• }
• /* if the target list is empty, copy the collected dssdevs, if any */
• if (priv->num_dssdevs == 0) {

• for (i = 0; i < num_dssdevs; i++)
  

• 	priv->dssdevs[i] = dssdevs[i];
  

• priv->num_dssdevs = num_dssdevs;
  

• } else {

• u32 idx;
  

• /* check if we have dssdev which is not carried over */
  

• for_each_set_bit(idx, &dssdev_mask, ARRAY_SIZE(dssdevs))
  

• 	omap_dss_put_device(dssdevs[idx]);
  

• }

}

static int omap_connect_dssdevs(struct drm_device *ddev)

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:52 EET Tomi Valkeinen wrote:

We define max width and height in mode_config to 2048. These maximums affect many things, which are independent and depend on platform. We need to do more fine grained checks in the code paths for each component, and so the maximum values in mode_config should just be "big enough" to cover all use cases.

Change the maximum width & height to 8192.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/omapdrm/omap_drv.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 5a27a47b628e..2df125369781 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -412,11 +412,14 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 8; dev->mode_config.min_height = 2;

• /* note: eventually will need some cpu_is_omapXYZ() type stuff here

• * to fill in these limits properly on different OMAP generations..
  

• /*

• * Note: these values are used for multiple independent things:
  

• * connector mode filtering, buffer sizes, crtc sizes...
  

• * Use big enough values here to cover all use cases, and do more
  

• * specific checking in the respective code paths.
  

  */

• dev->mode_config.max_width = 2048;
• dev->mode_config.max_height = 2048;

• dev->mode_config.max_width = 8192;
• dev->mode_config.max_height = 8192;

This makes me ponder on the usefulness of the max_width and max_height fields. If we end up having to set them to very large values so they don't get in the way, and implement independent checks manually, the fields should then be split into finer-grained parameters to make DRM core checks useful.

dev->mode_config.funcs = &omap_mode_config_funcs; dev->mode_config.helper_private = &omap_mode_config_helper_funcs;

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:54 EET Tomi Valkeinen wrote:

The setup code for color space conversion is a bit messy. This patch cleans it up.

For some reason the TRM uses values in YCrCb order, which is also used in the current driver, whereas everywhere else it's YCbCr (which also matches YUV order). This patch changes the tables to use the common order to avoid confusion.

The tables are split into separate lines, and comments added for clarity.

WB color conversion registers are similar but different than non-WB, but the same function was used to write both. It worked fine because the coef table was adjusted accordingly, but that was rather confusing. This patch adds a separate function to write the WB values so that the coef table can be written in an understandable way.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/omapdrm/dss/dispc.c | 59 ++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index ff09e2be470f..697274317f7c 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -345,11 +345,6 @@ static const struct { }, };

-struct color_conv_coef {

• int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
• int full_range;

-};

static unsigned long dispc_fclk_rate(void); static unsigned long dispc_core_clk_rate(void); static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel); @@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id plane, int fir_hinc, } }

+struct csc_coef_yuv2rgb {

• int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;

If you made this a 3x3 matrix without explicit names for the fields I think you wouldn't need two structure and two functions.

• bool full_range;

+};

+struct csc_coef_rgb2yuv {

• int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
• bool full_range;

+};

static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,

• const struct color_conv_coef *ct)
  

• const struct csc_coef_yuv2rgb *ct)
  

{ #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))

@@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->bcb));

• REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);

• REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);

true and false should be equal to 1 and 0 respectively, so the !! shouldn't be needed.

+#undef CVAL +}

+static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv *ct) +{

• const enum omap_plane_id plane = OMAP_DSS_WB;

+#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))

• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg, ct->yr));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
• REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);

#undef CVAL } @@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void) { int i; int num_ovl = dispc_get_num_ovls();

• const struct color_conv_coef ctbl_bt601_5_ovl = {

• /* YUV -> RGB */
  

• 298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
  

• /* YUV -> RGB, ITU-R BT.601, limited range */
• const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298,    0,  409,	/* ry, rcb, rcr */
  

• 298, -100, -208,	/* gy, gcb, gcr */
  

• 298,  516,    0,	/* by, bcb, bcr */
  

The 517 turned into 516, was that intentional ?

• false,			/* limited range */
  

  };

• const struct color_conv_coef ctbl_bt601_5_wb = {

• /* RGB -> YUV */
  

• 66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
  

• /* RGB -> YUV, ITU-R BT.601, limited range */

• const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {

•  66, 129,  25,		/* yr,   yg,  yb */
  

• -38, -74, 112,		/* cbr, cbg, cbb */
  

• 112, -94, -18,		/* crr, crg, crb */
  

• false,			/* limited range */
  

  };

  for (i = 1; i < num_ovl; i++)

• dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
  

• dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
  
  

  if (dispc.feat->has_writeback)

• dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, &ctbl_bt601_5_wb);
  

• dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
  

}

static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

Hi Tomi,

On Wednesday, 28 February 2018 12:09:35 EET Tomi Valkeinen wrote:

On 27/02/18 16:08, Laurent Pinchart wrote:

...

On Monday, 12 February 2018 11:44:54 EET Tomi Valkeinen wrote:

...

The setup code for color space conversion is a bit messy. This patch cleans it up.

For some reason the TRM uses values in YCrCb order, which is also used in the current driver, whereas everywhere else it's YCbCr (which also matches YUV order). This patch changes the tables to use the common order to avoid confusion.

The tables are split into separate lines, and comments added for clarity.

WB color conversion registers are similar but different than non-WB, but the same function was used to write both. It worked fine because the coef table was adjusted accordingly, but that was rather confusing. This patch adds a separate function to write the WB values so that the coef table can be written in an understandable way.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/omapdrm/dss/dispc.c | 59 ++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index ff09e2be470f..697274317f7c 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -345,11 +345,6 @@ static const struct { },

};

...

-struct color_conv_coef {

• int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
• int full_range;

-};

static unsigned long dispc_fclk_rate(void); static unsigned long dispc_core_clk_rate(void); static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel); @@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id plane, int fir_hinc, }

}

+struct csc_coef_yuv2rgb {

• int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;

If you made this a 3x3 matrix without explicit names for the fields I think you wouldn't need two structure and two functions.

That is true, but I think it would also make the code more difficult to follow. It's not exactly obvious which coefficients go where.

I don't expect this code to be often read, and I think that with appropriate comments in the source code and proper formatting of the table, as you do below, it should be clear enough, but I'll leave it up to you. We're really programming a 3x3 matrix in the hardware, so I think that modeling it as a 3x3 matrix in the driver makes sense :-)

...

...

• bool full_range;

+};

+struct csc_coef_rgb2yuv {

• int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
• bool full_range;

+};

static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,

• const struct color_conv_coef *ct)
  

• const struct csc_coef_yuv2rgb *ct)
  
  

{ #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))

@@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->bcb));

• REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);

• REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);

true and false should be equal to 1 and 0 respectively, so the !! shouldn't be needed.

Yep.

...

...

+#undef CVAL +}

+static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv *ct) +{

• const enum omap_plane_id plane = OMAP_DSS_WB;

+#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))

• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg, ct->yr));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
• dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
• REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);

#undef CVAL }

@@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void) { int i; int num_ovl = dispc_get_num_ovls();

• const struct color_conv_coef ctbl_bt601_5_ovl = {

• /* YUV -> RGB */
  

• 298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
  

• /* YUV -> RGB, ITU-R BT.601, limited range */
• const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298,    0,  409,	/* ry, rcb, rcr */
  

• 298, -100, -208,	/* gy, gcb, gcr */
  

• 298,  516,    0,	/* by, bcb, bcr */
  

The 517 turned into 516, was that intentional ?

Good point. It's been a while, but I did recalculate these, and with rounding, all the other values match except the 517, so I changed it according to my calculations.

That said, I'm not sure if the original calculations are correct, as they seem to use 256 as multiplier, which would not result 0xff from 1.0 value. In any case, I changed the single value that seemed to be off from the other ones.

I have no issue with that, but could you please mention it in the commit message ?