[PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Peter Ujfalusi]

Hi,

Changes since v1:
- rebased it on drm-next
- Dropped the devm_kzalloc conversion patch

Changes since RFC:
- Comments from Laurent have been addressed:
 - Get alias ID once and store it for later use in sorting
 - Commit message updated for 'drm/omap: Manage the usable omap_dss_device list
   within omap_drm_private' patch
- I have kept the first patch to convert to use devm_kzalloc for the private
  struct as I still think it is as correct as the way Laurent is proposing.

The series adds support for changing the order of the displays defined by DT
display aliases.

The motivation to do such a thing is that for example the fb emulation is
treating the first display/crtc as the 'main' display and will create the
fb emulation based on the first display's properties.
There are many custom applications using DRM directly and they assume that the
first connector is the 'main' display.
Afaik weston provides no means either to change the 'main/preferred' display.

It should be the work of user space application (except the fb emulation) to
somehow deal with the 'main' display selection for their needs, but
unfortunately they are not capable of diong so for some reason.

We have boards with LCD panel and HDMI for example and in DT the LCD is set as
display0, but in certain useage scenarios it is desired to have the HDMI as the
'main' display instead of the LCD.

With the kernel cmd line parameter it is possible to change the pre defined
order without recompiling the kernel/DT.

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

The first 6 patch of the series is doing some generic clean up and prepares the
code so the display ordering is going to be easy to add.

Regards,
Peter
---
Peter Ujfalusi (6):
  drm/omap: Allocate drm_device earlier and unref it as last step
  drm/omap: Manage the usable omap_dss_device list within
    omap_drm_private
  drm/omap: Separate the dssdevs array setup from the connect function
  drm/omap: Do dss_device (display) ordering in omap_drv.c
  drm/omap: dss: Remove display ordering from dss/display.c
  drm/omap: Add kernel parameter to specify the desired display order

 drivers/gpu/drm/omapdrm/dss/display.c |  15 +--
 drivers/gpu/drm/omapdrm/dss/omapdss.h |   3 +-
 drivers/gpu/drm/omapdrm/omap_drv.c    | 225 +++++++++++++++++++++++++---------
 drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
 4 files changed, 176 insertions(+), 70 deletions(-)

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 1/6] drm/omap: Allocate drm_device earlier and unref it as last step

[Peter Ujfalusi]

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.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 ef3b0e3571ec..aeef9e8e6932 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -530,6 +530,14 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	priv->dispc = dispc_get_dispc(priv->dss);
 	priv->dispc_ops = dispc_get_ops(priv->dss);
 
+	/* Allocate and initialize the DRM device. */
+	ddev = drm_dev_alloc(&omap_drm_driver, priv->dev);
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	priv->ddev = ddev;
+	ddev->dev_private = priv;
+
 	omap_crtc_pre_init(priv);
 
 	ret = omap_connect_dssdevs();
@@ -543,16 +551,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	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, priv->dev);
-	if (IS_ERR(ddev)) {
-		ret = PTR_ERR(ddev);
-		goto err_destroy_wq;
-	}
-
-	priv->ddev = ddev;
-	ddev->dev_private = priv;
-
 	/* Get memory bandwidth limits */
 	if (priv->dispc_ops->get_memory_bandwidth_limit)
 		priv->max_bandwidth =
@@ -563,7 +561,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	ret = omap_modeset_init(ddev);
 	if (ret) {
 		dev_err(priv->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. */
@@ -599,14 +597,13 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 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;
 }
 
@@ -630,12 +627,12 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
 	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);
 }
 
 static int pdev_probe(struct platform_device *pdev)
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 2/6] drm/omap: Manage the usable omap_dss_device list within omap_drm_private

[Peter Ujfalusi]

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>
---
 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 aeef9e8e6932..52cb8df04551 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(priv->dispc);
 	int num_mgrs = priv->dispc_ops->get_num_mgrs(priv->dispc);
 	int num_crtcs, crtc_idx, plane_idx;
-	int ret;
+	int ret, i;
 	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;
@@ -335,11 +346,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);
 	}
