[RFC 0/7] drm/omap: Module parameter for display order configuration

[RFC 0/7] drm/omap: Module parameter for display order configuration

[Peter Ujfalusi]

Hi

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 (7):
  drm/omap: Use devm_kzalloc() to allocate omap_drm_private
  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    | 244 ++++++++++++++++++++++++----------
 drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
 4 files changed, 183 insertions(+), 82 deletions(-)

-- 
2.14.1


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

[RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Peter Ujfalusi]

It makes the cleanup paths a bit cleaner.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@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 9b3c36b48356..903510d8054e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -548,19 +548,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);
@@ -574,7 +572,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;
@@ -624,10 +622,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();
@@ -659,7 +655,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();
-- 
2.14.1


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: [RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote:
> It makes the cleanup paths a bit cleaner.

But it also goes against a proper implementation of object lifetime management 
:-( In DRM driver private structures are accessible from file operations, and 
must thus be reference-counted and only freed when the last userspace user 
goes away. This may be after the devm_* resources are released right after the 
.remove() operation is called. Using devm_kzalloc() would thus make it 
impossible to properly reference-count our data structures.

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@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 9b3c36b48356..903510d8054e
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -548,19 +548,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);
> @@ -574,7 +572,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;
> @@ -624,10 +622,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();
> @@ -659,7 +655,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();


-- 
Regards,

Laurent Pinchart

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

Re: [RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Peter Ujfalusi]


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

On 2017-09-01 14:10, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote:
>> It makes the cleanup paths a bit cleaner.
> 
> But it also goes against a proper implementation of object lifetime management 
> :-( In DRM driver private structures are accessible from file operations, and 
> must thus be reference-counted and only freed when the last userspace user 
> goes away. This may be after the devm_* resources are released right after the 
> .remove() operation is called. Using devm_kzalloc() would thus make it 
> impossible to properly reference-count our data structures.

Hrm, not sure if I follow you on this.
Before this patch the 'priv' was freed up in pdev_remove(). With this
patch the 'priv' is going to be freed up after the pdev_remove() has
returned, so the 'priv' would be available longer than before as we
converted it as managed resource.

If what you are saying is true, then we already have the same issue and
I don't see how this could have been working or need to work. If you
remove the driver and you need to keep the private data available after
the driver has been unloaded, who is going to free that up?

> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@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 9b3c36b48356..903510d8054e
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -548,19 +548,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);
>> @@ -574,7 +572,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;
>> @@ -624,10 +622,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();
>> @@ -659,7 +655,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();
> 
> 

- Péter

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

Re: [RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Laurent Pinchart]

Hi Peter,

On Monday, 4 September 2017 12:13:40 EEST Peter Ujfalusi wrote:
> On 2017-09-01 14:10, Laurent Pinchart wrote:
> > On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote:
> >> It makes the cleanup paths a bit cleaner.
> > 
> > But it also goes against a proper implementation of object lifetime
> > management :-( In DRM driver private structures are accessible from file
> > operations, and must thus be reference-counted and only freed when the
> > last userspace user goes away. This may be after the devm_* resources are
> > released right after the .remove() operation is called. Using
> > devm_kzalloc() would thus make it impossible to properly reference-count
> > our data structures.
> 
> Hrm, not sure if I follow you on this.
> Before this patch the 'priv' was freed up in pdev_remove(). With this
> patch the 'priv' is going to be freed up after the pdev_remove() has
> returned, so the 'priv' would be available longer than before as we
> converted it as managed resource.
> 
> If what you are saying is true, then we already have the same issue and
> I don't see how this could have been working or need to work. If you
> remove the driver and you need to keep the private data available after
> the driver has been unloaded, who is going to free that up?

At the moment the memory is freed at .remove() time, which can lead to memory 
corruption if a user has a handle on the device (for instance an open file 
handle that is then close()d). Fixing this requires moving memory free to the 
drm_driver::release() handler. devm_kzalloc() goes in the wrong direction.

> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@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 9b3c36b48356..903510d8054e
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> @@ -548,19 +548,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);
> >> 
> >> @@ -574,7 +572,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;
> >> 
> >> @@ -624,10 +622,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();
> >> 
> >> @@ -659,7 +655,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();

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Peter Ujfalusi]

Hi Laurent,


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

On 2017-09-04 12:41, Laurent Pinchart wrote:
> At the moment the memory is freed at .remove() time, which can lead to memory 
> corruption if a user has a handle on the device (for instance an open file 
> handle that is then close()d). Fixing this requires moving memory free to the 
> drm_driver::release() handler. devm_kzalloc() goes in the wrong direction.

Ah, OK, so the current way is buggy as well.

How do you plan to fix that?
I think this should work:

struct omap_drm_private {
	/* First member in the private struct! */
+	struct drm_device ddev;
...
};

Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead
of drm_dev_alloc()

then priv->ddev.dev_private = priv;

in this case the drm_dev_unref() would free up our omap_drm_private, right?

I think this is what other DRM drivers are doing, not all, but i915 does
this at least.

But by the description most of the DRM drivers are doing this wrong, right?

- Péter

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

Re: [RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Laurent Pinchart]

Hi Peter,

On Monday, 4 September 2017 14:16:32 EEST Peter Ujfalusi wrote:
> On 2017-09-04 12:41, Laurent Pinchart wrote:
> > At the moment the memory is freed at .remove() time, which can lead to
> > memory corruption if a user has a handle on the device (for instance an
> > open file handle that is then close()d). Fixing this requires moving
> > memory free to the drm_driver::release() handler. devm_kzalloc() goes in
> > the wrong direction.
> Ah, OK, so the current way is buggy as well.
> 
> How do you plan to fix that?
> I think this should work:
> 
> struct omap_drm_private {
> 	/* First member in the private struct! */
> +	struct drm_device ddev;
> ...
> };
> 
> Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead
> of drm_dev_alloc()
> 
> then priv->ddev.dev_private = priv;
> 
> in this case the drm_dev_unref() would free up our omap_drm_private, right?

That's the idea, yes. I got a local patch for that in my tree.

> I think this is what other DRM drivers are doing, not all, but i915 does
> this at least.
> 
> But by the description most of the DRM drivers are doing this wrong, right?

Correct, most drivers get it wrong. We'll have to fix it, but given that we 
have race conditions in the core that prevent proper hot-unplug support at the 
moment, I didn't want to start pushing for fixing drivers. Once we get the 
core sorted out, it will be time to address the other side of the issue.

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Peter Ujfalusi]

Hi Laurent,


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

On 2017-09-04 17:19, Laurent Pinchart wrote:
>> Ah, OK, so the current way is buggy as well.
>>
>> How do you plan to fix that?
>> I think this should work:
>>
>> struct omap_drm_private {
>> 	/* First member in the private struct! */
>> +	struct drm_device ddev;
>> ...
>> };
>>
>> Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead
>> of drm_dev_alloc()
>>
>> then priv->ddev.dev_private = priv;
>>
>> in this case the drm_dev_unref() would free up our omap_drm_private, right?
> 
> That's the idea, yes. I got a local patch for that in my tree.

Hrm, so what is the difference between what I do in this patch and the
described fix?
To be precise what is the difference between:

struct drm_device *ddev = platform_get_drvdata(pdev);
struct omap_drm_private *priv = ddev->dev_private;
...
drm_dev_unref(ddev);
...
devm would free priv

compared to

struct omap_drm_private {
	/* First member in the private struct! */
	struct drm_device ddev;
	....
};

struct drm_device *ddev = platform_get_drvdata(pdev);
struct omap_drm_private *priv = ddev->dev_private;
...
/* Here we would free priv since &priv->ddev == ddev */
drm_dev_unref(ddev);


The drm_dev_unregister() is provided as a protection for the issue you
are describing, the comment suggest that at least:
 * This should be called first in the device teardown code to make sure
 * userspace can't access the device instance any more.

and we do call it. As the first step.


> 
>> I think this is what other DRM drivers are doing, not all, but i915 does
>> this at least.
>>
>> But by the description most of the DRM drivers are doing this wrong, right?
> 
> Correct, most drivers get it wrong. We'll have to fix it, but given that we 
> have race conditions in the core that prevent proper hot-unplug support at the 
> moment, I didn't want to start pushing for fixing drivers. Once we get the 
> core sorted out, it will be time to address the other side of the issue.
> 

- Péter

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

[RFC 2/7] 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>
---
 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 903510d8054e..bd7fe317a365 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -552,6 +552,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();
@@ -568,22 +576,12 @@ 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);
-
 	omap_gem_init(ddev);
 
 	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. */
@@ -619,14 +617,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;
 }
 
@@ -652,13 +649,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;
 }
 
-- 
2.14.1


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: [RFC 2/7] drm/omap: Allocate drm_device earlier and unref it as last step

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Tuesday, 29 August 2017 10:32:13 EEST Peter Ujfalusi wrote:
> 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>

Pending a new version due to patch 1/7 not being correct,

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 903510d8054e..bd7fe317a365
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -552,6 +552,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();
> @@ -568,22 +576,12 @@ 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);
> -
>  	omap_gem_init(ddev);
> 
>  	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. */
> @@ -619,14 +617,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;
>  }
> 
> @@ -652,13 +649,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;
>  }


-- 
Regards,

Laurent Pinchart

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

[RFC 3/7] 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.

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 bd7fe317a365..b089604e871c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -146,18 +146,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;
+	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;
@@ -170,6 +179,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;
+			}
 		}
 	}
 
@@ -180,7 +197,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;
 }
@@ -205,7 +222,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;
 	u32 plane_crtc_mask;
 
 	drm_mode_config_init(dev);
@@ -222,11 +239,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) ||
@@ -244,15 +257,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;
@@ -326,11 +337,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);
 	}
@@ -339,11 +353,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);
 	}
@@ -562,7 +579,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;
 
@@ -597,7 +614,7 @@ static int pdev_probe(struct platform_device *pdev)
 	priv->fbdev = 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
@@ -610,7 +627,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);
 	if (priv->fbdev)
 		omap_fbdev_free(ddev);
@@ -620,7 +637,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);
@@ -636,7 +653,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);
 
 	if (priv->fbdev)
@@ -651,7 +668,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);
@@ -660,11 +677,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;
 
@@ -679,11 +699,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;
 
@@ -703,7 +726,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;
@@ -714,7 +737,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 4bd1e9070b31..cccaae787a7c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -51,6 +51,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];
 
-- 
2.14.1


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: [RFC 3/7] drm/omap: Manage the usable omap_dss_device list within omap_drm_private

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Tuesday, 29 August 2017 10:32:14 EEST Peter Ujfalusi wrote:
> 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.
> 
> 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 bd7fe317a365..b089604e871c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -146,18 +146,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;
> +	int i;
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {

is is never negative, you can make it an unsigned int. This comment applies 
through the whole patch.

> +		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;
> @@ -170,6 +179,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;
> +			}
>  		}
>  	}
> 
> @@ -180,7 +197,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;
>  }
> @@ -205,7 +222,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;
>  	u32 plane_crtc_mask;
> 
>  	drm_mode_config_init(dev);
> @@ -222,11 +239,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) ||
> @@ -244,15 +257,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;
> -

I believe this hunk is correct as dss devices are only disconnected by calls 
to the oma_dss_driver .disconnect() operation, which is only called from 
omap_disconnect_dssdevs(), but you should at the very least explain why in the 
commit message.

>  		encoder = omap_encoder_init(dev, dssdev);
>  		if (!encoder)
>  			return -ENOMEM;
> @@ -326,11 +337,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);
>  	}
> @@ -339,11 +353,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);
>  	}
> @@ -562,7 +579,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;
> 
> @@ -597,7 +614,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	priv->fbdev = 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
> @@ -610,7 +627,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);
>  	if (priv->fbdev)
>  		omap_fbdev_free(ddev);
> @@ -620,7 +637,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);
> @@ -636,7 +653,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);
> 
>  	if (priv->fbdev)
> @@ -651,7 +668,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);
> @@ -660,11 +677,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;
> 
> @@ -679,11 +699,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;
> 
> @@ -703,7 +726,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;
> @@ -714,7 +737,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 4bd1e9070b31..cccaae787a7c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -51,6 +51,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];


-- 
Regards,

Laurent Pinchart

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

Re: [RFC 3/7] drm/omap: Manage the usable omap_dss_device list within omap_drm_private

[Peter Ujfalusi]

Hi Laurent,


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

On 2017-09-01 14:27, Laurent Pinchart wrote:
>> -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;
>> +	int i;
>> +
>> +	for (i = 0; i < priv->num_dssdevs; i++) {
> 
> is is never negative, you can make it an unsigned int. This comment applies 
> through the whole patch.

True, are there any benefits to have unsigned int compared to int? I
don't think we are going to hit size limitation with the int, but if you
prefer to have unsigned int, then sure, I can change the num_displays
and 'i's to unsigned.

>>  	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;
>> -
> 
> I believe this hunk is correct as dss devices are only disconnected by calls 
> to the oma_dss_driver .disconnect() operation, which is only called from 
> omap_disconnect_dssdevs(), but you should at the very least explain why in the 
> commit message.

The check became irrelevant as we did not add dssdevs to the array if
their connect failed and as you have said we only disconnect ddsdevs in
omap_disconnect_dssdevs() - in cleanup paths.

I will update the commit message with this information.

- Péter

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

Re: [RFC 3/7] drm/omap: Manage the usable omap_dss_device list within omap_drm_private

[Laurent Pinchart]

Hi Peter,

On Monday, 4 September 2017 12:19:19 EEST Peter Ujfalusi wrote:
> On 2017-09-01 14:27, Laurent Pinchart wrote:
> >> -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;
> >> +	int i;
> >> +
> >> +	for (i = 0; i < priv->num_dssdevs; i++) {
> > 
> > is is never negative, you can make it an unsigned int. This comment
> > applies through the whole patch.
> 
> True, are there any benefits to have unsigned int compared to int? I
> don't think we are going to hit size limitation with the int, but if you
> prefer to have unsigned int, then sure, I can change the num_displays
> and 'i's to unsigned.

Making it explicit that the variable can't be negative allows the compiler to 
warn when encoutering a < 0 test. It also makes the code self-documented, 
showing the reader that the variable isn't expected to become negative.

> >>  	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;
> >> -
> > 
> > I believe this hunk is correct as dss devices are only disconnected by
> > calls to the oma_dss_driver .disconnect() operation, which is only called
> > from omap_disconnect_dssdevs(), but you should at the very least explain
> > why in the commit message.
> 
> The check became irrelevant as we did not add dssdevs to the array if
> their connect failed and as you have said we only disconnect ddsdevs in
> omap_disconnect_dssdevs() - in cleanup paths.
> 
> I will update the commit message with this information.

-- 
Regards,

Laurent Pinchart

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

[RFC 4/7] 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 b089604e871c..32dc0e88220f 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -162,34 +162,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:
-- 
2.14.1


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

[RFC 5/7] 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/omap_drv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 32dc0e88220f..0e100a359d26 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -18,6 +18,8 @@
  */
 
 #include <linux/sys_soc.h>
+#include <linux/sort.h>
+#include <linux/of.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -162,6 +164,22 @@ 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;
+	int  id1, id2;
+
+	id1 = of_alias_get_id(dssdev1->dev->of_node, "display");
+	id2 = of_alias_get_id(dssdev2->dev->of_node, "display");
+
+	if (id1 > id2)
+		return 1;
+	else if (id1 < id2)
+		return -1;
+	return 0;
+}
+
 static void omap_collect_dssdevs(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
@@ -176,6 +194,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)
-- 
2.14.1


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: [RFC 5/7] drm/omap: Do dss_device (display) ordering in omap_drv.c

[Laurent Pinchart]

Hi Peter,

Thank  you for the patch.

On Tuesday, 29 August 2017 10:32:16 EEST Peter Ujfalusi wrote:
> 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/omap_drv.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 32dc0e88220f..0e100a359d26
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -18,6 +18,8 @@
>   */
> 
>  #include <linux/sys_soc.h>
> +#include <linux/sort.h>
> +#include <linux/of.h>

Please keep this list alphabetically sorted.

>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -162,6 +164,22 @@ 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;
> +	int  id1, id2;
> +
> +	id1 = of_alias_get_id(dssdev1->dev->of_node, "display");
> +	id2 = of_alias_get_id(dssdev2->dev->of_node, "display");

Getting the alias id is a bit costly, how about caching them ?

> +	if (id1 > id2)
> +		return 1;
> +	else if (id1 < id2)
> +		return -1;
> +	return 0;
> +}
> +
>  static void omap_collect_dssdevs(struct drm_device *ddev)
>  {
>  	struct omap_drm_private *priv = ddev->dev_private;
> @@ -176,6 +194,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)


-- 
Regards,

Laurent Pinchart

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

Re: [RFC 5/7] drm/omap: Do dss_device (display) ordering in omap_drv.c

[Peter Ujfalusi]

Hi Laurent,


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

On 2017-09-01 14:32, Laurent Pinchart wrote:
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> b/drivers/gpu/drm/omapdrm/omap_drv.c index 32dc0e88220f..0e100a359d26
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -18,6 +18,8 @@
>>   */
>>
>>  #include <linux/sys_soc.h>
>> +#include <linux/sort.h>
>> +#include <linux/of.h>
> 
> Please keep this list alphabetically sorted.

OK.

> 
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_atomic_helper.h>
>> @@ -162,6 +164,22 @@ 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;
>> +	int  id1, id2;
>> +
>> +	id1 = of_alias_get_id(dssdev1->dev->of_node, "display");
>> +	id2 = of_alias_get_id(dssdev2->dev->of_node, "display");
> 
> Getting the alias id is a bit costly, how about caching them ?

This is going to be done once and the ID will be never used after this.
We could add the ID to 'struct omap_dss_device' and initialize it in
omapdss_register_display, if that's what you are referring to.

- Péter

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

Re: [RFC 5/7] drm/omap: Do dss_device (display) ordering in omap_drv.c

[Laurent Pinchart]

Hi Peter,

On Monday, 4 September 2017 12:26:08 EEST Peter Ujfalusi wrote:
> On 2017-09-01 14:32, Laurent Pinchart wrote:
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> b/drivers/gpu/drm/omapdrm/omap_drv.c index 32dc0e88220f..0e100a359d26
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> @@ -18,6 +18,8 @@
> >> 
> >>   */
> >>  
> >>  #include <linux/sys_soc.h>
> >> 
> >> +#include <linux/sort.h>
> >> +#include <linux/of.h>
> > 
> > Please keep this list alphabetically sorted.
> 
> OK.
> 
> >>  #include <drm/drm_atomic.h>
> >>  #include <drm/drm_atomic_helper.h>
> >> 
> >> @@ -162,6 +164,22 @@ 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;
> >> +	int  id1, id2;
> >> +
> >> +	id1 = of_alias_get_id(dssdev1->dev->of_node, "display");
> >> +	id2 = of_alias_get_id(dssdev2->dev->of_node, "display");
> > 
> > Getting the alias id is a bit costly, how about caching them ?
> 
> This is going to be done once and the ID will be never used after this.
> We could add the ID to 'struct omap_dss_device' and initialize it in
> omapdss_register_display, if that's what you are referring to.

Yes, that's what I meant.

-- 
Regards,

Laurent Pinchart

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

[RFC 6/7] 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 |  3 ---
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/display.c b/drivers/gpu/drm/omapdrm/dss/display.c
index 42279933790e..0ce18edf6191 100644
--- a/drivers/gpu/drm/omapdrm/dss/display.c
+++ b/drivers/gpu/drm/omapdrm/dss/display.c
@@ -44,7 +44,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;
 
 	/*
@@ -55,26 +54,18 @@ int omapdss_register_display(struct omap_dss_device *dssdev)
 	if (id < 0)
 		id = disp_num_counter++;
 
-	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 47a331670963..b206aa092235 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -475,9 +475,6 @@ struct omap_dss_device {
 
 	struct list_head panel_list;
 
-	/* alias in the form of "display%d" */
-	char alias[16];
-
 	enum omap_display_type type;
 	enum omap_display_type output_type;
 
-- 
2.14.1


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

[RFC 7/7] 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 0e100a359d26..ce260c48d686 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -36,6 +36,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
  */
@@ -183,12 +191,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;
@@ -196,8 +213,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)
-- 
2.14.1


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: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Laurent Pinchart]

Hi Peter,

On Tuesday, 29 August 2017 10:32:11 EEST Peter Ujfalusi wrote:
> Hi
> 
> 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.

One could argue that the DT should then be updated. The device tree is a 
description of the whole system, not just the board. If a board is integrated 
in a system that makes HDMI the primary display, it would make sense for DT to 
reflect that.

> 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.

This will conflict with the work I'm doing on merging the omapdrm and omapdss 
driver, so I'm a bit reluctant to merge this first :-/ In particular, with the 
two drivers merged, couldn't we implement this module parameter without moving 
the display sorting from omapdss to omapdrm ?

> ---
> Peter Ujfalusi (7):
>   drm/omap: Use devm_kzalloc() to allocate omap_drm_private
>   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    | 244 +++++++++++++++++++++----------
>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
>  4 files changed, 183 insertions(+), 82 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Peter Ujfalusi]

Hi Laurent,


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

On 2017-09-01 14:36, Laurent Pinchart wrote:
>> 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.
> 
> One could argue that the DT should then be updated. The device tree is a 
> description of the whole system, not just the board. If a board is integrated 
> in a system that makes HDMI the primary display, it would make sense for DT to 
> reflect that.

Yes, in case when the device is prepared for a use case this can be done
with recompiling the DT, but when you have a device which uses the LCD
as primary display by design and move that to connect it to a HDMI
TV/monitor and want to use it there for a prolonged time, you might not
want to set up a development environment just to recompile the DT. In
this case you just add the kernel parameter and be done with the
adaptation to the new use case.

>> 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.
> 
> This will conflict with the work I'm doing on merging the omapdrm and omapdss 
> driver, so I'm a bit reluctant to merge this first :-/

Understand. I will update the patches based on the comments and roll it
in my wip branch for now and going to send v1 when the omapdrm is
ompadss is merged?

> In particular, with the 
> two drivers merged, couldn't we implement this module parameter without moving 
> the display sorting from omapdss to omapdrm ?

If they are merged, then we will only have omapdss ;)

I wanted to have all sorting in one place so it is going to be easier to
locate them and since they are in one place it might make easier to
merge the the omapdss to omapdrm. Or not.

> 
>> ---
>> Peter Ujfalusi (7):
>>   drm/omap: Use devm_kzalloc() to allocate omap_drm_private
>>   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    | 244 +++++++++++++++++++++----------
>>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
>>  4 files changed, 183 insertions(+), 82 deletions(-)
> 

- Péter

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

Re: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Laurent Pinchart]

Hi Peter,

On Monday, 4 September 2017 13:03:36 EEST Peter Ujfalusi wrote:

I should drop dates when I reply to such old e-mails...

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> On 2017-09-01 14:36, Laurent Pinchart wrote:
> >> 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.
> > 
> > One could argue that the DT should then be updated. The device tree is a
> > description of the whole system, not just the board. If a board is
> > integrated in a system that makes HDMI the primary display, it would make
> > sense for DT to reflect that.
> 
> Yes, in case when the device is prepared for a use case this can be done
> with recompiling the DT, but when you have a device which uses the LCD
> as primary display by design and move that to connect it to a HDMI
> TV/monitor and want to use it there for a prolonged time, you might not
> want to set up a development environment just to recompile the DT. In
> this case you just add the kernel parameter and be done with the
> adaptation to the new use case.

One could also argue that in that case it would be better to handle that in 
userspace, as it would be more user-friendly than having to change the kernel 
command line.

(What, does it show that I'm trying to push features out of the kernel ? :-))

> >> 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.
> > 
> > This will conflict with the work I'm doing on merging the omapdrm and
> > omapdss driver, so I'm a bit reluctant to merge this first :-/
> 
> Understand. I will update the patches based on the comments and roll it
> in my wip branch for now and going to send v1 when the omapdrm is
> ompadss is merged?

I'll reply to the v2 of your patch series to address this.

> > In particular, with the two drivers merged, couldn't we implement this
> > module parameter without moving the display sorting from omapdss to
> > omapdrm ?
> 
> If they are merged, then we will only have omapdss ;)
> 
> I wanted to have all sorting in one place so it is going to be easier to
> locate them and since they are in one place it might make easier to
> merge the the omapdss to omapdrm. Or not.
> 
> >> ---
> >> 
> >> Peter Ujfalusi (7):
> >>   drm/omap: Use devm_kzalloc() to allocate omap_drm_private
> >>   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    | 244 +++++++++++++++++++---------
> >>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
> >>  4 files changed, 183 insertions(+), 82 deletions(-)

-- 
Regards,

Laurent Pinchart



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

Re: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Pekka Paalanen]

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

On Tue, 29 Aug 2017 10:32:11 +0300
Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> Hi
> 
> 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.

Hi,

that's because Weston does not have a concept of main or preferred
display to begin with.

If what you refer to involves running Weston with the fbdev-backend,
then Weston has nothing to do with the issue. Weston only
uses /dev/fb0, whatever that might be and however that might work, as
set up by the kernel.


Thanks,
pq

> 
> 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 (7):
>   drm/omap: Use devm_kzalloc() to allocate omap_drm_private
>   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    | 244 ++++++++++++++++++++++++----------
>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +
>  4 files changed, 183 insertions(+), 82 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: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Tomi Valkeinen]

Hi,


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

On 05/10/17 12:56, Pekka Paalanen wrote:

> that's because Weston does not have a concept of main or preferred
> display to begin with.
> 
> If what you refer to involves running Weston with the fbdev-backend,
> then Weston has nothing to do with the issue. Weston only
> uses /dev/fb0, whatever that might be and however that might work, as
> set up by the kernel.

No, it's with DRM backend.

If I recall right, the problem is that when you open a window, it opens
on the first display (first DRM connector). Or (again, if I recall
right), if the mouse pointer is on a particular display, then the window
opens there. So no means to say "run this application and put its
windows on the HDMI display".

 Tomi

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

Re: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Pekka Paalanen]

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

On Thu, 5 Oct 2017 13:01:50 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> Hi,
> 
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 05/10/17 12:56, Pekka Paalanen wrote:
> 
> > that's because Weston does not have a concept of main or preferred
> > display to begin with.
> > 
> > If what you refer to involves running Weston with the fbdev-backend,
> > then Weston has nothing to do with the issue. Weston only
> > uses /dev/fb0, whatever that might be and however that might work, as
> > set up by the kernel.  
> 
> No, it's with DRM backend.
> 
> If I recall right, the problem is that when you open a window, it opens
> on the first display (first DRM connector). Or (again, if I recall
> right), if the mouse pointer is on a particular display, then the window
> opens there. So no means to say "run this application and put its
> windows on the HDMI display".

Hi Tomi,

in Weston, the initial window placement is essentially random. There is
some guessing based on the current pointer location, but as there could
be several pointers, that's not reliable either. When there is no
pointer to guess from and Weston needs to pick an output, it simply
picks the first in a list. We might as well randomize it too, since all
outputs are equal in the current design. Only luck guarantees the order
in the output list.

If an application wants to show up on a specific output, there is
currently no Wayland extension that I recall to allow that, aside from
IVI-shell/LayerManager infrastructure. If you really need something on
a specific output, one way is to write a new protocol extension that
suits your use case, especially if your use case is not a normal PC
desktop.

If your use case is a normal PC desktop, then you could participate in
the xdg-shell protocol suite development to get a desktop standard for
initial window placement. That would probably be preceded by designing
a protocol extension that would let clients meaningfully choose an
initial output to begin with.

There is also the option of writing your own shell plugin to Weston, to
give you exactly the window management behaviour you want, including
the choice of the output.

The very least, there was some work towards configuring the output
layout in weston.ini that is still unfinished, which might perhaps
provide a workaround similar to changing connector ordering but with
only half the luck required. There is already a way to turn a connector
off in weston.ini, and my current work is aiming to allow
force-enabling disconnected connectors as well.

Changing connector ordering in the kernel to get Weston to do something
it has never deliberately intended in the first place is just wrong, so
please do not use Weston as a justification for this kernel module
parameter.


Thanks,
pq

[-- 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: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Tomi Valkeinen]


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

On 05/10/17 13:43, Pekka Paalanen wrote:

> in Weston, the initial window placement is essentially random. There is
> some guessing based on the current pointer location, but as there could
> be several pointers, that's not reliable either. When there is no
> pointer to guess from and Weston needs to pick an output, it simply
> picks the first in a list. We might as well randomize it too, since all
> outputs are equal in the current design. Only luck guarantees the order
> in the output list.

Please don't make our lives harder, just because =). As you explain
above, it is not random at the moment.

I understand relying on the above logic is not very good, and it can
change in the future, but as you explain below, we're not in a position
where we have a good way to deal with the use case at the moment.

> If an application wants to show up on a specific output, there is
> currently no Wayland extension that I recall to allow that, aside from
> IVI-shell/LayerManager infrastructure. If you really need something on
> a specific output, one way is to write a new protocol extension that
> suits your use case, especially if your use case is not a normal PC
> desktop.

Yes, we really need something on a specific user-defined output. One of
the use cases is a board with an LCD with touchscreen and an HDMI. No
mouse. So the pointer is never on the HDMI. We may want to launch a
"view-only" application shown on the HDMI.

Or, we may have a mouse, and use the HDMI as the "main screen", which
means that our launcher application is shown on the HDMI instead of the LCD.

Or we may not have any mouse/touchscreen at all (although if I recall
right, this needs a hack in weston to get it start), and we want to
launch applications on specific screen.

Just a few examples.

And yes, if you create a real product, perhaps IVI-shell or such is the
way to go. Our use is more of a development/demo cases, although I don't
see why they would not be usable in a real product too.

I think just having an env variable, which gives a hint to weston about
on which display the window should be openend, would cover our use cases.

> If your use case is a normal PC desktop, then you could participate in
> the xdg-shell protocol suite development to get a desktop standard for
> initial window placement. That would probably be preceded by designing
> a protocol extension that would let clients meaningfully choose an
> initial output to begin with.
> 
> There is also the option of writing your own shell plugin to Weston, to
> give you exactly the window management behaviour you want, including
> the choice of the output.
> 
> The very least, there was some work towards configuring the output
> layout in weston.ini that is still unfinished, which might perhaps
> provide a workaround similar to changing connector ordering but with
> only half the luck required. There is already a way to turn a connector
> off in weston.ini, and my current work is aiming to allow
> force-enabling disconnected connectors as well.
> 
> Changing connector ordering in the kernel to get Weston to do something
> it has never deliberately intended in the first place is just wrong, so
> please do not use Weston as a justification for this kernel module
> parameter.

Weston was just one piece of the puzzle. If it was just Weston, I guess
we'd have looked at patching Weston instead of the kernel.

As the cover letter says, it's also about the fb emulation and custom
DRM apps. It seems quite common for these applications to just pick the
first connector. And the series also gives features for debugging,
testing, and disabling displays altogether.

The series is an RFC, and kind of feels like a hack. I think the order
of the DRM connectors should be considered random, in a perfect world.
Still, this series fixes real issues without the need to go fix every
broken/not-finished legacy and non-legacy app out there that uses DRM,
and gives us some debug/test functionality.

 Tomi

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

Re: [RFC 0/7] drm/omap: Module parameter for display order configuration

[Pekka Paalanen]

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

On Thu, 5 Oct 2017 14:24:27 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 05/10/17 13:43, Pekka Paalanen wrote:
> 
> > in Weston, the initial window placement is essentially random. There is
> > some guessing based on the current pointer location, but as there could
> > be several pointers, that's not reliable either. When there is no
> > pointer to guess from and Weston needs to pick an output, it simply
> > picks the first in a list. We might as well randomize it too, since all
> > outputs are equal in the current design. Only luck guarantees the order
> > in the output list.  
> 
> Please don't make our lives harder, just because =). As you explain
> above, it is not random at the moment.
> 
> I understand relying on the above logic is not very good, and it can
> change in the future, but as you explain below, we're not in a position
> where we have a good way to deal with the use case at the moment.

As long as you are aware it can break any time. *I* won't break it just
for the sake of it. :-)

But I also don't intend to make sure the behaviour stays the same.

> > If an application wants to show up on a specific output, there is
> > currently no Wayland extension that I recall to allow that, aside from
> > IVI-shell/LayerManager infrastructure. If you really need something on
> > a specific output, one way is to write a new protocol extension that
> > suits your use case, especially if your use case is not a normal PC
> > desktop.  
> 
> Yes, we really need something on a specific user-defined output. One of
> the use cases is a board with an LCD with touchscreen and an HDMI. No
> mouse. So the pointer is never on the HDMI. We may want to launch a
> "view-only" application shown on the HDMI.
> 
> Or, we may have a mouse, and use the HDMI as the "main screen", which
> means that our launcher application is shown on the HDMI instead of the LCD.
> 
> Or we may not have any mouse/touchscreen at all (although if I recall
> right, this needs a hack in weston to get it start), and we want to
> launch applications on specific screen.
> 
> Just a few examples.

Right, all those point in the direction of a custom shell plugin where
you can have your window management rules. Trying to drive window
management from outside a Wayland compositor is not a good idea...

> And yes, if you create a real product, perhaps IVI-shell or such is the
> way to go. Our use is more of a development/demo cases, although I don't
> see why they would not be usable in a real product too.

..which is why I never liked the IVI LayerManager etc. approach much.

IVI-shell is really just a wrapper that still wants you to write a
plugin to do the actual window management. On the application facing
side it removes all the desktop windowing features and literally just
offers you a window ID number to base window management on. No
provision for clients to tell the output, aside from what you can imply
with an ID.

> I think just having an env variable, which gives a hint to weston about
> on which display the window should be openend, would cover our use cases.

That is a very special environment where one could expect that to work.

That would be possible given:
- a protocol extension to relay the wanted output
- client toolkit patch to read the env var and use the protocol
  extension
- a new weston shell plugin to implement the protocol extension

If one were to write a special protocol extension just for your use
case, the extension could be dead simple.

There is another plugin in Weston, fullscreen-shell, but it is in a bit
bad shape and only supports a single client at a time, but allows
explicit control of outputs. It has no window management as it has no
windows: you associate a wl_surface for an output, and that's it. If
you needed windows, you could use sub-surfaces.

> > If your use case is a normal PC desktop, then you could participate in
> > the xdg-shell protocol suite development to get a desktop standard for
> > initial window placement. That would probably be preceded by designing
> > a protocol extension that would let clients meaningfully choose an
> > initial output to begin with.
> > 
> > There is also the option of writing your own shell plugin to Weston, to
> > give you exactly the window management behaviour you want, including
> > the choice of the output.
> > 
> > The very least, there was some work towards configuring the output
> > layout in weston.ini that is still unfinished, which might perhaps
> > provide a workaround similar to changing connector ordering but with
> > only half the luck required. There is already a way to turn a connector
> > off in weston.ini, and my current work is aiming to allow
> > force-enabling disconnected connectors as well.
> > 
> > Changing connector ordering in the kernel to get Weston to do something
> > it has never deliberately intended in the first place is just wrong, so
> > please do not use Weston as a justification for this kernel module
> > parameter.  
> 
> Weston was just one piece of the puzzle. If it was just Weston, I guess
> we'd have looked at patching Weston instead of the kernel.
> 
> As the cover letter says, it's also about the fb emulation and custom
> DRM apps. It seems quite common for these applications to just pick the
> first connector. And the series also gives features for debugging,
> testing, and disabling displays altogether.
> 
> The series is an RFC, and kind of feels like a hack. I think the order
> of the DRM connectors should be considered random, in a perfect world.
> Still, this series fixes real issues without the need to go fix every
> broken/not-finished legacy and non-legacy app out there that uses DRM,
> and gives us some debug/test functionality.

True. And then you will be stuck with it.

It's also hard to care about fbdev.

My point was not to NAK this series. My point was to remove Weston from
the justification, that is all.


Thanks,
pq

[-- 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