@@ -348,11 +362,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);
 	}
@@ -540,7 +557,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 
 	omap_crtc_pre_init(priv);
 
-	ret = omap_connect_dssdevs();
+	ret = omap_connect_dssdevs(ddev);
 	if (ret)
 		goto err_crtc_uninit;
 
@@ -577,7 +594,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	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
@@ -590,7 +607,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	return 0;
 
 err_cleanup_helpers:
-	omap_modeset_disable_external_hpd();
+	omap_modeset_disable_external_hpd(ddev);
 	drm_kms_helper_poll_fini(ddev);
 
 	omap_fbdev_fini(ddev);
@@ -600,7 +617,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 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);
@@ -615,7 +632,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
 
 	drm_dev_unregister(ddev);
 
-	omap_modeset_disable_external_hpd();
+	omap_modeset_disable_external_hpd(ddev);
 	drm_kms_helper_poll_fini(ddev);
 
 	omap_fbdev_fini(ddev);
@@ -629,7 +646,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
 
 	destroy_workqueue(priv->wq);
 
-	omap_disconnect_dssdevs();
+	omap_disconnect_dssdevs(ddev);
 	omap_crtc_pre_uninit();
 
 	drm_dev_unref(ddev);
@@ -674,11 +691,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;
 
@@ -693,11 +713,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;
 
@@ -718,7 +741,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;
@@ -730,7 +753,7 @@ static int omap_drm_resume(struct device *dev)
 	struct drm_device *drm_dev = priv->ddev;
 
 	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 6eaee4df4559..69d261f8e56b 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -54,6 +54,9 @@ struct omap_drm_private {
 	struct dispc_device *dispc;
 	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];
 
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 3/6] drm/omap: Separate the dssdevs array setup from the connect function

[Peter Ujfalusi]

In order to ease up on the logic, 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>
---
 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 52cb8df04551..2cd2349622a9 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:
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 4/6] drm/omap: Do dss_device (display) ordering in omap_drv.c

[Peter Ujfalusi]

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>
---
 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 14d74adb13fb..d1056d0d32c2 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -467,6 +467,7 @@ struct omap_dss_device {
 
 	/* alias in the form of "display%d" */
 	char alias[16];
+	int alias_id;
 
 	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 2cd2349622a9..65666e946799 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)
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 5/6] drm/omap: dss: Remove display ordering from dss/display.c

[Peter Ujfalusi]

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>
---
 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);
 
 	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 d1056d0d32c2..d22212fd2f2a 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -465,8 +465,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;
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 6/6] drm/omap: Add kernel parameter to specify the desired display order

[Peter Ujfalusi]

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@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 65666e946799..645674d4be71 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 long dssdev_mask = 0;
+	int i;
+
+	/* 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) &&
+		    (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)
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Laurent Pinchart]

Hi Peter,

On Wednesday, 21 March 2018 12:08:25 EEST Peter Ujfalusi wrote:
> Hi,
> 
> Changes since v1:
> - rebased it on drm-next
> - Dropped the devm_kzalloc conversion patch
> 
> Changes since RFC:
> - Comments from Laurent have been addressed:
>  - Get alias ID once and store it for later use in sorting
>  - Commit message updated for 'drm/omap: Manage the usable omap_dss_device
> list within omap_drm_private' patch
> - I have kept the first patch to convert to use devm_kzalloc for the private
> struct as I still think it is as correct as the way Laurent is proposing.
> 
> The series adds support for changing the order of the displays defined by DT
> display aliases.
> 
> The motivation to do such a thing is that for example the fb emulation is
> treating the first display/crtc as the 'main' display and will create the
> fb emulation based on the first display's properties.
> There are many custom applications using DRM directly and they assume that
> the first connector is the 'main' display.
> Afaik weston provides no means either to change the 'main/preferred'
> display.

I'm still not sure that this problem should be addressed in the kernel. 
However, regardless of that, I found patches 1/6, 2/6, 4/6 and 5/6 useful for 
the omapdrm cleanup and rework I'm working on.

The first version can be found in the "[PATCH/RFC 00/60] omapdrm: Reverse 
direction of DSS device (dis)connect operations" patch series, and I will send 
a second version in the near future.

> It should be the work of user space application (except the fb emulation) to
> somehow deal with the 'main' display selection for their needs, but
> unfortunately they are not capable of diong so for some reason.
> 
> We have boards with LCD panel and HDMI for example and in DT the LCD is set
> as display0, but in certain useage scenarios it is desired to have the HDMI
> as the 'main' display instead of the LCD.
> 
> With the kernel cmd line parameter it is possible to change the pre defined
> order without recompiling the kernel/DT.
> 
> 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
> 
> The first 6 patch of the series is doing some generic clean up and prepares
> the code so the display ordering is going to be easy to add.
> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (6):
>   drm/omap: Allocate drm_device earlier and unref it as last step
>   drm/omap: Manage the usable omap_dss_device list within
>     omap_drm_private
>   drm/omap: Separate the dssdevs array setup from the connect function
>   drm/omap: Do dss_device (display) ordering in omap_drv.c
>   drm/omap: dss: Remove display ordering from dss/display.c
>   drm/omap: Add kernel parameter to specify the desired display order
> 
>  drivers/gpu/drm/omapdrm/dss/display.c |  15 +--
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   3 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c    | 225 ++++++++++++++++++++++++-------
>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
>  4 files changed, 176 insertions(+), 70 deletions(-)

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4442 bytes --]

On Wed, 21 Mar 2018 12:08:25 +0200
Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> Hi,
> 
> Changes since v1:
> - rebased it on drm-next
> - Dropped the devm_kzalloc conversion patch
> 
> Changes since RFC:
> - Comments from Laurent have been addressed:
>  - Get alias ID once and store it for later use in sorting
>  - Commit message updated for 'drm/omap: Manage the usable omap_dss_device list
>    within omap_drm_private' patch
> - I have kept the first patch to convert to use devm_kzalloc for the private
>   struct as I still think it is as correct as the way Laurent is proposing.
> 
> The series adds support for changing the order of the displays defined by DT
> display aliases.
> 
> The motivation to do such a thing is that for example the fb emulation is
> treating the first display/crtc as the 'main' display and will create the
> fb emulation based on the first display's properties.

Hi,

catering for fbdev seems strange to me. But if you do care, why is this
a driver-specific option instead of a generic DRM fb emulation
configuration option?

> There are many custom applications using DRM directly and they assume that the
> first connector is the 'main' display.

This is very unfortunate, but I still think it would be better to fix
all those apps than patch kernel drivers one at a time to cater for
user preferences with driver-specific kernel options.

> Afaik weston provides no means either to change the 'main/preferred' display.

Please, do not use Weston as justification for this. If you have window
positioning problems in Weston, please come talk to us on #wayland or
wayland-devel@, so we can discuss what you *actually* need.

Weston's behaviour in this respect has not been even defined yet, which
makes a kernel option to work around it ever more awkward.

The reason why Weston lacks this kind of configurability is that no-one
has needed it yet, I mean, come forth with a proposal, as far as I can
recall. If people keep working around it elsewhere, it is unlikely Weston
ever gets it.

> It should be the work of user space application (except the fb emulation) to
> somehow deal with the 'main' display selection for their needs, but
> unfortunately they are not capable of diong so for some reason.

Aside from Weston, which apps are these?

> We have boards with LCD panel and HDMI for example and in DT the LCD is set as
> display0, but in certain useage scenarios it is desired to have the HDMI as the
> 'main' display instead of the LCD.
> 
> With the kernel cmd line parameter it is possible to change the pre defined
> order without recompiling the kernel/DT.
> 
> 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
> 
> The first 6 patch of the series is doing some generic clean up and prepares the
> code so the display ordering is going to be easy to add.

Libweston has just received a bunch of patches rewriting the whole
output configuration API. Now it allows force-enabling outputs and
programming shared-CRTC clone mode. A next logical step would be to
bring output layout configuration out of libweston and into weston, so
that the layout could actually be configured at all. One could
introduce the concept of a "primary" output at the same time and fix
the window manager to do something useful with it.


Thanks,
pq

> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (6):
>   drm/omap: Allocate drm_device earlier and unref it as last step
>   drm/omap: Manage the usable omap_dss_device list within
>     omap_drm_private
>   drm/omap: Separate the dssdevs array setup from the connect function
>   drm/omap: Do dss_device (display) ordering in omap_drv.c
>   drm/omap: dss: Remove display ordering from dss/display.c
>   drm/omap: Add kernel parameter to specify the desired display order
> 
>  drivers/gpu/drm/omapdrm/dss/display.c |  15 +--
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   3 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c    | 225 +++++++++++++++++++++++++---------
>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
>  4 files changed, 176 insertions(+), 70 deletions(-)
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Tomi Valkeinen]

Hi,

On 28/05/18 13:55, Pekka Paalanen wrote:

>> The series adds support for changing the order of the displays defined by DT
>> display aliases.
>>
>> The motivation to do such a thing is that for example the fb emulation is
>> treating the first display/crtc as the 'main' display and will create the
>> fb emulation based on the first display's properties.
> 
> Hi,
> 
> catering for fbdev seems strange to me. But if you do care, why is this

Strange how? Because we should get rid of it? Sure. But it's there, and
it's being used by almost everyone, at least in the form of fb console.

> a driver-specific option instead of a generic DRM fb emulation
> configuration option?

That'd be fine for me, but I think the actual code to do this
"create-connectors-in-this-order" work is still driver specific. Perhaps
we could have a simple option to just choose the "main" display for
fbdev (without chaing the way the connectors are created), though, which
could perhaps be common code.

>> There are many custom applications using DRM directly and they assume that the
>> first connector is the 'main' display.
> 
> This is very unfortunate, but I still think it would be better to fix
> all those apps than patch kernel drivers one at a time to cater for
> user preferences with driver-specific kernel options.

I agree, but it's still a problem because we don't have access to those
apps. And they keep popping up when e.g. being used on a new platform
with multiple displays.

>> Afaik weston provides no means either to change the 'main/preferred' display.
> 
> Please, do not use Weston as justification for this. If you have window
> positioning problems in Weston, please come talk to us on #wayland or
> wayland-devel@, so we can discuss what you *actually* need.

Weston is just one piece of the puzzle. If Weston was the only problem,
then yes, we'd be working on Weston. But it's the combination of fbdev,
custom DRM based apps and Weston.

> Weston's behaviour in this respect has not been even defined yet, which
> makes a kernel option to work around it ever more awkward.
> 
> The reason why Weston lacks this kind of configurability is that no-one
> has needed it yet, I mean, come forth with a proposal, as far as I can
> recall. If people keep working around it elsewhere, it is unlikely Weston
> ever gets it.
> 
>> It should be the work of user space application (except the fb emulation) to
>> somehow deal with the 'main' display selection for their needs, but
>> unfortunately they are not capable of diong so for some reason.
> 
> Aside from Weston, which apps are these?

Mostly custom proprietary apps. But I think also Qt's DRM backend had
this problem, although I might remember wrong or it might have been
improved.

Overall, I don't like these module parameters much. But they did solve
issues our customers were having quite easily.

I am fine with dropping this from mainline, and just carrying it in the
TI kernel until we've come up with other solutions to the problems.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Daniel Vetter]

On Mon, May 28, 2018 at 02:44:28PM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 28/05/18 13:55, Pekka Paalanen wrote:
> 
> >> The series adds support for changing the order of the displays defined by DT
> >> display aliases.
> >>
> >> The motivation to do such a thing is that for example the fb emulation is
> >> treating the first display/crtc as the 'main' display and will create the
> >> fb emulation based on the first display's properties.
> > 
> > Hi,
> > 
> > catering for fbdev seems strange to me. But if you do care, why is this
> 
> Strange how? Because we should get rid of it? Sure. But it's there, and
> it's being used by almost everyone, at least in the form of fb console.
> 
> > a driver-specific option instead of a generic DRM fb emulation
> > configuration option?
> 
> That'd be fine for me, but I think the actual code to do this
> "create-connectors-in-this-order" work is still driver specific. Perhaps
> we could have a simple option to just choose the "main" display for
> fbdev (without chaing the way the connectors are created), though, which
> could perhaps be common code.

Thus far the rules has been that we try to put the integrated panel first,
and that's it. See drm_helper_move_panel_connectors_to_head(). If there's
any standard, then imo that's it. Not a more generic "the first display is
the preferred one" because frankly we don't know that - or how do you
guess that I want to use my laptop just as a work-station, and never use
the integrated panel when the hdmi thing is connected? You need
configurability in userspace and fbdev for that.

And yes, for fbdev some fbdev option would probably be best. We already
have options/parsing to disable/force certain connectors, we could also
have another flag to designate it the preferred primary.

Also, the only thing where primary/secondary matters for fbdev emulation
is for the vblank handling. Afaiui that was added for the gpu blob drivers
being stupid and refusing to use kms. Definitely don't want to add huge
driver-specific hacks for that :-)

> >> There are many custom applications using DRM directly and they assume that the
> >> first connector is the 'main' display.
> > 
> > This is very unfortunate, but I still think it would be better to fix
> > all those apps than patch kernel drivers one at a time to cater for
> > user preferences with driver-specific kernel options.
> 
> I agree, but it's still a problem because we don't have access to those
> apps. And they keep popping up when e.g. being used on a new platform
> with multiple displays.
> 
> >> Afaik weston provides no means either to change the 'main/preferred' display.
> > 
> > Please, do not use Weston as justification for this. If you have window
> > positioning problems in Weston, please come talk to us on #wayland or
> > wayland-devel@, so we can discuss what you *actually* need.
> 
> Weston is just one piece of the puzzle. If Weston was the only problem,
> then yes, we'd be working on Weston. But it's the combination of fbdev,
> custom DRM based apps and Weston.
> 
> > Weston's behaviour in this respect has not been even defined yet, which
> > makes a kernel option to work around it ever more awkward.
> > 
> > The reason why Weston lacks this kind of configurability is that no-one
> > has needed it yet, I mean, come forth with a proposal, as far as I can
> > recall. If people keep working around it elsewhere, it is unlikely Weston
> > ever gets it.
> > 
> >> It should be the work of user space application (except the fb emulation) to
> >> somehow deal with the 'main' display selection for their needs, but
> >> unfortunately they are not capable of diong so for some reason.
> > 
> > Aside from Weston, which apps are these?
> 
> Mostly custom proprietary apps. But I think also Qt's DRM backend had
> this problem, although I might remember wrong or it might have been
> improved.
> 
> Overall, I don't like these module parameters much. But they did solve
> issues our customers were having quite easily.
> 
> I am fine with dropping this from mainline, and just carrying it in the
> TI kernel until we've come up with other solutions to the problems.

Imo the fbdev one makes some sense, but if the only reason for it is the
userspace gpu blob maybe not so much. For everything else, you need some
form of configuration in userspace anyway. KMS doesn't have a concept of
"primary" display, and I have no idea how we'd even make that happen
everywhere.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Tomi Valkeinen]

On 29/05/18 09:42, Daniel Vetter wrote:

> Thus far the rules has been that we try to put the integrated panel first,
> and that's it. See drm_helper_move_panel_connectors_to_head(). If there's

Ok. But we may also have multiple integrated panels.

> any standard, then imo that's it. Not a more generic "the first display is
> the preferred one" because frankly we don't know that - or how do you
> guess that I want to use my laptop just as a work-station, and never use
> the integrated panel when the hdmi thing is connected? You need
> configurability in userspace and fbdev for that.

I can't guess, which is why we added this parameter. I agree that 
(ignoring fbdev for now) the userspace should handle this. But as 
described in drm_helper_move_panel_connectors_to_head(), this isn't the 
case at the moment.

> And yes, for fbdev some fbdev option would probably be best. We already
> have options/parsing to disable/force certain connectors, we could also
> have another flag to designate it the preferred primary.

It's true that the only important thing here is the "primary", i.e. we 
don't care how the second or third displays are ordered.

We wanted to use the connector names here in the parameter, but 
connector names are not available before the connectors are created, so 
we ended up with the list of numbers...

> Also, the only thing where primary/secondary matters for fbdev emulation
> is for the vblank handling. Afaiui that was added for the gpu blob drivers
> being stupid and refusing to use kms. Definitely don't want to add huge
> driver-specific hacks for that :-)

For us it was about the fbdev size, which was setup at probe time to the 
size of the first display (Or smallest of the connected ones? I can't 
remember). We wanted the fbdev size to be the size of the "primary" display.

> Imo the fbdev one makes some sense, but if the only reason for it is the
> userspace gpu blob maybe not so much. For everything else, you need some
> form of configuration in userspace anyway. KMS doesn't have a concept of
> "primary" display, and I have no idea how we'd even make that happen
> everywhere.

We were not aiming at a "primary" display in the sense that it'd be 
something that the userspace should care about. More like a 
fallback-primary-display, in case the userspace only uses the first 
connector.

But I do agree that this feels hacky. So we can carry this in the TI 
kernel for now as a temporary measure.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/6] drm/omap: Module parameter for display order configuration

[Daniel Vetter]

On Tue, May 29, 2018 at 10:16:20AM +0300, Tomi Valkeinen wrote:
> On 29/05/18 09:42, Daniel Vetter wrote:
> 
> > Thus far the rules has been that we try to put the integrated panel first,
> > and that's it. See drm_helper_move_panel_connectors_to_head(). If there's
> 
> Ok. But we may also have multiple integrated panels.
> 
> > any standard, then imo that's it. Not a more generic "the first display is
> > the preferred one" because frankly we don't know that - or how do you
> > guess that I want to use my laptop just as a work-station, and never use
> > the integrated panel when the hdmi thing is connected? You need
> > configurability in userspace and fbdev for that.
> 
> I can't guess, which is why we added this parameter. I agree that (ignoring
> fbdev for now) the userspace should handle this. But as described in
> drm_helper_move_panel_connectors_to_head(), this isn't the case at the
> moment.
> 
> > And yes, for fbdev some fbdev option would probably be best. We already
> > have options/parsing to disable/force certain connectors, we could also
> > have another flag to designate it the preferred primary.
> 
> It's true that the only important thing here is the "primary", i.e. we don't
> care how the second or third displays are ordered.
> 
> We wanted to use the connector names here in the parameter, but connector
> names are not available before the connectors are created, so we ended up
> with the list of numbers...
> 
> > Also, the only thing where primary/secondary matters for fbdev emulation
> > is for the vblank handling. Afaiui that was added for the gpu blob drivers
> > being stupid and refusing to use kms. Definitely don't want to add huge
> > driver-specific hacks for that :-)
> 
> For us it was about the fbdev size, which was setup at probe time to the
> size of the first display (Or smallest of the connected ones? I can't
> remember). We wanted the fbdev size to be the size of the "primary" display.

It's smallest of all connected ones. Atm you do not get fbdev size == size
of primary. It's only >= size of primary. And no way to modeset :-/

> > Imo the fbdev one makes some sense, but if the only reason for it is the
> > userspace gpu blob maybe not so much. For everything else, you need some
> > form of configuration in userspace anyway. KMS doesn't have a concept of
> > "primary" display, and I have no idea how we'd even make that happen
> > everywhere.
> 
> We were not aiming at a "primary" display in the sense that it'd be
> something that the userspace should care about. More like a
> fallback-primary-display, in case the userspace only uses the first
> connector.
> 
> But I do agree that this feels hacky. So we can carry this in the TI kernel
> for now as a temporary measure.

One even more impressive hack would be to have this generic in the drm
core. I.e. at drm_dev_register time we'd move the primary connector to the
front. Only issue is that this might upset the driver, so we might need to
do that only for the getresources ioctl (and not for anything else).

The problem with that is that drm connectors have no unique/stable names,
so I'm not sure how we could make this work correctly ....
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel