[PATCH 00/24] drm/omap: misc patches

[PATCH 00/24] drm/omap: misc patches

[Tomi Valkeinen]

Hi,

Here's a bunch of misc patches I forward ported from TI's kernel. I think some
of the patches have already been sent for review earlier (Peter's), but I've
included them all here.

 Tomi

Benoit Parrot (2):
  drm/omap: dispc: disp_wb_setup to check return code
  drm/omap: Add pclk setting case when channel is DSS_WB

Jyri Sarha (1):
  drm/omap: Allow HDMI audio setup even if we do not have video
    configured

Peter Ujfalusi (8):
  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
  drm/omap: Init fbdev emulation only when we have displays

Tomi Valkeinen (13):
  drm/omap: fix omap_fbdev_free() when omap_fbdev_create() wasn't called
  drm/omap: cleanup fbdev init/free
  drm/omap: add HPD support to connector-dvi
  dt-bindings: display: add HPD gpio to DVI connector
  drm/omap: remove leftover enums
  drm/omap: set WB channel-in in wb_setup()
  drm/omap: fix WBDELAYCOUNT for HDMI
  drm/omap: fix WBDELAYCOUNT with interlace
  drm/omap: fix WB height with interlace
  drm/omap: fix scaling limits for WB
  drm/omap: add writeback funcs to dispc_ops
  drm/omap: fix maximum sizes
  drm/omap: cleanup color space conversion

 .../bindings/display/connector/dvi-connector.txt   |   1 +
 drivers/gpu/drm/omapdrm/displays/connector-dvi.c   | 114 +++++++++
 drivers/gpu/drm/omapdrm/dss/dispc.c                | 161 +++++++++----
 drivers/gpu/drm/omapdrm/dss/display.c              |  15 +-
 drivers/gpu/drm/omapdrm/dss/dss.h                  |  18 --
 drivers/gpu/drm/omapdrm/dss/hdmi4.c                |  33 ++-
 drivers/gpu/drm/omapdrm/dss/hdmi5.c                |  37 ++-
 drivers/gpu/drm/omapdrm/dss/omapdss.h              |  37 +--
 drivers/gpu/drm/omapdrm/omap_drv.c                 | 260 +++++++++++++++------
 drivers/gpu/drm/omapdrm/omap_drv.h                 |   3 +
 drivers/gpu/drm/omapdrm/omap_fbdev.c               |  16 +-
 drivers/gpu/drm/omapdrm/omap_fbdev.h               |   5 +-
 12 files changed, 483 insertions(+), 217 deletions(-)

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

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

[PATCH 01/24] drm/omap: fix omap_fbdev_free() when omap_fbdev_create() wasn't called

[Tomi Valkeinen]

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 1ace63e2ff22..632ebcf2165f 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -303,7 +303,8 @@ void omap_fbdev_free(struct drm_device *dev)
 	fbdev = to_omap_fbdev(priv->fbdev);
 
 	/* unpin the GEM object pinned in omap_fbdev_create() */
-	omap_gem_unpin(fbdev->bo);
+	if (fbdev->bo)
+		omap_gem_unpin(fbdev->bo);
 
 	/* this will free the backing object */
 	if (fbdev->fb)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 01/24] drm/omap: fix omap_fbdev_free() when omap_fbdev_create() wasn't called

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:31 EET Tomi Valkeinen wrote:
> If we have no crtcs/connectors, fbdev init goes fine, but
> omap_fbdev_create() is never called. This means that omap_fbdev->bo is
> NULL and omap_fbdev_free() crashes.
> 
> Add a check to omap_fbdev_free() to handle the NULL case.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

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

> ---
>  drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 1ace63e2ff22..632ebcf2165f
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -303,7 +303,8 @@ void omap_fbdev_free(struct drm_device *dev)
>  	fbdev = to_omap_fbdev(priv->fbdev);
> 
>  	/* unpin the GEM object pinned in omap_fbdev_create() */
> -	omap_gem_unpin(fbdev->bo);
> +	if (fbdev->bo)
> +		omap_gem_unpin(fbdev->bo);
> 
>  	/* this will free the backing object */
>  	if (fbdev->fb)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 01/24] drm/omap: fix omap_fbdev_free() when omap_fbdev_create() wasn't called

[Tomi Valkeinen]

On 27/02/18 13:28, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Monday, 12 February 2018 11:44:31 EET Tomi Valkeinen wrote:
>> If we have no crtcs/connectors, fbdev init goes fine, but
>> omap_fbdev_create() is never called. This means that omap_fbdev->bo is
>> NULL and omap_fbdev_free() crashes.
>>
>> Add a check to omap_fbdev_free() to handle the NULL case.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> I wonder whether we shouldn't fail probe if we have no CRTC or connector, but 
> regardless of that, this change looks good to me.

We can use the writeback without any displays.

 Tomi

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

[PATCH 02/24] drm/omap: cleanup fbdev init/free

[Tomi Valkeinen]

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c   |  9 ++++-----
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++-----
 drivers/gpu/drm/omapdrm/omap_fbdev.h |  5 ++---
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index dd68b2556f5b..485684c637ff 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev)
 	for (i = 0; i < priv->num_crtcs; i++)
 		drm_crtc_vblank_off(priv->crtcs[i]);
 
-	priv->fbdev = omap_fbdev_init(ddev);
+	omap_fbdev_init(ddev);
 
 	drm_kms_helper_poll_init(ddev);
 	omap_modeset_enable_external_hpd();
@@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev)
 err_cleanup_helpers:
 	omap_modeset_disable_external_hpd();
 	drm_kms_helper_poll_fini(ddev);
-	if (priv->fbdev)
-		omap_fbdev_free(ddev);
+
+	omap_fbdev_free(ddev);
 err_cleanup_modeset:
 	drm_mode_config_cleanup(ddev);
 	omap_drm_irq_uninstall(ddev);
@@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev)
 	omap_modeset_disable_external_hpd();
 	drm_kms_helper_poll_fini(ddev);
 
-	if (priv->fbdev)
-		omap_fbdev_free(ddev);
+	omap_fbdev_free(ddev);
 
 	drm_atomic_helper_shutdown(ddev);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 632ebcf2165f..30ce3d562414 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
 }
 
 /* initialize fbdev helper */
-struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
+void omap_fbdev_init(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_fbdev *fbdev = NULL;
@@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
 
 	priv->fbdev = helper;
 
-	return helper;
+	return;
 
 fini:
 	drm_fb_helper_fini(helper);
@@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
 	kfree(fbdev);
 
 	dev_warn(dev->dev, "omap_fbdev_init failed\n");
-	/* well, limp along without an fbdev.. maybe X11 will work? */
-
-	return NULL;
 }
 
 void omap_fbdev_free(struct drm_device *dev)
@@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev)
 
 	DBG();
 
+	if (!priv->fbdev)
+		return;
+
 	drm_fb_helper_unregister_fbi(helper);
 
 	drm_fb_helper_fini(helper);
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h b/drivers/gpu/drm/omapdrm/omap_fbdev.h
index 1f5ba0996a1a..3bd6ea661a18 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.h
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h
@@ -24,12 +24,11 @@ struct drm_device;
 struct drm_fb_helper;
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev);
+void omap_fbdev_init(struct drm_device *dev);
 void omap_fbdev_free(struct drm_device *dev);
 #else
-static inline struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
+static inline void omap_fbdev_init(struct drm_device *dev)
 {
-	return NULL;
 }
 static inline void omap_fbdev_free(struct drm_device *dev)
 {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 02/24] drm/omap: cleanup fbdev init/free

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:32 EET Tomi Valkeinen wrote:
> omap_fbdev_init() and omap_fbdev_free() use priv->fbdev directly.
> However, omap_fbdev_init() returns the fbdev, and omap_drv.c also
> assigns the return value to priv->fbdev. This is slightly confusing.
> 
> Clean this up by removing the omap_fbdev_init() return value, as we
> don't care whether fbdev init succeeded or not. Also change omap_drv.c
> to call omap_fbdev_init() always, and omap_fbdev_init() does the check
> if fbdev was initialized.

I assume you mean omap_fbdev_free() for the last two occurrences (and on a 
side note I wonder whether we should rename it to omap_fbdev_cleanup() or 
omap_fbdev_fini() for symmetry with omap_fbdev_init()).

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c   |  9 ++++-----
>  drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++-----
>  drivers/gpu/drm/omapdrm/omap_fbdev.h |  5 ++---
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b2556f5b..485684c637ff
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	for (i = 0; i < priv->num_crtcs; i++)
>  		drm_crtc_vblank_off(priv->crtcs[i]);
> 
> -	priv->fbdev = omap_fbdev_init(ddev);
> +	omap_fbdev_init(ddev);
> 
>  	drm_kms_helper_poll_init(ddev);
>  	omap_modeset_enable_external_hpd();
> @@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev)
>  err_cleanup_helpers:
>  	omap_modeset_disable_external_hpd();
>  	drm_kms_helper_poll_fini(ddev);
> -	if (priv->fbdev)
> -		omap_fbdev_free(ddev);
> +
> +	omap_fbdev_free(ddev);
>  err_cleanup_modeset:
>  	drm_mode_config_cleanup(ddev);
>  	omap_drm_irq_uninstall(ddev);
> @@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev)
>  	omap_modeset_disable_external_hpd();
>  	drm_kms_helper_poll_fini(ddev);
> 
> -	if (priv->fbdev)
> -		omap_fbdev_free(ddev);
> +	omap_fbdev_free(ddev);
> 
>  	drm_atomic_helper_shutdown(ddev);
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 632ebcf2165f..30ce3d562414
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
> }
> 
>  /* initialize fbdev helper */
> -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
> +void omap_fbdev_init(struct drm_device *dev)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
>  	struct omap_fbdev *fbdev = NULL;
> @@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device
> *dev)
> 
>  	priv->fbdev = helper;
> 
> -	return helper;
> +	return;
> 
>  fini:
>  	drm_fb_helper_fini(helper);
> @@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device
> *dev) kfree(fbdev);
> 
>  	dev_warn(dev->dev, "omap_fbdev_init failed\n");

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

> -	/* well, limp along without an fbdev.. maybe X11 will work? */
> -
> -	return NULL;
>  }
> 
>  void omap_fbdev_free(struct drm_device *dev)
> @@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev)
> 
>  	DBG();
> 
> +	if (!priv->fbdev)
> +		return;
> +

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

>  	drm_fb_helper_unregister_fbi(helper);
> 
>  	drm_fb_helper_fini(helper);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h
> b/drivers/gpu/drm/omapdrm/omap_fbdev.h index 1f5ba0996a1a..3bd6ea661a18
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.h
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h
> @@ -24,12 +24,11 @@ struct drm_device;
>  struct drm_fb_helper;
> 
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev);
> +void omap_fbdev_init(struct drm_device *dev);
>  void omap_fbdev_free(struct drm_device *dev);
>  #else
> -static inline struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
> +static inline void omap_fbdev_init(struct drm_device *dev)
>  {
> -	return NULL;
>  }
>  static inline void omap_fbdev_free(struct drm_device *dev)
>  {

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 02/24] drm/omap: cleanup fbdev init/free

[Tomi Valkeinen]

On 27/02/18 13:38, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Monday, 12 February 2018 11:44:32 EET Tomi Valkeinen wrote:
>> omap_fbdev_init() and omap_fbdev_free() use priv->fbdev directly.
>> However, omap_fbdev_init() returns the fbdev, and omap_drv.c also
>> assigns the return value to priv->fbdev. This is slightly confusing.
>>
>> Clean this up by removing the omap_fbdev_init() return value, as we
>> don't care whether fbdev init succeeded or not. Also change omap_drv.c
>> to call omap_fbdev_init() always, and omap_fbdev_init() does the check
>> if fbdev was initialized.
> 
> I assume you mean omap_fbdev_free() for the last two occurrences (and on a 

Right, true.

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

Yep, I think that makes sense.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_drv.c   |  9 ++++-----
>>  drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++-----
>>  drivers/gpu/drm/omapdrm/omap_fbdev.h |  5 ++---
>>  3 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b2556f5b..485684c637ff
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev)
>>  	for (i = 0; i < priv->num_crtcs; i++)
>>  		drm_crtc_vblank_off(priv->crtcs[i]);
>>
>> -	priv->fbdev = omap_fbdev_init(ddev);
>> +	omap_fbdev_init(ddev);
>>
>>  	drm_kms_helper_poll_init(ddev);
>>  	omap_modeset_enable_external_hpd();
>> @@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev)
>>  err_cleanup_helpers:
>>  	omap_modeset_disable_external_hpd();
>>  	drm_kms_helper_poll_fini(ddev);
>> -	if (priv->fbdev)
>> -		omap_fbdev_free(ddev);
>> +
>> +	omap_fbdev_free(ddev);
>>  err_cleanup_modeset:
>>  	drm_mode_config_cleanup(ddev);
>>  	omap_drm_irq_uninstall(ddev);
>> @@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev)
>>  	omap_modeset_disable_external_hpd();
>>  	drm_kms_helper_poll_fini(ddev);
>>
>> -	if (priv->fbdev)
>> -		omap_fbdev_free(ddev);
>> +	omap_fbdev_free(ddev);
>>
>>  	drm_atomic_helper_shutdown(ddev);
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 632ebcf2165f..30ce3d562414
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
>> }
>>
>>  /* initialize fbdev helper */
>> -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev)
>> +void omap_fbdev_init(struct drm_device *dev)
>>  {
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	struct omap_fbdev *fbdev = NULL;
>> @@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device
>> *dev)
>>
>>  	priv->fbdev = helper;
>>
>> -	return helper;
>> +	return;
>>
>>  fini:
>>  	drm_fb_helper_fini(helper);
>> @@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device
>> *dev) kfree(fbdev);
>>
>>  	dev_warn(dev->dev, "omap_fbdev_init failed\n");
> 
> While at it I'd remove the error message when drm_fb_helper_init() fails, it 
> duplicates this one.

Ok.

>> -	/* well, limp along without an fbdev.. maybe X11 will work? */
>> -
>> -	return NULL;
>>  }
>>
>>  void omap_fbdev_free(struct drm_device *dev)
>> @@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev)
>>
>>  	DBG();
>>
>> +	if (!priv->fbdev)
>> +		return;
>> +
> 
> I'd either write this as if (!helper) and replace the other occurrences of 
> priv->fbdev below, or replace helper with priv->fbdev in the whole function.

Ok.

 Tomi

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

[PATCH 03/24] drm/omap: add HPD support to connector-dvi

[Tomi Valkeinen]

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 114 +++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
index 391d80364346..f9f8700223c3 100644
--- a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
+++ b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/gpio/consumer.h>
 
 #include <drm/drm_edid.h>
 
@@ -44,6 +45,13 @@ struct panel_drv_data {
 	struct videomode vm;
 
 	struct i2c_adapter *i2c_adapter;
+
+	struct gpio_desc *hpd_gpio;
+
+	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
+	void *hpd_cb_data;
+	bool hpd_enabled;
+	struct mutex hpd_lock;
 };
 
 #define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
@@ -189,6 +197,9 @@ static int dvic_read_edid(struct omap_dss_device *dssdev,
 	struct panel_drv_data *ddata = to_panel_data(dssdev);
 	int r, l, bytes_read;
 
+	if (ddata->hpd_gpio && !gpiod_get_value_cansleep(ddata->hpd_gpio))
+		return -ENODEV;
+
 	if (!ddata->i2c_adapter)
 		return -ENODEV;
 
@@ -220,6 +231,9 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
 	unsigned char out;
 	int r;
 
+	if (ddata->hpd_gpio)
+		return gpiod_get_value_cansleep(ddata->hpd_gpio);
+
 	if (!ddata->i2c_adapter)
 		return true;
 
@@ -228,6 +242,60 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
 	return r == 0;
 }
 
+static int dvic_register_hpd_cb(struct omap_dss_device *dssdev,
+				 void (*cb)(void *cb_data,
+					    enum drm_connector_status status),
+				 void *cb_data)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	if (!ddata->hpd_gpio)
+		return -ENOTSUPP;
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_cb = cb;
+	ddata->hpd_cb_data = cb_data;
+	mutex_unlock(&ddata->hpd_lock);
+	return 0;
+}
+
+static void dvic_unregister_hpd_cb(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	if (!ddata->hpd_gpio)
+		return;
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_cb = NULL;
+	ddata->hpd_cb_data = NULL;
+	mutex_unlock(&ddata->hpd_lock);
+}
+
+static void dvic_enable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	if (!ddata->hpd_gpio)
+		return;
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_enabled = true;
+	mutex_unlock(&ddata->hpd_lock);
+}
+
+static void dvic_disable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	if (!ddata->hpd_gpio)
+		return;
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_enabled = false;
+	mutex_unlock(&ddata->hpd_lock);
+}
+
 static struct omap_dss_driver dvic_driver = {
 	.connect	= dvic_connect,
 	.disconnect	= dvic_disconnect,
@@ -241,14 +309,60 @@ static struct omap_dss_driver dvic_driver = {
 
 	.read_edid	= dvic_read_edid,
 	.detect		= dvic_detect,
+
+	.register_hpd_cb	= dvic_register_hpd_cb,
+	.unregister_hpd_cb	= dvic_unregister_hpd_cb,
+	.enable_hpd		= dvic_enable_hpd,
+	.disable_hpd		= dvic_disable_hpd,
 };
 
+static irqreturn_t dvic_hpd_isr(int irq, void *data)
+{
+	struct panel_drv_data *ddata = data;
+
+	mutex_lock(&ddata->hpd_lock);
+	if (ddata->hpd_enabled && ddata->hpd_cb) {
+		enum drm_connector_status status;
+
+		if (dvic_detect(&ddata->dssdev))
+			status = connector_status_connected;
+		else
+			status = connector_status_disconnected;
+
+		ddata->hpd_cb(ddata->hpd_cb_data, status);
+	}
+	mutex_unlock(&ddata->hpd_lock);
+
+	return IRQ_HANDLED;
+}
+
 static int dvic_probe_of(struct platform_device *pdev)
 {
 	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *adapter_node;
 	struct i2c_adapter *adapter;
+	struct gpio_desc *gpio;
+	int r;
+
+	gpio = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
+	if (IS_ERR(gpio)) {
+		dev_err(&pdev->dev, "failed to parse HPD gpio\n");
+		return PTR_ERR(gpio);
+	}
+
+	ddata->hpd_gpio = gpio;
+
+	mutex_init(&ddata->hpd_lock);
+
+	if (ddata->hpd_gpio) {
+		r = devm_request_threaded_irq(&pdev->dev,
+			gpiod_to_irq(ddata->hpd_gpio), NULL, dvic_hpd_isr,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			"DVI HPD", ddata);
+		if (r)
+			return r;
+	}
 
 	adapter_node = of_parse_phandle(node, "ddc-i2c-bus", 0);
 	if (adapter_node) {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 03/24] drm/omap: add HPD support to connector-dvi

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:33 EET Tomi Valkeinen wrote:
> Add HPD support to the DVI connector driver. The code is almost
> identical to the HPD code in the HDMI connector driver.

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

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 114 ++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
> b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c index
> 391d80364346..f9f8700223c3 100644
> --- a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
> +++ b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/gpio/consumer.h>

Could you please keep headers alphabetically sorted ?

>  #include <drm/drm_edid.h>
> 
> @@ -44,6 +45,13 @@ struct panel_drv_data {
>  	struct videomode vm;
> 
>  	struct i2c_adapter *i2c_adapter;
> +
> +	struct gpio_desc *hpd_gpio;
> +
> +	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
> +	void *hpd_cb_data;
> +	bool hpd_enabled;
> +	struct mutex hpd_lock;

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

>  };
> 
>  #define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
> @@ -189,6 +197,9 @@ static int dvic_read_edid(struct omap_dss_device
> *dssdev, struct panel_drv_data *ddata = to_panel_data(dssdev);
>  	int r, l, bytes_read;
> 
> +	if (ddata->hpd_gpio && !gpiod_get_value_cansleep(ddata->hpd_gpio))
> +		return -ENODEV;
> +
>  	if (!ddata->i2c_adapter)
>  		return -ENODEV;
> 
> @@ -220,6 +231,9 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
>  	unsigned char out;
>  	int r;
> 
> +	if (ddata->hpd_gpio)
> +		return gpiod_get_value_cansleep(ddata->hpd_gpio);
> +
>  	if (!ddata->i2c_adapter)
>  		return true;
> 
> @@ -228,6 +242,60 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
> return r == 0;
>  }
> 
> +static int dvic_register_hpd_cb(struct omap_dss_device *dssdev,
> +				 void (*cb)(void *cb_data,
> +					    enum drm_connector_status status),
> +				 void *cb_data)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return -ENOTSUPP;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_cb = cb;
> +	ddata->hpd_cb_data = cb_data;
> +	mutex_unlock(&ddata->hpd_lock);
> +	return 0;
> +}
> +
> +static void dvic_unregister_hpd_cb(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_cb = NULL;
> +	ddata->hpd_cb_data = NULL;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
> +static void dvic_enable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_enabled = true;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
> +static void dvic_disable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_enabled = false;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
>  static struct omap_dss_driver dvic_driver = {
>  	.connect	= dvic_connect,
>  	.disconnect	= dvic_disconnect,
> @@ -241,14 +309,60 @@ static struct omap_dss_driver dvic_driver = {
> 
>  	.read_edid	= dvic_read_edid,
>  	.detect		= dvic_detect,
> +
> +	.register_hpd_cb	= dvic_register_hpd_cb,
> +	.unregister_hpd_cb	= dvic_unregister_hpd_cb,
> +	.enable_hpd		= dvic_enable_hpd,
> +	.disable_hpd		= dvic_disable_hpd,
>  };
> 
> +static irqreturn_t dvic_hpd_isr(int irq, void *data)
> +{
> +	struct panel_drv_data *ddata = data;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	if (ddata->hpd_enabled && ddata->hpd_cb) {
> +		enum drm_connector_status status;
> +
> +		if (dvic_detect(&ddata->dssdev))
> +			status = connector_status_connected;
> +		else
> +			status = connector_status_disconnected;
> +
> +		ddata->hpd_cb(ddata->hpd_cb_data, status);
> +	}
> +	mutex_unlock(&ddata->hpd_lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int dvic_probe_of(struct platform_device *pdev)
>  {
>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>  	struct device_node *node = pdev->dev.of_node;
>  	struct device_node *adapter_node;
>  	struct i2c_adapter *adapter;
> +	struct gpio_desc *gpio;
> +	int r;
> +
> +	gpio = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
> +	if (IS_ERR(gpio)) {
> +		dev_err(&pdev->dev, "failed to parse HPD gpio\n");
> +		return PTR_ERR(gpio);
> +	}
> +
> +	ddata->hpd_gpio = gpio;
> +
> +	mutex_init(&ddata->hpd_lock);

Shouldn't you also have a mutex_destroy ?

> +	if (ddata->hpd_gpio) {
> +		r = devm_request_threaded_irq(&pdev->dev,
> +			gpiod_to_irq(ddata->hpd_gpio), NULL, dvic_hpd_isr,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			"DVI HPD", ddata);
> +		if (r)
> +			return r;
> +	}
> 
>  	adapter_node = of_parse_phandle(node, "ddc-i2c-bus", 0);
>  	if (adapter_node) {

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 03/24] drm/omap: add HPD support to connector-dvi

[Tomi Valkeinen]

On 27/02/18 14:58, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Monday, 12 February 2018 11:44:33 EET Tomi Valkeinen wrote:
>> Add HPD support to the DVI connector driver. The code is almost
>> identical to the HPD code in the HDMI connector driver.
> 
> Would it make sense to share a single implementation then ? Or is that an 
> exercise left for the reader (that is, me) ? :-)

It would, but I suspect all these will go away soon with the
drm_bridge/panel work, so I didn't want to start doing a bigger refactoring.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 114 ++++++++++++++++++++
>>  1 file changed, 114 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
>> b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c index
>> 391d80364346..f9f8700223c3 100644
>> --- a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
>> +++ b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> +#include <linux/gpio/consumer.h>
> 
> Could you please keep headers alphabetically sorted ?

Sure.

>>  #include <drm/drm_edid.h>
>>
>> @@ -44,6 +45,13 @@ struct panel_drv_data {
>>  	struct videomode vm;
>>
>>  	struct i2c_adapter *i2c_adapter;
>> +
>> +	struct gpio_desc *hpd_gpio;
>> +
>> +	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
>> +	void *hpd_cb_data;
>> +	bool hpd_enabled;
>> +	struct mutex hpd_lock;
> 
> Locks should have a comment that describes which fields they protect.

Added.
>>  };
>>
>>  #define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> @@ -189,6 +197,9 @@ static int dvic_read_edid(struct omap_dss_device
>> *dssdev, struct panel_drv_data *ddata = to_panel_data(dssdev);
>>  	int r, l, bytes_read;
>>
>> +	if (ddata->hpd_gpio && !gpiod_get_value_cansleep(ddata->hpd_gpio))
>> +		return -ENODEV;
>> +
>>  	if (!ddata->i2c_adapter)
>>  		return -ENODEV;
>>
>> @@ -220,6 +231,9 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
>>  	unsigned char out;
>>  	int r;
>>
>> +	if (ddata->hpd_gpio)
>> +		return gpiod_get_value_cansleep(ddata->hpd_gpio);
>> +
>>  	if (!ddata->i2c_adapter)
>>  		return true;
>>
>> @@ -228,6 +242,60 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
>> return r == 0;
>>  }
>>
>> +static int dvic_register_hpd_cb(struct omap_dss_device *dssdev,
>> +				 void (*cb)(void *cb_data,
>> +					    enum drm_connector_status status),
>> +				 void *cb_data)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> +	if (!ddata->hpd_gpio)
>> +		return -ENOTSUPP;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	ddata->hpd_cb = cb;
>> +	ddata->hpd_cb_data = cb_data;
>> +	mutex_unlock(&ddata->hpd_lock);
>> +	return 0;
>> +}
>> +
>> +static void dvic_unregister_hpd_cb(struct omap_dss_device *dssdev)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> +	if (!ddata->hpd_gpio)
>> +		return;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	ddata->hpd_cb = NULL;
>> +	ddata->hpd_cb_data = NULL;
>> +	mutex_unlock(&ddata->hpd_lock);
>> +}
>> +
>> +static void dvic_enable_hpd(struct omap_dss_device *dssdev)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> +	if (!ddata->hpd_gpio)
>> +		return;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	ddata->hpd_enabled = true;
>> +	mutex_unlock(&ddata->hpd_lock);
>> +}
>> +
>> +static void dvic_disable_hpd(struct omap_dss_device *dssdev)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> +	if (!ddata->hpd_gpio)
>> +		return;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	ddata->hpd_enabled = false;
>> +	mutex_unlock(&ddata->hpd_lock);
>> +}
>> +
>>  static struct omap_dss_driver dvic_driver = {
>>  	.connect	= dvic_connect,
>>  	.disconnect	= dvic_disconnect,
>> @@ -241,14 +309,60 @@ static struct omap_dss_driver dvic_driver = {
>>
>>  	.read_edid	= dvic_read_edid,
>>  	.detect		= dvic_detect,
>> +
>> +	.register_hpd_cb	= dvic_register_hpd_cb,
>> +	.unregister_hpd_cb	= dvic_unregister_hpd_cb,
>> +	.enable_hpd		= dvic_enable_hpd,
>> +	.disable_hpd		= dvic_disable_hpd,
>>  };
>>
>> +static irqreturn_t dvic_hpd_isr(int irq, void *data)
>> +{
>> +	struct panel_drv_data *ddata = data;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	if (ddata->hpd_enabled && ddata->hpd_cb) {
>> +		enum drm_connector_status status;
>> +
>> +		if (dvic_detect(&ddata->dssdev))
>> +			status = connector_status_connected;
>> +		else
>> +			status = connector_status_disconnected;
>> +
>> +		ddata->hpd_cb(ddata->hpd_cb_data, status);
>> +	}
>> +	mutex_unlock(&ddata->hpd_lock);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int dvic_probe_of(struct platform_device *pdev)
>>  {
>>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>>  	struct device_node *node = pdev->dev.of_node;
>>  	struct device_node *adapter_node;
>>  	struct i2c_adapter *adapter;
>> +	struct gpio_desc *gpio;
>> +	int r;
>> +
>> +	gpio = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
>> +	if (IS_ERR(gpio)) {
>> +		dev_err(&pdev->dev, "failed to parse HPD gpio\n");
>> +		return PTR_ERR(gpio);
>> +	}
>> +
>> +	ddata->hpd_gpio = gpio;
>> +
>> +	mutex_init(&ddata->hpd_lock);
> 
> Shouldn't you also have a mutex_destroy ?

Yep. Interestingly, we don't destroy any of the mutexes in omapdss/drm.
Luckily mutex_destroy doesn't seem to do much (just for debug purposes),
but still, we need to add those at some point.

 Tomi

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

[PATCH 04/24] dt-bindings: display: add HPD gpio to DVI connector

[Tomi Valkeinen]

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 Documentation/devicetree/bindings/display/connector/dvi-connector.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/connector/dvi-connector.txt b/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
index fc53f7c60bc6..207e42e9eba0 100644
--- a/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
+++ b/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
@@ -10,6 +10,7 @@ Optional properties:
 - analog: the connector has DVI analog pins
 - digital: the connector has DVI digital pins
 - dual-link: the connector has pins for DVI dual-link
+- hpd-gpios: HPD GPIO number
 
 Required nodes:
 - Video port for DVI input
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/24] dt-bindings: display: add HPD gpio to DVI connector

[Rob Herring]

On Mon, Feb 12, 2018 at 11:44:34AM +0200, Tomi Valkeinen wrote:
> Add hpd-gpios property to dvi-connector.txt.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/display/connector/dvi-connector.txt | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/24] dt-bindings: display: add HPD gpio to DVI connector

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:34 EET Tomi Valkeinen wrote:
> Add hpd-gpios property to dvi-connector.txt.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org

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

> ---
>  Documentation/devicetree/bindings/display/connector/dvi-connector.txt | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
> b/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
> index fc53f7c60bc6..207e42e9eba0 100644
> --- a/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
> +++ b/Documentation/devicetree/bindings/display/connector/dvi-connector.txt
> @@ -10,6 +10,7 @@ Optional properties:
>  - analog: the connector has DVI analog pins
>  - digital: the connector has DVI digital pins
>  - dual-link: the connector has pins for DVI dual-link
> +- hpd-gpios: HPD GPIO number
> 
>  Required nodes:
>  - Video port for DVI input


-- 
Regards,

Laurent Pinchart

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

[PATCH 05/24] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

It makes the cleanup paths a bit cleaner.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 485684c637ff..3cd9188ab30b 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -529,19 +529,17 @@ static int pdev_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	omap_crtc_pre_init();
 
 	ret = omap_connect_dssdevs();
 	if (ret)
 		goto err_crtc_uninit;
 
-	/* Allocate and initialize the driver private structure. */
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		ret = -ENOMEM;
-		goto err_disconnect_dssdevs;
-	}
-
+	/* Initialize the driver private structure. */
 	priv->dispc_ops = dispc_get_ops();
 
 	soc = soc_device_match(omapdrm_soc_devices);
@@ -555,7 +553,7 @@ static int pdev_probe(struct platform_device *pdev)
 	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
 	if (IS_ERR(ddev)) {
 		ret = PTR_ERR(ddev);
-		goto err_free_priv;
+		goto err_destroy_wq;
 	}
 
 	ddev->dev_private = priv;
@@ -610,10 +608,8 @@ static int pdev_probe(struct platform_device *pdev)
 err_free_drm_dev:
 	omap_gem_deinit(ddev);
 	drm_dev_unref(ddev);
-err_free_priv:
+err_destroy_wq:
 	destroy_workqueue(priv->wq);
-	kfree(priv);
-err_disconnect_dssdevs:
 	omap_disconnect_dssdevs();
 err_crtc_uninit:
 	omap_crtc_pre_uninit();
@@ -644,7 +640,6 @@ static int pdev_remove(struct platform_device *pdev)
 	drm_dev_unref(ddev);
 
 	destroy_workqueue(priv->wq);
-	kfree(priv);
 
 	omap_disconnect_dssdevs();
 	omap_crtc_pre_uninit();
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 05/24] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:35 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> It makes the cleanup paths a bit cleaner.

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

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 485684c637ff..3cd9188ab30b
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -529,19 +529,17 @@ static int pdev_probe(struct platform_device *pdev)
>  		return ret;
>  	}
> 
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
>  	omap_crtc_pre_init();
> 
>  	ret = omap_connect_dssdevs();
>  	if (ret)
>  		goto err_crtc_uninit;
> 
> -	/* Allocate and initialize the driver private structure. */
> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> -	if (!priv) {
> -		ret = -ENOMEM;
> -		goto err_disconnect_dssdevs;
> -	}
> -
> +	/* Initialize the driver private structure. */
>  	priv->dispc_ops = dispc_get_ops();
> 
>  	soc = soc_device_match(omapdrm_soc_devices);
> @@ -555,7 +553,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
>  	if (IS_ERR(ddev)) {
>  		ret = PTR_ERR(ddev);
> -		goto err_free_priv;
> +		goto err_destroy_wq;
>  	}
> 
>  	ddev->dev_private = priv;
> @@ -610,10 +608,8 @@ static int pdev_probe(struct platform_device *pdev)
>  err_free_drm_dev:
>  	omap_gem_deinit(ddev);
>  	drm_dev_unref(ddev);
> -err_free_priv:
> +err_destroy_wq:
>  	destroy_workqueue(priv->wq);
> -	kfree(priv);
> -err_disconnect_dssdevs:
>  	omap_disconnect_dssdevs();
>  err_crtc_uninit:
>  	omap_crtc_pre_uninit();
> @@ -644,7 +640,6 @@ static int pdev_remove(struct platform_device *pdev)
>  	drm_dev_unref(ddev);
> 
>  	destroy_workqueue(priv->wq);
> -	kfree(priv);
> 
>  	omap_disconnect_dssdevs();
>  	omap_crtc_pre_uninit();

-- 
Regards,

Laurent Pinchart

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

[PATCH 06/24] drm/omap: Allocate drm_device earlier and unref it as last step

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 3cd9188ab30b..57d11f9aeead 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -533,6 +533,14 @@ static int pdev_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	/* Allocate and initialize the DRM device. */
+	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	ddev->dev_private = priv;
+	platform_set_drvdata(pdev, ddev);
+
 	omap_crtc_pre_init();
 
 	ret = omap_connect_dssdevs();
@@ -549,16 +557,6 @@ static int pdev_probe(struct platform_device *pdev)
 	spin_lock_init(&priv->list_lock);
 	INIT_LIST_HEAD(&priv->obj_list);
 
-	/* Allocate and initialize the DRM device. */
-	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
-	if (IS_ERR(ddev)) {
-		ret = PTR_ERR(ddev);
-		goto err_destroy_wq;
-	}
-
-	ddev->dev_private = priv;
-	platform_set_drvdata(pdev, ddev);
-
 	/* Get memory bandwidth limits */
 	if (priv->dispc_ops->get_memory_bandwidth_limit)
 		priv->max_bandwidth =
@@ -569,7 +567,7 @@ static int pdev_probe(struct platform_device *pdev)
 	ret = omap_modeset_init(ddev);
 	if (ret) {
 		dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);
-		goto err_free_drm_dev;
+		goto err_gem_deinit;
 	}
 
 	/* Initialize vblank handling, start with all CRTCs disabled. */
@@ -605,14 +603,13 @@ static int pdev_probe(struct platform_device *pdev)
 err_cleanup_modeset:
 	drm_mode_config_cleanup(ddev);
 	omap_drm_irq_uninstall(ddev);
-err_free_drm_dev:
+err_gem_deinit:
 	omap_gem_deinit(ddev);
-	drm_dev_unref(ddev);
-err_destroy_wq:
 	destroy_workqueue(priv->wq);
 	omap_disconnect_dssdevs();
 err_crtc_uninit:
 	omap_crtc_pre_uninit();
+	drm_dev_unref(ddev);
 	return ret;
 }
 
@@ -637,13 +634,13 @@ static int pdev_remove(struct platform_device *pdev)
 	omap_drm_irq_uninstall(ddev);
 	omap_gem_deinit(ddev);
 
-	drm_dev_unref(ddev);
-
 	destroy_workqueue(priv->wq);
 
 	omap_disconnect_dssdevs();
 	omap_crtc_pre_uninit();
 
+	drm_dev_unref(ddev);
+
 	return 0;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 06/24] drm/omap: Allocate drm_device earlier and unref it as last step

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:36 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> If we allocate the drm_device earlier we can just return the error code
> without the need to use goto.
> Do the unref of the drm_device as a last step when cleaning up. This will
> make the drm_device available longer for us and makes sure that we only
> free up the memory when all other cleanups have been already done.

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

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 3cd9188ab30b..57d11f9aeead
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -533,6 +533,14 @@ static int pdev_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
> 
> +	/* Allocate and initialize the DRM device. */
> +	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> +	if (IS_ERR(ddev))
> +		return PTR_ERR(ddev);
> +
> +	ddev->dev_private = priv;
> +	platform_set_drvdata(pdev, ddev);
> +
>  	omap_crtc_pre_init();
> 
>  	ret = omap_connect_dssdevs();
> @@ -549,16 +557,6 @@ static int pdev_probe(struct platform_device *pdev)
>  	spin_lock_init(&priv->list_lock);
>  	INIT_LIST_HEAD(&priv->obj_list);
> 
> -	/* Allocate and initialize the DRM device. */
> -	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> -	if (IS_ERR(ddev)) {
> -		ret = PTR_ERR(ddev);
> -		goto err_destroy_wq;
> -	}
> -
> -	ddev->dev_private = priv;
> -	platform_set_drvdata(pdev, ddev);
> -
>  	/* Get memory bandwidth limits */
>  	if (priv->dispc_ops->get_memory_bandwidth_limit)
>  		priv->max_bandwidth =
> @@ -569,7 +567,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	ret = omap_modeset_init(ddev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> -		goto err_free_drm_dev;
> +		goto err_gem_deinit;
>  	}
> 
>  	/* Initialize vblank handling, start with all CRTCs disabled. */
> @@ -605,14 +603,13 @@ static int pdev_probe(struct platform_device *pdev)
>  err_cleanup_modeset:
>  	drm_mode_config_cleanup(ddev);
>  	omap_drm_irq_uninstall(ddev);
> -err_free_drm_dev:
> +err_gem_deinit:
>  	omap_gem_deinit(ddev);
> -	drm_dev_unref(ddev);
> -err_destroy_wq:
>  	destroy_workqueue(priv->wq);
>  	omap_disconnect_dssdevs();
>  err_crtc_uninit:
>  	omap_crtc_pre_uninit();
> +	drm_dev_unref(ddev);
>  	return ret;
>  }
> 
> @@ -637,13 +634,13 @@ static int pdev_remove(struct platform_device *pdev)
>  	omap_drm_irq_uninstall(ddev);
>  	omap_gem_deinit(ddev);
> 
> -	drm_dev_unref(ddev);
> -
>  	destroy_workqueue(priv->wq);
> 
>  	omap_disconnect_dssdevs();
>  	omap_crtc_pre_uninit();
> 
> +	drm_dev_unref(ddev);
> +
>  	return 0;
>  }


-- 
Regards,

Laurent Pinchart

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

[PATCH 07/24] drm/omap: Manage the usable omap_dss_device list within omap_drm_private

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

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

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 95 +++++++++++++++++++++++---------------
 drivers/gpu/drm/omapdrm/omap_drv.h |  3 ++
 2 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 57d11f9aeead..869a8ab6aa4e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -149,18 +149,27 @@ static int get_connector_type(struct omap_dss_device *dssdev)
 	}
 }
 
-static void omap_disconnect_dssdevs(void)
+static void omap_disconnect_dssdevs(struct drm_device *ddev)
 {
-	struct omap_dss_device *dssdev = NULL;
+	struct omap_drm_private *priv = ddev->dev_private;
+	unsigned int i;
+
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
 
-	for_each_dss_dev(dssdev)
 		dssdev->driver->disconnect(dssdev);
+		priv->dssdevs[i] = NULL;
+		omap_dss_put_device(dssdev);
+	}
+
+	priv->num_dssdevs = 0;
 }
 
-static int omap_connect_dssdevs(void)
+static int omap_connect_dssdevs(struct drm_device *ddev)
 {
-	int r;
+	struct omap_drm_private *priv = ddev->dev_private;
 	struct omap_dss_device *dssdev = NULL;
+	int r;
 
 	if (!omapdss_stack_is_ready())
 		return -EPROBE_DEFER;
@@ -173,6 +182,14 @@ static int omap_connect_dssdevs(void)
 		} else if (r) {
 			dev_warn(dssdev->dev, "could not connect display: %s\n",
 				dssdev->name);
+		} else {
+			omap_dss_get_device(dssdev);
+			priv->dssdevs[priv->num_dssdevs++] = dssdev;
+			if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
+				/* To balance the 'for_each_dss_dev' loop */
+				omap_dss_put_device(dssdev);
+				break;
+			}
 		}
 	}
 
@@ -183,7 +200,7 @@ static int omap_connect_dssdevs(void)
 	 * if we are deferring probe, we disconnect the devices we previously
 	 * connected
 	 */
-	omap_disconnect_dssdevs();
+	omap_disconnect_dssdevs(ddev);
 
 	return r;
 }
@@ -208,7 +225,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_ovls = priv->dispc_ops->get_num_ovls();
 	int num_mgrs = priv->dispc_ops->get_num_mgrs();
 	int num_crtcs, crtc_idx, plane_idx;
-	int ret;
+	int ret, i;
 	u32 plane_crtc_mask;
 
 	drm_mode_config_init(dev);
@@ -225,11 +242,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	 * configuration does not match the expectations or exceeds
 	 * the available resources, the configuration is rejected.
 	 */
-	num_crtcs = 0;
-	for_each_dss_dev(dssdev)
-		if (omapdss_device_is_connected(dssdev))
-			num_crtcs++;
-
+	num_crtcs = priv->num_dssdevs;
 	if (num_crtcs > num_mgrs || num_crtcs > num_ovls ||
 	    num_crtcs > ARRAY_SIZE(priv->crtcs) ||
 	    num_crtcs > ARRAY_SIZE(priv->planes) ||
@@ -247,15 +260,13 @@ static int omap_modeset_init(struct drm_device *dev)
 
 	crtc_idx = 0;
 	plane_idx = 0;
-	for_each_dss_dev(dssdev) {
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
 		struct drm_connector *connector;
 		struct drm_encoder *encoder;
 		struct drm_plane *plane;
 		struct drm_crtc *crtc;
 
-		if (!omapdss_device_is_connected(dssdev))
-			continue;
-
 		encoder = omap_encoder_init(dev, dssdev);
 		if (!encoder)
 			return -ENOMEM;
@@ -329,11 +340,14 @@ static int omap_modeset_init(struct drm_device *dev)
 /*
  * Enable the HPD in external components if supported
  */
-static void omap_modeset_enable_external_hpd(void)
+static void omap_modeset_enable_external_hpd(struct drm_device *ddev)
 {
-	struct omap_dss_device *dssdev = NULL;
+	struct omap_drm_private *priv = ddev->dev_private;
+	int i;
+
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
 
-	for_each_dss_dev(dssdev) {
 		if (dssdev->driver->enable_hpd)
 			dssdev->driver->enable_hpd(dssdev);
 	}
@@ -342,11 +356,14 @@ static void omap_modeset_enable_external_hpd(void)
 /*
  * Disable the HPD in external components if supported
  */
-static void omap_modeset_disable_external_hpd(void)
+static void omap_modeset_disable_external_hpd(struct drm_device *ddev)
 {
-	struct omap_dss_device *dssdev = NULL;
+	struct omap_drm_private *priv = ddev->dev_private;
+	int i;
+
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
 
-	for_each_dss_dev(dssdev) {
 		if (dssdev->driver->disable_hpd)
 			dssdev->driver->disable_hpd(dssdev);
 	}
@@ -543,7 +560,7 @@ static int pdev_probe(struct platform_device *pdev)
 
 	omap_crtc_pre_init();
 
-	ret = omap_connect_dssdevs();
+	ret = omap_connect_dssdevs(ddev);
 	if (ret)
 		goto err_crtc_uninit;
 
@@ -583,7 +600,7 @@ static int pdev_probe(struct platform_device *pdev)
 	omap_fbdev_init(ddev);
 
 	drm_kms_helper_poll_init(ddev);
-	omap_modeset_enable_external_hpd();
+	omap_modeset_enable_external_hpd(ddev);
 
 	/*
 	 * Register the DRM device with the core and the connectors with
@@ -596,7 +613,7 @@ static int pdev_probe(struct platform_device *pdev)
 	return 0;
 
 err_cleanup_helpers:
-	omap_modeset_disable_external_hpd();
+	omap_modeset_disable_external_hpd(ddev);
 	drm_kms_helper_poll_fini(ddev);
 
 	omap_fbdev_free(ddev);
@@ -606,7 +623,7 @@ static int pdev_probe(struct platform_device *pdev)
 err_gem_deinit:
 	omap_gem_deinit(ddev);
 	destroy_workqueue(priv->wq);
-	omap_disconnect_dssdevs();
+	omap_disconnect_dssdevs(ddev);
 err_crtc_uninit:
 	omap_crtc_pre_uninit();
 	drm_dev_unref(ddev);
@@ -622,7 +639,7 @@ static int pdev_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(ddev);
 
-	omap_modeset_disable_external_hpd();
+	omap_modeset_disable_external_hpd(ddev);
 	drm_kms_helper_poll_fini(ddev);
 
 	omap_fbdev_free(ddev);
@@ -636,7 +653,7 @@ static int pdev_remove(struct platform_device *pdev)
 
 	destroy_workqueue(priv->wq);
 
-	omap_disconnect_dssdevs();
+	omap_disconnect_dssdevs(ddev);
 	omap_crtc_pre_uninit();
 
 	drm_dev_unref(ddev);
@@ -645,11 +662,14 @@ static int pdev_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int omap_drm_suspend_all_displays(void)
+static int omap_drm_suspend_all_displays(struct drm_device *ddev)
 {
-	struct omap_dss_device *dssdev = NULL;
+	struct omap_drm_private *priv = ddev->dev_private;
+	int i;
+
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
 
-	for_each_dss_dev(dssdev) {
 		if (!dssdev->driver)
 			continue;
 
@@ -664,11 +684,14 @@ static int omap_drm_suspend_all_displays(void)
 	return 0;
 }
 
-static int omap_drm_resume_all_displays(void)
+static int omap_drm_resume_all_displays(struct drm_device *ddev)
 {
-	struct omap_dss_device *dssdev = NULL;
+	struct omap_drm_private *priv = ddev->dev_private;
+	int i;
+
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
 
-	for_each_dss_dev(dssdev) {
 		if (!dssdev->driver)
 			continue;
 
@@ -688,7 +711,7 @@ static int omap_drm_suspend(struct device *dev)
 	drm_kms_helper_poll_disable(drm_dev);
 
 	drm_modeset_lock_all(drm_dev);
-	omap_drm_suspend_all_displays();
+	omap_drm_suspend_all_displays(drm_dev);
 	drm_modeset_unlock_all(drm_dev);
 
 	return 0;
@@ -699,7 +722,7 @@ static int omap_drm_resume(struct device *dev)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 
 	drm_modeset_lock_all(drm_dev);
-	omap_drm_resume_all_displays();
+	omap_drm_resume_all_displays(drm_dev);
 	drm_modeset_unlock_all(drm_dev);
 
 	drm_kms_helper_poll_enable(drm_dev);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index ba322c519999..c9e433a91cd0 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -50,6 +50,9 @@ struct omap_drm_private {
 
 	const struct dispc_ops *dispc_ops;
 
+	unsigned int num_dssdevs;
+	struct omap_dss_device *dssdevs[8];
+
 	unsigned int num_crtcs;
 	struct drm_crtc *crtcs[8];
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 07/24] drm/omap: Manage the usable omap_dss_device list within omap_drm_private

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:37 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Instead of reaching back to DSS to iterate through the dss_devices every
> time, use an internal array where we store the available and usable
> dss_devices.
> 
> At the same time remove the omapdss_device_is_connected() check from
> omap_modeset_init() as it became irrelevant: We are not adding dssdevs
> if their connect failed.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 95 ++++++++++++++++++++---------------
>  drivers/gpu/drm/omapdrm/omap_drv.h |  3 ++
>  2 files changed, 62 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 57d11f9aeead..869a8ab6aa4e
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -149,18 +149,27 @@ static int get_connector_type(struct omap_dss_device
> *dssdev) }
>  }
> 
> -static void omap_disconnect_dssdevs(void)
> +static void omap_disconnect_dssdevs(struct drm_device *ddev)
>  {
> -	struct omap_dss_device *dssdev = NULL;
> +	struct omap_drm_private *priv = ddev->dev_private;
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
> 
> -	for_each_dss_dev(dssdev)
>  		dssdev->driver->disconnect(dssdev);
> +		priv->dssdevs[i] = NULL;
> +		omap_dss_put_device(dssdev);
> +	}
> +
> +	priv->num_dssdevs = 0;
>  }
> 
> -static int omap_connect_dssdevs(void)
> +static int omap_connect_dssdevs(struct drm_device *ddev)
>  {
> -	int r;
> +	struct omap_drm_private *priv = ddev->dev_private;
>  	struct omap_dss_device *dssdev = NULL;
> +	int r;
> 
>  	if (!omapdss_stack_is_ready())
>  		return -EPROBE_DEFER;
> @@ -173,6 +182,14 @@ static int omap_connect_dssdevs(void)
>  		} else if (r) {
>  			dev_warn(dssdev->dev, "could not connect display: %s\n",
>  				dssdev->name);
> +		} else {
> +			omap_dss_get_device(dssdev);
> +			priv->dssdevs[priv->num_dssdevs++] = dssdev;
> +			if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
> +				/* To balance the 'for_each_dss_dev' loop */
> +				omap_dss_put_device(dssdev);
> +				break;
> +			}
>  		}
>  	}
> 
> @@ -183,7 +200,7 @@ static int omap_connect_dssdevs(void)
>  	 * if we are deferring probe, we disconnect the devices we previously
>  	 * connected
>  	 */
> -	omap_disconnect_dssdevs();
> +	omap_disconnect_dssdevs(ddev);
> 
>  	return r;
>  }
> @@ -208,7 +225,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  	int num_ovls = priv->dispc_ops->get_num_ovls();
>  	int num_mgrs = priv->dispc_ops->get_num_mgrs();
>  	int num_crtcs, crtc_idx, plane_idx;
> -	int ret;
> +	int ret, i;

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

Apart from that,

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

>  	u32 plane_crtc_mask;
> 
>  	drm_mode_config_init(dev);
> @@ -225,11 +242,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  	 * configuration does not match the expectations or exceeds
>  	 * the available resources, the configuration is rejected.
>  	 */
> -	num_crtcs = 0;
> -	for_each_dss_dev(dssdev)
> -		if (omapdss_device_is_connected(dssdev))
> -			num_crtcs++;
> -
> +	num_crtcs = priv->num_dssdevs;
>  	if (num_crtcs > num_mgrs || num_crtcs > num_ovls ||
>  	    num_crtcs > ARRAY_SIZE(priv->crtcs) ||
>  	    num_crtcs > ARRAY_SIZE(priv->planes) ||
> @@ -247,15 +260,13 @@ static int omap_modeset_init(struct drm_device *dev)
> 
>  	crtc_idx = 0;
>  	plane_idx = 0;
> -	for_each_dss_dev(dssdev) {
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
>  		struct drm_connector *connector;
>  		struct drm_encoder *encoder;
>  		struct drm_plane *plane;
>  		struct drm_crtc *crtc;
> 
> -		if (!omapdss_device_is_connected(dssdev))
> -			continue;
> -
>  		encoder = omap_encoder_init(dev, dssdev);
>  		if (!encoder)
>  			return -ENOMEM;
> @@ -329,11 +340,14 @@ static int omap_modeset_init(struct drm_device *dev)
>  /*
>   * Enable the HPD in external components if supported
>   */
> -static void omap_modeset_enable_external_hpd(void)
> +static void omap_modeset_enable_external_hpd(struct drm_device *ddev)
>  {
> -	struct omap_dss_device *dssdev = NULL;
> +	struct omap_drm_private *priv = ddev->dev_private;
> +	int i;
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
> 
> -	for_each_dss_dev(dssdev) {
>  		if (dssdev->driver->enable_hpd)
>  			dssdev->driver->enable_hpd(dssdev);
>  	}
> @@ -342,11 +356,14 @@ static void omap_modeset_enable_external_hpd(void)
>  /*
>   * Disable the HPD in external components if supported
>   */
> -static void omap_modeset_disable_external_hpd(void)
> +static void omap_modeset_disable_external_hpd(struct drm_device *ddev)
>  {
> -	struct omap_dss_device *dssdev = NULL;
> +	struct omap_drm_private *priv = ddev->dev_private;
> +	int i;
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
> 
> -	for_each_dss_dev(dssdev) {
>  		if (dssdev->driver->disable_hpd)
>  			dssdev->driver->disable_hpd(dssdev);
>  	}
> @@ -543,7 +560,7 @@ static int pdev_probe(struct platform_device *pdev)
> 
>  	omap_crtc_pre_init();
> 
> -	ret = omap_connect_dssdevs();
> +	ret = omap_connect_dssdevs(ddev);
>  	if (ret)
>  		goto err_crtc_uninit;
> 
> @@ -583,7 +600,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	omap_fbdev_init(ddev);
> 
>  	drm_kms_helper_poll_init(ddev);
> -	omap_modeset_enable_external_hpd();
> +	omap_modeset_enable_external_hpd(ddev);
> 
>  	/*
>  	 * Register the DRM device with the core and the connectors with
> @@ -596,7 +613,7 @@ static int pdev_probe(struct platform_device *pdev)
>  	return 0;
> 
>  err_cleanup_helpers:
> -	omap_modeset_disable_external_hpd();
> +	omap_modeset_disable_external_hpd(ddev);
>  	drm_kms_helper_poll_fini(ddev);
> 
>  	omap_fbdev_free(ddev);
> @@ -606,7 +623,7 @@ static int pdev_probe(struct platform_device *pdev)
>  err_gem_deinit:
>  	omap_gem_deinit(ddev);
>  	destroy_workqueue(priv->wq);
> -	omap_disconnect_dssdevs();
> +	omap_disconnect_dssdevs(ddev);
>  err_crtc_uninit:
>  	omap_crtc_pre_uninit();
>  	drm_dev_unref(ddev);
> @@ -622,7 +639,7 @@ static int pdev_remove(struct platform_device *pdev)
> 
>  	drm_dev_unregister(ddev);
> 
> -	omap_modeset_disable_external_hpd();
> +	omap_modeset_disable_external_hpd(ddev);
>  	drm_kms_helper_poll_fini(ddev);
> 
>  	omap_fbdev_free(ddev);
> @@ -636,7 +653,7 @@ static int pdev_remove(struct platform_device *pdev)
> 
>  	destroy_workqueue(priv->wq);
> 
> -	omap_disconnect_dssdevs();
> +	omap_disconnect_dssdevs(ddev);
>  	omap_crtc_pre_uninit();
> 
>  	drm_dev_unref(ddev);
> @@ -645,11 +662,14 @@ static int pdev_remove(struct platform_device *pdev)
>  }
> 
>  #ifdef CONFIG_PM_SLEEP
> -static int omap_drm_suspend_all_displays(void)
> +static int omap_drm_suspend_all_displays(struct drm_device *ddev)
>  {
> -	struct omap_dss_device *dssdev = NULL;
> +	struct omap_drm_private *priv = ddev->dev_private;
> +	int i;
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
> 
> -	for_each_dss_dev(dssdev) {
>  		if (!dssdev->driver)
>  			continue;
> 
> @@ -664,11 +684,14 @@ static int omap_drm_suspend_all_displays(void)
>  	return 0;
>  }
> 
> -static int omap_drm_resume_all_displays(void)
> +static int omap_drm_resume_all_displays(struct drm_device *ddev)
>  {
> -	struct omap_dss_device *dssdev = NULL;
> +	struct omap_drm_private *priv = ddev->dev_private;
> +	int i;
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
> 
> -	for_each_dss_dev(dssdev) {
>  		if (!dssdev->driver)
>  			continue;
> 
> @@ -688,7 +711,7 @@ static int omap_drm_suspend(struct device *dev)
>  	drm_kms_helper_poll_disable(drm_dev);
> 
>  	drm_modeset_lock_all(drm_dev);
> -	omap_drm_suspend_all_displays();
> +	omap_drm_suspend_all_displays(drm_dev);
>  	drm_modeset_unlock_all(drm_dev);
> 
>  	return 0;
> @@ -699,7 +722,7 @@ static int omap_drm_resume(struct device *dev)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
> 
>  	drm_modeset_lock_all(drm_dev);
> -	omap_drm_resume_all_displays();
> +	omap_drm_resume_all_displays(drm_dev);
>  	drm_modeset_unlock_all(drm_dev);
> 
>  	drm_kms_helper_poll_enable(drm_dev);
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> b/drivers/gpu/drm/omapdrm/omap_drv.h index ba322c519999..c9e433a91cd0
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -50,6 +50,9 @@ struct omap_drm_private {
> 
>  	const struct dispc_ops *dispc_ops;
> 
> +	unsigned int num_dssdevs;
> +	struct omap_dss_device *dssdevs[8];
> +
>  	unsigned int num_crtcs;
>  	struct drm_crtc *crtcs[8];

-- 
Regards,

Laurent Pinchart

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

[PATCH 08/24] drm/omap: Separate the dssdevs array setup from the connect function

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

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>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 54 ++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 869a8ab6aa4e..b5061fc7241a 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -165,34 +165,60 @@ static void omap_disconnect_dssdevs(struct drm_device *ddev)
 	priv->num_dssdevs = 0;
 }
 
-static int omap_connect_dssdevs(struct drm_device *ddev)
+static void omap_collect_dssdevs(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
 	struct omap_dss_device *dssdev = NULL;
-	int r;
+
+	for_each_dss_dev(dssdev) {
+		omap_dss_get_device(dssdev);
+		priv->dssdevs[priv->num_dssdevs++] = dssdev;
+		if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
+			/* To balance the 'for_each_dss_dev' loop */
+			omap_dss_put_device(dssdev);
+			break;
+		}
+	}
+}
+
+static int omap_connect_dssdevs(struct drm_device *ddev)
+{
+	struct omap_drm_private *priv = ddev->dev_private;
+	u32 working = 0;
+	int r, i, j;
 
 	if (!omapdss_stack_is_ready())
 		return -EPROBE_DEFER;
 
-	for_each_dss_dev(dssdev) {
+	omap_collect_dssdevs(ddev);
+
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		struct omap_dss_device *dssdev = priv->dssdevs[i];
+
 		r = dssdev->driver->connect(dssdev);
-		if (r == -EPROBE_DEFER) {
-			omap_dss_put_device(dssdev);
+		if (r == -EPROBE_DEFER)
 			goto cleanup;
-		} else if (r) {
+		else if (r)
 			dev_warn(dssdev->dev, "could not connect display: %s\n",
-				dssdev->name);
+				 dssdev->name);
+		else
+			working |= BIT(i);
+	}
+
+	/* Remove the dssdevs if their connect failed */
+	j = 0;
+	for (i = 0; i < priv->num_dssdevs; i++) {
+		if (working & BIT(i)) {
+			if (j != i)
+				priv->dssdevs[j] = priv->dssdevs[i];
+			j++;
 		} else {
-			omap_dss_get_device(dssdev);
-			priv->dssdevs[priv->num_dssdevs++] = dssdev;
-			if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
-				/* To balance the 'for_each_dss_dev' loop */
-				omap_dss_put_device(dssdev);
-				break;
-			}
+			omap_dss_put_device(priv->dssdevs[i]);
 		}
 	}
 
+	priv->num_dssdevs = j;
+
 	return 0;
 
 cleanup:
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 08/24] drm/omap: Separate the dssdevs array setup from the connect function

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:38 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> In order to ease up on the logic,

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

> break the current code to gather the dssdevs:
> 
> first get all available dssdevs, then call connect on each dssdev. As the
> last step remove the dssdevs which failed to connect from the available
> dssdev list.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 54 ++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 869a8ab6aa4e..b5061fc7241a
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -165,34 +165,60 @@ static void omap_disconnect_dssdevs(struct drm_device
> *ddev) priv->num_dssdevs = 0;
>  }
> 
> -static int omap_connect_dssdevs(struct drm_device *ddev)
> +static void omap_collect_dssdevs(struct drm_device *ddev)
>  {
>  	struct omap_drm_private *priv = ddev->dev_private;
>  	struct omap_dss_device *dssdev = NULL;
> -	int r;
> +
> +	for_each_dss_dev(dssdev) {
> +		omap_dss_get_device(dssdev);
> +		priv->dssdevs[priv->num_dssdevs++] = dssdev;
> +		if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
> +			/* To balance the 'for_each_dss_dev' loop */
> +			omap_dss_put_device(dssdev);
> +			break;
> +		}
> +	}
> +}
> +
> +static int omap_connect_dssdevs(struct drm_device *ddev)
> +{
> +	struct omap_drm_private *priv = ddev->dev_private;
> +	u32 working = 0;
> +	int r, i, j;
> 
>  	if (!omapdss_stack_is_ready())
>  		return -EPROBE_DEFER;
> 
> -	for_each_dss_dev(dssdev) {
> +	omap_collect_dssdevs(ddev);
> +
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		struct omap_dss_device *dssdev = priv->dssdevs[i];
> +
>  		r = dssdev->driver->connect(dssdev);
> -		if (r == -EPROBE_DEFER) {
> -			omap_dss_put_device(dssdev);
> +		if (r == -EPROBE_DEFER)
>  			goto cleanup;
> -		} else if (r) {
> +		else if (r)
>  			dev_warn(dssdev->dev, "could not connect display: %s\n",
> -				dssdev->name);
> +				 dssdev->name);
> +		else
> +			working |= BIT(i);
> +	}
> +
> +	/* Remove the dssdevs if their connect failed */
> +	j = 0;
> +	for (i = 0; i < priv->num_dssdevs; i++) {
> +		if (working & BIT(i)) {
> +			if (j != i)
> +				priv->dssdevs[j] = priv->dssdevs[i];
> +			j++;
>  		} else {
> -			omap_dss_get_device(dssdev);
> -			priv->dssdevs[priv->num_dssdevs++] = dssdev;
> -			if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
> -				/* To balance the 'for_each_dss_dev' loop */
> -				omap_dss_put_device(dssdev);
> -				break;
> -			}
> +			omap_dss_put_device(priv->dssdevs[i]);
>  		}
>  	}
> 
> +	priv->num_dssdevs = j;
> +
>  	return 0;
> 
>  cleanup:

-- 
Regards,

Laurent Pinchart

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

[PATCH 09/24] drm/omap: Do dss_device (display) ordering in omap_drv.c

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

Sort the dssdev array based on DT aliases.

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/display.c |  2 ++
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  1 +
 drivers/gpu/drm/omapdrm/omap_drv.c    | 18 ++++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/display.c b/drivers/gpu/drm/omapdrm/dss/display.c
index 424143128cd4..3ef99f344bd3 100644
--- a/drivers/gpu/drm/omapdrm/dss/display.c
+++ b/drivers/gpu/drm/omapdrm/dss/display.c
@@ -52,6 +52,8 @@ int omapdss_register_display(struct omap_dss_device *dssdev)
 	if (id < 0)
 		id = disp_num_counter++;
 
+	dssdev->alias_id = id;
+
 	snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);
 
 	/* Use 'label' property for name, if it exists */
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 4222661d4c88..bd5f174a3c56 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -478,6 +478,7 @@ struct omap_dss_device {
 
 	/* alias in the form of "display%d" */
 	char alias[16];
+	int alias_id;
 
 	enum omap_display_type type;
 	enum omap_display_type output_type;
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index b5061fc7241a..877c3749f69b 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -15,6 +15,8 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/of.h>
+#include <linux/sort.h>
 #include <linux/sys_soc.h>
 
 #include <drm/drm_atomic.h>
@@ -165,6 +167,18 @@ static void omap_disconnect_dssdevs(struct drm_device *ddev)
 	priv->num_dssdevs = 0;
 }
 
+static int omap_compare_dssdevs(const void *a, const void *b)
+{
+	const struct omap_dss_device *dssdev1 = *(struct omap_dss_device **)a;
+	const struct omap_dss_device *dssdev2 = *(struct omap_dss_device **)b;
+
+	if (dssdev1->alias_id > dssdev2->alias_id)
+		return 1;
+	else if (dssdev1->alias_id < dssdev2->alias_id)
+		return -1;
+	return 0;
+}
+
 static void omap_collect_dssdevs(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
@@ -179,6 +193,10 @@ static void omap_collect_dssdevs(struct drm_device *ddev)
 			break;
 		}
 	}
+
+	/* Sort the list by DT aliases */
+	sort(priv->dssdevs, priv->num_dssdevs, sizeof(priv->dssdevs[0]),
+	     omap_compare_dssdevs, NULL);
 }
 
 static int omap_connect_dssdevs(struct drm_device *ddev)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 09/24] drm/omap: Do dss_device (display) ordering in omap_drv.c

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:39 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Sort the dssdev array based on DT aliases.
> 
> With this change we can remove the panel ordering from dss/display.c and
> have all sorting related to dssdevs in one place.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/display.c |  2 ++
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  1 +
>  drivers/gpu/drm/omapdrm/omap_drv.c    | 18 ++++++++++++++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/display.c
> b/drivers/gpu/drm/omapdrm/dss/display.c index 424143128cd4..3ef99f344bd3
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/display.c
> +++ b/drivers/gpu/drm/omapdrm/dss/display.c
> @@ -52,6 +52,8 @@ int omapdss_register_display(struct omap_dss_device
> *dssdev) if (id < 0)
>  		id = disp_num_counter++;
> 
> +	dssdev->alias_id = id;
> +
>  	snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);
> 
>  	/* Use 'label' property for name, if it exists */
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 4222661d4c88..bd5f174a3c56
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -478,6 +478,7 @@ struct omap_dss_device {
> 
>  	/* alias in the form of "display%d" */
>  	char alias[16];
> +	int alias_id;

Can the alias ID be negative ? Apart from that,

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

>  	enum omap_display_type type;
>  	enum omap_display_type output_type;
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index b5061fc7241a..877c3749f69b
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -15,6 +15,8 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> 
> +#include <linux/of.h>
> +#include <linux/sort.h>
>  #include <linux/sys_soc.h>
> 
>  #include <drm/drm_atomic.h>
> @@ -165,6 +167,18 @@ static void omap_disconnect_dssdevs(struct drm_device
> *ddev) priv->num_dssdevs = 0;
>  }
> 
> +static int omap_compare_dssdevs(const void *a, const void *b)
> +{
> +	const struct omap_dss_device *dssdev1 = *(struct omap_dss_device **)a;
> +	const struct omap_dss_device *dssdev2 = *(struct omap_dss_device **)b;
> +
> +	if (dssdev1->alias_id > dssdev2->alias_id)
> +		return 1;
> +	else if (dssdev1->alias_id < dssdev2->alias_id)
> +		return -1;
> +	return 0;
> +}
> +
>  static void omap_collect_dssdevs(struct drm_device *ddev)
>  {
>  	struct omap_drm_private *priv = ddev->dev_private;
> @@ -179,6 +193,10 @@ static void omap_collect_dssdevs(struct drm_device
> *ddev) break;
>  		}
>  	}
> +
> +	/* Sort the list by DT aliases */
> +	sort(priv->dssdevs, priv->num_dssdevs, sizeof(priv->dssdevs[0]),
> +	     omap_compare_dssdevs, NULL);
>  }
> 
>  static int omap_connect_dssdevs(struct drm_device *ddev)

-- 
Regards,

Laurent Pinchart

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

[PATCH 10/24] drm/omap: dss: Remove display ordering from dss/display.c

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

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

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/display.c | 15 +++------------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  2 --
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/display.c b/drivers/gpu/drm/omapdrm/dss/display.c
index 3ef99f344bd3..7840837f4325 100644
--- a/drivers/gpu/drm/omapdrm/dss/display.c
+++ b/drivers/gpu/drm/omapdrm/dss/display.c
@@ -41,7 +41,6 @@ static int disp_num_counter;
 int omapdss_register_display(struct omap_dss_device *dssdev)
 {
 	struct omap_dss_driver *drv = dssdev->driver;
-	struct list_head *cur;
 	int id;
 
 	/*
@@ -54,26 +53,18 @@ int omapdss_register_display(struct omap_dss_device *dssdev)
 
 	dssdev->alias_id = id;
 
-	snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);
-
 	/* Use 'label' property for name, if it exists */
 	of_property_read_string(dssdev->dev->of_node, "label", &dssdev->name);
 
 	if (dssdev->name == NULL)
-		dssdev->name = dssdev->alias;
+		dssdev->name = devm_kasprintf(dssdev->dev, GFP_KERNEL,
+					      "display%d", id);
 
 	if (drv && drv->get_timings == NULL)
 		drv->get_timings = omapdss_default_get_timings;
 
 	mutex_lock(&panel_list_mutex);
-	list_for_each(cur, &panel_list) {
-		struct omap_dss_device *ldev = list_entry(cur,
-							 struct omap_dss_device,
-							 panel_list);
-		if (strcmp(ldev->alias, dssdev->alias) > 0)
-			break;
-	}
-	list_add_tail(&dssdev->panel_list, cur);
+	list_add_tail(&dssdev->panel_list, &panel_list);
 	mutex_unlock(&panel_list_mutex);
 	return 0;
 }
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index bd5f174a3c56..ac8ca37ff889 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -476,8 +476,6 @@ struct omap_dss_device {
 
 	struct list_head panel_list;
 
-	/* alias in the form of "display%d" */
-	char alias[16];
 	int alias_id;
 
 	enum omap_display_type type;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 10/24] drm/omap: dss: Remove display ordering from dss/display.c

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:40 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> The previous patch implements the ordering of the dss_devices based on DT
> aliases in omap_drm.c, so there is no need to do the ordering in
> dss/display.c anymore.
> 
> At the same time remove the alias member of the omap_dss_device struct
> since it is no longer needed. The only place it was used is in the
> omapdss_register_display() function.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/display.c | 15 +++------------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  2 --
>  2 files changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/display.c
> b/drivers/gpu/drm/omapdrm/dss/display.c index 3ef99f344bd3..7840837f4325
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/display.c
> +++ b/drivers/gpu/drm/omapdrm/dss/display.c
> @@ -41,7 +41,6 @@ static int disp_num_counter;
>  int omapdss_register_display(struct omap_dss_device *dssdev)
>  {
>  	struct omap_dss_driver *drv = dssdev->driver;
> -	struct list_head *cur;
>  	int id;
> 
>  	/*
> @@ -54,26 +53,18 @@ int omapdss_register_display(struct omap_dss_device
> *dssdev)
> 
>  	dssdev->alias_id = id;
> 
> -	snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);
> -
>  	/* Use 'label' property for name, if it exists */
>  	of_property_read_string(dssdev->dev->of_node, "label", &dssdev->name);
> 
>  	if (dssdev->name == NULL)
> -		dssdev->name = dssdev->alias;
> +		dssdev->name = devm_kasprintf(dssdev->dev, GFP_KERNEL,
> +					      "display%d", id);

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

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

> 
>  	if (drv && drv->get_timings == NULL)
>  		drv->get_timings = omapdss_default_get_timings;
> 
>  	mutex_lock(&panel_list_mutex);
> -	list_for_each(cur, &panel_list) {
> -		struct omap_dss_device *ldev = list_entry(cur,
> -							 struct omap_dss_device,
> -							 panel_list);
> -		if (strcmp(ldev->alias, dssdev->alias) > 0)
> -			break;
> -	}
> -	list_add_tail(&dssdev->panel_list, cur);
> +	list_add_tail(&dssdev->panel_list, &panel_list);
>  	mutex_unlock(&panel_list_mutex);
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index bd5f174a3c56..ac8ca37ff889
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -476,8 +476,6 @@ struct omap_dss_device {
> 
>  	struct list_head panel_list;
> 
> -	/* alias in the form of "display%d" */
> -	char alias[16];
>  	int alias_id;
> 
>  	enum omap_display_type type;

-- 
Regards,

Laurent Pinchart

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

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

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

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

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 55 +++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 877c3749f69b..5a27a47b628e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -34,6 +34,14 @@
 #define DRIVER_MINOR		0
 #define DRIVER_PATCHLEVEL	0
 
+#define MAX_NR_DISPLAYS		8
+static int display_order[MAX_NR_DISPLAYS];
+static int display_order_nelm;
+module_param_array_named(displays, display_order, int, &display_order_nelm,
+			 0444);
+MODULE_PARM_DESC(displays,
+		 "ID array to specify the order of the active displays");
+
 /*
  * mode config funcs
  */
@@ -182,12 +190,21 @@ static int omap_compare_dssdevs(const void *a, const void *b)
 static void omap_collect_dssdevs(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
+	struct omap_dss_device *dssdevs[ARRAY_SIZE(priv->dssdevs)];
 	struct omap_dss_device *dssdev = NULL;
+	int num_dssdevs = 0;
+	unsigned long dssdev_mask = 0;
+	int i;
+
+	/* No displays should be enabled */
+	if (display_order_nelm == 1 && display_order[0] < 0)
+		return;
 
 	for_each_dss_dev(dssdev) {
 		omap_dss_get_device(dssdev);
-		priv->dssdevs[priv->num_dssdevs++] = dssdev;
-		if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
+		set_bit(num_dssdevs, &dssdev_mask);
+		dssdevs[num_dssdevs++] = dssdev;
+		if (num_dssdevs == ARRAY_SIZE(dssdevs)) {
 			/* To balance the 'for_each_dss_dev' loop */
 			omap_dss_put_device(dssdev);
 			break;
@@ -195,8 +212,38 @@ static void omap_collect_dssdevs(struct drm_device *ddev)
 	}
 
 	/* Sort the list by DT aliases */
-	sort(priv->dssdevs, priv->num_dssdevs, sizeof(priv->dssdevs[0]),
-	     omap_compare_dssdevs, NULL);
+	sort(dssdevs, num_dssdevs, sizeof(dssdevs[0]), omap_compare_dssdevs,
+	     NULL);
+
+	/* Do ordering based on the display_order parameter array */
+	for (i = 0; i < display_order_nelm; i++) {
+		int old_index = display_order[i];
+
+		if ((old_index >= 0 && old_index < num_dssdevs) &&
+		    (dssdev_mask & BIT(old_index))) {
+			priv->dssdevs[priv->num_dssdevs++] = dssdevs[old_index];
+			clear_bit(old_index, &dssdev_mask);
+		} else {
+			dev_err(ddev->dev,
+				"Ignoring invalid displays module parameter\n");
+			priv->num_dssdevs = 0;
+			break;
+		}
+	}
+
+	/* if the target list is empty, copy the collected dssdevs, if any */
+	if (priv->num_dssdevs == 0) {
+		for (i = 0; i < num_dssdevs; i++)
+			priv->dssdevs[i] = dssdevs[i];
+
+		priv->num_dssdevs = num_dssdevs;
+	} else {
+		u32 idx;
+
+		/* check if we have dssdev which is not carried over */
+		for_each_set_bit(idx, &dssdev_mask, ARRAY_SIZE(dssdevs))
+			omap_dss_put_device(dssdevs[idx]);
+	}
 }
 
 static int omap_connect_dssdevs(struct drm_device *ddev)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 11/24] drm/omap: Add kernel parameter to specify the desired display order

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:41 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> omapdrm.displays (int array) can be used to reorder the displays by id if
> needed. It can be also used to disable display.
> 
> If the board have two active displays:
> 0 - LCD
> 1 - HDMI
> then:
> omapdrm.displays=0,1 - represents the original order (LCD, HDMI)
> omapdrm.displays=1,0 - represents reverse order (HDMI, LCD)
> omapdrm.displays=0 - only the LCD is enabled
> omapdrm.displays=1 - only the HDMI is enabled
> omapdrm.displays=-1 - disable all displays

What's the use case for this ?

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 55 ++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 877c3749f69b..5a27a47b628e
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -34,6 +34,14 @@
>  #define DRIVER_MINOR		0
>  #define DRIVER_PATCHLEVEL	0
> 
> +#define MAX_NR_DISPLAYS		8
> +static int display_order[MAX_NR_DISPLAYS];
> +static int display_order_nelm;
> +module_param_array_named(displays, display_order, int, &display_order_nelm,
> +			 0444);
> +MODULE_PARM_DESC(displays,
> +		 "ID array to specify the order of the active displays");
> +
>  /*
>   * mode config funcs
>   */
> @@ -182,12 +190,21 @@ static int omap_compare_dssdevs(const void *a, const
> void *b) static void omap_collect_dssdevs(struct drm_device *ddev)
>  {
>  	struct omap_drm_private *priv = ddev->dev_private;
> +	struct omap_dss_device *dssdevs[ARRAY_SIZE(priv->dssdevs)];
>  	struct omap_dss_device *dssdev = NULL;
> +	int num_dssdevs = 0;

unsigned int.

> +	unsigned long dssdev_mask = 0;
> +	int i;

unsigned int.

> +
> +	/* No displays should be enabled */
> +	if (display_order_nelm == 1 && display_order[0] < 0)
> +		return;
> 
>  	for_each_dss_dev(dssdev) {
>  		omap_dss_get_device(dssdev);
> -		priv->dssdevs[priv->num_dssdevs++] = dssdev;
> -		if (priv->num_dssdevs == ARRAY_SIZE(priv->dssdevs)) {
> +		set_bit(num_dssdevs, &dssdev_mask);
> +		dssdevs[num_dssdevs++] = dssdev;
> +		if (num_dssdevs == ARRAY_SIZE(dssdevs)) {
>  			/* To balance the 'for_each_dss_dev' loop */
>  			omap_dss_put_device(dssdev);
>  			break;
> @@ -195,8 +212,38 @@ static void omap_collect_dssdevs(struct drm_device
> *ddev) }
> 
>  	/* Sort the list by DT aliases */
> -	sort(priv->dssdevs, priv->num_dssdevs, sizeof(priv->dssdevs[0]),
> -	     omap_compare_dssdevs, NULL);
> +	sort(dssdevs, num_dssdevs, sizeof(dssdevs[0]), omap_compare_dssdevs,
> +	     NULL);
> +
> +	/* Do ordering based on the display_order parameter array */
> +	for (i = 0; i < display_order_nelm; i++) {
> +		int old_index = display_order[i];
> +
> +		if ((old_index >= 0 && old_index < num_dssdevs) &&

No need for the inner parentheses.

> +		    (dssdev_mask & BIT(old_index))) {
> +			priv->dssdevs[priv->num_dssdevs++] = dssdevs[old_index];
> +			clear_bit(old_index, &dssdev_mask);
> +		} else {
> +			dev_err(ddev->dev,
> +				"Ignoring invalid displays module parameter\n");
> +			priv->num_dssdevs = 0;
> +			break;
> +		}
> +	}
> +
> +	/* if the target list is empty, copy the collected dssdevs, if any */
> +	if (priv->num_dssdevs == 0) {
> +		for (i = 0; i < num_dssdevs; i++)
> +			priv->dssdevs[i] = dssdevs[i];
> +
> +		priv->num_dssdevs = num_dssdevs;
> +	} else {
> +		u32 idx;
> +
> +		/* check if we have dssdev which is not carried over */
> +		for_each_set_bit(idx, &dssdev_mask, ARRAY_SIZE(dssdevs))
> +			omap_dss_put_device(dssdevs[idx]);
> +	}
>  }
> 
>  static int omap_connect_dssdevs(struct drm_device *ddev)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 11/24] drm/omap: Add kernel parameter to specify the desired display order

[Tomi Valkeinen]

On 27/02/18 15:35, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Monday, 12 February 2018 11:44:41 EET Tomi Valkeinen wrote:
>> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>
>> omapdrm.displays (int array) can be used to reorder the displays by id if
>> needed. It can be also used to disable display.
>>
>> If the board have two active displays:
>> 0 - LCD
>> 1 - HDMI
>> then:
>> omapdrm.displays=0,1 - represents the original order (LCD, HDMI)
>> omapdrm.displays=1,0 - represents reverse order (HDMI, LCD)
>> omapdrm.displays=0 - only the LCD is enabled
>> omapdrm.displays=1 - only the HDMI is enabled
>> omapdrm.displays=-1 - disable all displays
> 
> What's the use case for this ?

fbdev uses the first display as "main" display. There are also many apps
using DRM APIs that unfortunately depend on the order, and presume the
first one is the initial or main display. It's also a nice tool for
debugging, so that you can easily fully disable a display or all displays.

 Tomi

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

[PATCH 12/24] drm/omap: Init fbdev emulation only when we have displays

[Tomi Valkeinen]

From: Peter Ujfalusi <peter.ujfalusi@ti.com>

Do not try to init the fbdev if either num_crtcs or num_connectors is 0.
In this case we do not have display so the fbdev init would fail anyways.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 30ce3d562414..509283af5b0c 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -249,6 +249,9 @@ void omap_fbdev_init(struct drm_device *dev)
 	struct drm_fb_helper *helper;
 	int ret = 0;
 
+	if (!priv->num_crtcs || !priv->num_connectors)
+		return;
+
 	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
 	if (!fbdev)
 		goto fail;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 12/24] drm/omap: Init fbdev emulation only when we have displays

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:42 EET Tomi Valkeinen wrote:
> From: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Do not try to init the fbdev if either num_crtcs or num_connectors is 0.
> In this case we do not have display so the fbdev init would fail anyways.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

I'd move this earlier in the series with the other fbdev-related patches.

> ---
>  drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 30ce3d562414..509283af5b0c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -249,6 +249,9 @@ void omap_fbdev_init(struct drm_device *dev)
>  	struct drm_fb_helper *helper;
>  	int ret = 0;
> 
> +	if (!priv->num_crtcs || !priv->num_connectors)
> +		return;
> +
>  	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>  	if (!fbdev)
>  		goto fail;

-- 
Regards,

Laurent Pinchart

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

[PATCH 13/24] drm/omap: remove leftover enums

[Tomi Valkeinen]

A few enums are not used anywhere, so remove them.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/omapdss.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index ac8ca37ff889..51aefd80bcd4 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -159,21 +159,6 @@ enum omap_overlay_caps {
 	OMAP_DSS_OVL_CAP_REPLICATION = 1 << 5,
 };
 
-enum omap_dss_clk_source {
-	OMAP_DSS_CLK_SRC_FCK = 0,		/* OMAP2/3: DSS1_ALWON_FCLK
-						 * OMAP4: DSS_FCLK */
-	OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC,	/* OMAP3: DSI1_PLL_FCLK
-						 * OMAP4: PLL1_CLK1 */
-	OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DSI,	/* OMAP3: DSI2_PLL_FCLK
-						 * OMAP4: PLL1_CLK2 */
-	OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC,	/* OMAP4: PLL2_CLK1 */
-	OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DSI,	/* OMAP4: PLL2_CLK2 */
-};
-
-enum omap_hdmi_flags {
-	OMAP_HDMI_SDA_SCL_EXTERNAL_PULLUP = 1 << 0,
-};
-
 enum omap_dss_output_id {
 	OMAP_DSS_OUTPUT_DPI	= 1 << 0,
 	OMAP_DSS_OUTPUT_DBI	= 1 << 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: [PATCH 13/24] drm/omap: remove leftover enums

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:43 EET Tomi Valkeinen wrote:
> A few enums are not used anywhere, so remove them.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

> ---
>  drivers/gpu/drm/omapdrm/dss/omapdss.h | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index ac8ca37ff889..51aefd80bcd4
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -159,21 +159,6 @@ enum omap_overlay_caps {
>  	OMAP_DSS_OVL_CAP_REPLICATION = 1 << 5,
>  };
> 
> -enum omap_dss_clk_source {
> -	OMAP_DSS_CLK_SRC_FCK = 0,		/* OMAP2/3: DSS1_ALWON_FCLK
> -						 * OMAP4: DSS_FCLK */
> -	OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC,	/* OMAP3: DSI1_PLL_FCLK
> -						 * OMAP4: PLL1_CLK1 */
> -	OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DSI,	/* OMAP3: DSI2_PLL_FCLK
> -						 * OMAP4: PLL1_CLK2 */
> -	OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC,	/* OMAP4: PLL2_CLK1 */
> -	OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DSI,	/* OMAP4: PLL2_CLK2 */
> -};
> -
> -enum omap_hdmi_flags {
> -	OMAP_HDMI_SDA_SCL_EXTERNAL_PULLUP = 1 << 0,
> -};
> -
>  enum omap_dss_output_id {
>  	OMAP_DSS_OUTPUT_DPI	= 1 << 0,
>  	OMAP_DSS_OUTPUT_DBI	= 1 << 1,

-- 
Regards,

Laurent Pinchart

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

[PATCH 14/24] drm/omap: dispc: disp_wb_setup to check return code

[Tomi Valkeinen]

From: Benoit Parrot <bparrot@ti.com>

When dispc_wb_setup() calls dispc_ovl_setup_common() it does not
check for failure but instead keeps on partially setting up WB.
This causes the WB H/W to be partially initialized and yield
unexpected behavior.

Make sure return code is successful before proceeding.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 679931e108f9..30bcee6580f5 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2678,6 +2678,8 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 		wi->height, wi->fourcc, wi->rotation, zorder,
 		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
 		replication, vm, mem_to_mem);
+	if (r)
+		return r;
 
 	switch (wi->fourcc) {
 	case DRM_FORMAT_RGB565:
@@ -2718,7 +2720,7 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 		REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), wbdelay, 7, 0);
 	}
 
-	return r;
+	return 0;
 }
 
 static int dispc_ovl_enable(enum omap_plane_id plane, bool enable)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 14/24] drm/omap: dispc: disp_wb_setup to check return code

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:44 EET Tomi Valkeinen wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> When dispc_wb_setup() calls dispc_ovl_setup_common() it does not
> check for failure but instead keeps on partially setting up WB.
> This causes the WB H/W to be partially initialized and yield
> unexpected behavior.
> 
> Make sure return code is successful before proceeding.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 679931e108f9..30bcee6580f5
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -2678,6 +2678,8 @@ int dispc_wb_setup(const struct
> omap_dss_writeback_info *wi, wi->height, wi->fourcc, wi->rotation, zorder,
>  		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
>  		replication, vm, mem_to_mem);
> +	if (r)
> +		return r;
> 
>  	switch (wi->fourcc) {
>  	case DRM_FORMAT_RGB565:
> @@ -2718,7 +2720,7 @@ int dispc_wb_setup(const struct
> omap_dss_writeback_info *wi, REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane),
> wbdelay, 7, 0);
>  	}
> 
> -	return r;
> +	return 0;
>  }
> 
>  static int dispc_ovl_enable(enum omap_plane_id plane, bool enable)

-- 
Regards,

Laurent Pinchart

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

[PATCH 15/24] drm/omap: Add pclk setting case when channel is DSS_WB

[Tomi Valkeinen]

From: Benoit Parrot <bparrot@ti.com>

In dispc_set_ovl_common() we need to initialize pclk to a valid
value when we use WB in capture mode (i.e. mem_2_mem is false).
Otherwise dispc_ovl_calc_scaling() fails.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 30bcee6580f5..5e7bdff2821d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2488,6 +2488,10 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 	unsigned long pclk = dispc_plane_pclk_rate(plane);
 	unsigned long lclk = dispc_plane_lclk_rate(plane);
 
+	/* when setting up WB, dispc_plane_pclk_rate() returns 0 */
+	if (plane == OMAP_DSS_WB)
+		pclk = vm->pixelclock;
+
 	if (paddr == 0 && rotation_type != OMAP_DSS_ROT_TILER)
 		return -EINVAL;
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 16/24] drm/omap: set WB channel-in in wb_setup()

[Tomi Valkeinen]

We need to know the WB channel-in in wb_setup() to be able to configure
WB properly for capture mode. At the moment channel-in is set
separately.

This patch moves channel-in to wb_setup().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 11 +++--------
 drivers/gpu/drm/omapdrm/dss/dss.h   |  3 ++-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 5e7bdff2821d..7f9186894bd5 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -1187,13 +1187,6 @@ static enum omap_channel dispc_ovl_get_channel_out(enum omap_plane_id plane)
 	}
 }
 
-void dispc_wb_set_channel_in(enum dss_writeback_channel channel)
-{
-	enum omap_plane_id plane = OMAP_DSS_WB;
-
-	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), channel, 18, 16);
-}
-
 static void dispc_ovl_set_burst_size(enum omap_plane_id plane,
 		enum omap_burst_size burst_size)
 {
@@ -2659,7 +2652,8 @@ static int dispc_ovl_setup(enum omap_plane_id plane,
 }
 
 int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
-		bool mem_to_mem, const struct videomode *vm)
+		bool mem_to_mem, const struct videomode *vm,
+		enum dss_writeback_channel channel_in)
 {
 	int r;
 	u32 l;
@@ -2704,6 +2698,7 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 	/* setup extra DISPC_WB_ATTRIBUTES */
 	l = dispc_read_reg(DISPC_OVL_ATTRIBUTES(plane));
 	l = FLD_MOD(l, truncation, 10, 10);	/* TRUNCATIONENABLE */
+	l = FLD_MOD(l, channel_in, 18, 16);	/* CHANNELIN */
 	l = FLD_MOD(l, mem_to_mem, 19, 19);	/* WRITEBACKMODE */
 	if (mem_to_mem)
 		l = FLD_MOD(l, 1, 26, 24);	/* CAPTUREMODE */
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h
index 7f3fa5330408..19143ab5393c 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss.h
@@ -385,7 +385,8 @@ bool dispc_wb_go_busy(void);
 void dispc_wb_go(void);
 void dispc_wb_set_channel_in(enum dss_writeback_channel channel);
 int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
-		bool mem_to_mem, const struct videomode *vm);
+		bool mem_to_mem, const struct videomode *vm,
+		enum dss_writeback_channel channel_in);
 
 #ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
 static inline void dss_collect_irq_stats(u32 irqstatus, unsigned int *irq_arr)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 17/24] drm/omap: fix WBDELAYCOUNT for HDMI

[Tomi Valkeinen]

For HDMI, WBDELAYCOUNT starts counting at the start of vsync, not at the
start of vfp.

This patch adjusts the wbdelay for HDMI accordingly.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 7f9186894bd5..669ee9df224b 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2712,8 +2712,12 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 	} else {
 		int wbdelay;
 
-		wbdelay = min(vm->vfront_porch +
-			      vm->vsync_len + vm->vback_porch, (u32)255);
+		if (channel_in == DSS_WB_TV_MGR)
+			wbdelay = min(vm->vsync_len + vm->vback_porch,
+				(u32)255);
+		else
+			wbdelay = min(vm->vfront_porch +
+				vm->vsync_len + vm->vback_porch, (u32)255);
 
 		/* WBDELAYCOUNT */
 		REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), wbdelay, 7, 0);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 18/24] drm/omap: fix WBDELAYCOUNT with interlace

[Tomi Valkeinen]

Vertical blanking needs to be halved on interlace modes. WBDELAYCOUNT
was calculated without such halving, resulting in WBUNCOMPLETE errors.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Acked-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 669ee9df224b..99bbc97d0de4 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2710,14 +2710,18 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 		/* WBDELAYCOUNT */
 		REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), 0, 7, 0);
 	} else {
-		int wbdelay;
+		u32 wbdelay;
 
 		if (channel_in == DSS_WB_TV_MGR)
-			wbdelay = min(vm->vsync_len + vm->vback_porch,
-				(u32)255);
+			wbdelay = vm->vsync_len + vm->vback_porch;
 		else
-			wbdelay = min(vm->vfront_porch +
-				vm->vsync_len + vm->vback_porch, (u32)255);
+			wbdelay = vm->vfront_porch + vm->vsync_len +
+				vm->vback_porch;
+
+		if (vm->flags & DISPLAY_FLAGS_INTERLACED)
+			wbdelay /= 2;
+
+		wbdelay = min(wbdelay, 255u);
 
 		/* WBDELAYCOUNT */
 		REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), wbdelay, 7, 0);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 19/24] drm/omap: fix WB height with interlace

[Tomi Valkeinen]

When using WB capture from interlaced source, we need to halve the
picture heights correctly.

Unfortunately the current dispc_ovl_setup_common() doesn't deal with
interlace very neatly, so the end result is a bit messy.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Acked-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 99bbc97d0de4..3d804187df13 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2496,18 +2496,19 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 	out_width = out_width == 0 ? width : out_width;
 	out_height = out_height == 0 ? height : out_height;
 
-	if (ilace && height == out_height)
-		fieldmode = true;
-
-	if (ilace) {
-		if (fieldmode)
-			in_height /= 2;
-		pos_y /= 2;
-		out_height /= 2;
-
-		DSSDBG("adjusting for ilace: height %d, pos_y %d, "
-			"out_height %d\n", in_height, pos_y,
-			out_height);
+	if (plane != OMAP_DSS_WB) {
+		if (ilace && height == out_height)
+			fieldmode = true;
+
+		if (ilace) {
+			if (fieldmode)
+				in_height /= 2;
+			pos_y /= 2;
+			out_height /= 2;
+
+			DSSDBG("adjusting for ilace: height %d, pos_y %d, out_height %d\n",
+				in_height, pos_y, out_height);
+		}
 	}
 
 	if (!dispc_ovl_color_mode_supported(plane, fourcc))
@@ -2667,6 +2668,9 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 	enum omap_overlay_caps caps =
 		OMAP_DSS_OVL_CAP_SCALE | OMAP_DSS_OVL_CAP_PRE_MULT_ALPHA;
 
+	if (vm->flags & DISPLAY_FLAGS_INTERLACED)
+		in_height /= 2;
+
 	DSSDBG("dispc_wb_setup, pa %x, pa_uv %x, %d,%d -> %dx%d, cmode %x, "
 		"rot %d\n", wi->paddr, wi->p_uv_addr, in_width,
 		in_height, wi->width, wi->height, wi->fourcc, wi->rotation);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 20/24] drm/omap: fix scaling limits for WB

[Tomi Valkeinen]

WB has additional scaling limits when the output color format is one of
the YUV formats. These limits are not handled at the moment, causing
bad scaling and/or NULL dereference crash.

This patchs adds the check so that dispc returns an error for bad
scaling request.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 3d804187df13..2d19852553f5 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2381,7 +2381,8 @@ static int dispc_ovl_calc_scaling_44xx(unsigned long pclk, unsigned long lclk,
 #define DIV_FRAC(dividend, divisor) \
 	((dividend) * 100 / (divisor) - ((dividend) / (divisor) * 100))
 
-static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk,
+static int dispc_ovl_calc_scaling(enum omap_plane_id plane,
+		unsigned long pclk, unsigned long lclk,
 		enum omap_overlay_caps caps,
 		const struct videomode *vm,
 		u16 width, u16 height, u16 out_width, u16 out_height,
@@ -2389,7 +2390,8 @@ static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk,
 		int *x_predecim, int *y_predecim, u16 pos_x,
 		enum omap_dss_rotation_type rotation_type, bool mem_to_mem)
 {
-	const int maxdownscale = dispc.feat->max_downscale;
+	int maxhdownscale = dispc.feat->max_downscale;
+	int maxvdownscale = dispc.feat->max_downscale;
 	const int max_decim_limit = 16;
 	unsigned long core_clk = 0;
 	int decim_x, decim_y, ret;
@@ -2397,6 +2399,20 @@ static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk,
 	if (width == out_width && height == out_height)
 		return 0;
 
+	if (plane == OMAP_DSS_WB) {
+		switch (fourcc) {
+		case DRM_FORMAT_NV12:
+			maxhdownscale = maxvdownscale = 2;
+			break;
+		case DRM_FORMAT_YUYV:
+		case DRM_FORMAT_UYVY:
+			maxhdownscale = 2;
+			maxvdownscale = 4;
+			break;
+		default:
+			break;
+		}
+	}
 	if (!mem_to_mem && (pclk == 0 || vm->pixelclock == 0)) {
 		DSSERR("cannot calculate scaling settings: pclk is zero\n");
 		return -EINVAL;
@@ -2414,8 +2430,8 @@ static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk,
 				2 : max_decim_limit;
 	}
 
-	decim_x = DIV_ROUND_UP(DIV_ROUND_UP(width, out_width), maxdownscale);
-	decim_y = DIV_ROUND_UP(DIV_ROUND_UP(height, out_height), maxdownscale);
+	decim_x = DIV_ROUND_UP(DIV_ROUND_UP(width, out_width), maxhdownscale);
+	decim_y = DIV_ROUND_UP(DIV_ROUND_UP(height, out_height), maxvdownscale);
 
 	if (decim_x > *x_predecim || out_width > width * 8)
 		return -EINVAL;
@@ -2514,7 +2530,7 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 	if (!dispc_ovl_color_mode_supported(plane, fourcc))
 		return -EINVAL;
 
-	r = dispc_ovl_calc_scaling(pclk, lclk, caps, vm, in_width,
+	r = dispc_ovl_calc_scaling(plane, pclk, lclk, caps, vm, in_width,
 			in_height, out_width, out_height, fourcc,
 			&five_taps, &x_predecim, &y_predecim, pos_x,
 			rotation_type, mem_to_mem);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 21/24] drm/omap: add writeback funcs to dispc_ops

[Tomi Valkeinen]

Add writeback specific dispc functions to dispc_ops so that omapdrm can
use them.  Also move 'enum dss_writeback_channel' to the public
omapdss.h for omapdrm.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 19 +++++++++++++++----
 drivers/gpu/drm/omapdrm/dss/dss.h     | 19 -------------------
 drivers/gpu/drm/omapdrm/dss/omapdss.h | 19 +++++++++++++++++++
 3 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 2d19852553f5..ff09e2be470f 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -698,7 +698,7 @@ static u32 dispc_mgr_get_sync_lost_irq(enum omap_channel channel)
 	return mgr_desc[channel].sync_lost_irq;
 }
 
-u32 dispc_wb_get_framedone_irq(void)
+static u32 dispc_wb_get_framedone_irq(void)
 {
 	return DISPC_IRQ_FRAMEDONEWB;
 }
@@ -730,12 +730,12 @@ static void dispc_mgr_go(enum omap_channel channel)
 	mgr_fld_write(channel, DISPC_MGR_FLD_GO, 1);
 }
 
-bool dispc_wb_go_busy(void)
+static bool dispc_wb_go_busy(void)
 {
 	return REG_GET(DISPC_CONTROL2, 6, 6) == 1;
 }
 
-void dispc_wb_go(void)
+static void dispc_wb_go(void)
 {
 	enum omap_plane_id plane = OMAP_DSS_WB;
 	bool enable, go;
@@ -2668,7 +2668,7 @@ static int dispc_ovl_setup(enum omap_plane_id plane,
 	return r;
 }
 
-int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
+static int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 		bool mem_to_mem, const struct videomode *vm,
 		enum dss_writeback_channel channel_in)
 {
@@ -2750,6 +2750,11 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 	return 0;
 }
 
+static bool dispc_has_writeback(void)
+{
+	return dispc.feat->has_writeback;
+}
+
 static int dispc_ovl_enable(enum omap_plane_id plane, bool enable)
 {
 	DSSDBG("dispc_enable_plane %d, %d\n", plane, enable);
@@ -4553,6 +4558,12 @@ static const struct dispc_ops dispc_ops = {
 	.ovl_enable = dispc_ovl_enable,
 	.ovl_setup = dispc_ovl_setup,
 	.ovl_get_color_modes = dispc_ovl_get_color_modes,
+
+	.wb_get_framedone_irq = dispc_wb_get_framedone_irq,
+	.wb_setup = dispc_wb_setup,
+	.has_writeback = dispc_has_writeback,
+	.wb_go_busy = dispc_wb_go_busy,
+	.wb_go = dispc_wb_go,
 };
 
 /* DISPC HW IP initialisation */
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h
index 19143ab5393c..e2e679544e41 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss.h
@@ -97,17 +97,6 @@ enum dss_dsi_content_type {
 	DSS_DSI_CONTENT_GENERIC,
 };
 
-enum dss_writeback_channel {
-	DSS_WB_LCD1_MGR =	0,
-	DSS_WB_LCD2_MGR =	1,
-	DSS_WB_TV_MGR =		2,
-	DSS_WB_OVL0 =		3,
-	DSS_WB_OVL1 =		4,
-	DSS_WB_OVL2 =		5,
-	DSS_WB_OVL3 =		6,
-	DSS_WB_LCD3_MGR =	7,
-};
-
 enum dss_clk_source {
 	DSS_CLK_SRC_FCK = 0,
 
@@ -380,14 +369,6 @@ int dispc_mgr_get_clock_div(enum omap_channel channel,
 		struct dispc_clock_info *cinfo);
 void dispc_set_tv_pclk(unsigned long pclk);
 
-u32 dispc_wb_get_framedone_irq(void);
-bool dispc_wb_go_busy(void);
-void dispc_wb_go(void);
-void dispc_wb_set_channel_in(enum dss_writeback_channel channel);
-int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
-		bool mem_to_mem, const struct videomode *vm,
-		enum dss_writeback_channel channel_in);
-
 #ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
 static inline void dss_collect_irq_stats(u32 irqstatus, unsigned int *irq_arr)
 {
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 51aefd80bcd4..2139735878c8 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -618,6 +618,17 @@ void omapdss_set_is_initialized(bool set);
 struct device_node *dss_of_port_get_parent_device(struct device_node *port);
 u32 dss_of_port_get_port_number(struct device_node *port);
 
+enum dss_writeback_channel {
+	DSS_WB_LCD1_MGR =	0,
+	DSS_WB_LCD2_MGR =	1,
+	DSS_WB_TV_MGR =		2,
+	DSS_WB_OVL0 =		3,
+	DSS_WB_OVL1 =		4,
+	DSS_WB_OVL2 =		5,
+	DSS_WB_OVL3 =		6,
+	DSS_WB_LCD3_MGR =	7,
+};
+
 struct dss_mgr_ops {
 	int (*connect)(enum omap_channel channel,
 		struct omap_dss_device *dst);
@@ -700,6 +711,14 @@ struct dispc_ops {
 			enum omap_channel channel);
 
 	const u32 *(*ovl_get_color_modes)(enum omap_plane_id plane);
+
+	u32 (*wb_get_framedone_irq)(void);
+	int (*wb_setup)(const struct omap_dss_writeback_info *wi,
+		bool mem_to_mem, const struct videomode *vm,
+		enum dss_writeback_channel channel_in);
+	bool (*has_writeback)(void);
+	bool (*wb_go_busy)(void);
+	void (*wb_go)(void);
 };
 
 void dispc_set_ops(const struct dispc_ops *o);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 22/24] drm/omap: fix maximum sizes

[Tomi Valkeinen]

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

Change the maximum width & height to 8192.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 5a27a47b628e..2df125369781 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -412,11 +412,14 @@ static int omap_modeset_init(struct drm_device *dev)
 	dev->mode_config.min_width = 8;
 	dev->mode_config.min_height = 2;
 
-	/* note: eventually will need some cpu_is_omapXYZ() type stuff here
-	 * to fill in these limits properly on different OMAP generations..
+	/*
+	 * Note: these values are used for multiple independent things:
+	 * connector mode filtering, buffer sizes, crtc sizes...
+	 * Use big enough values here to cover all use cases, and do more
+	 * specific checking in the respective code paths.
 	 */
-	dev->mode_config.max_width = 2048;
-	dev->mode_config.max_height = 2048;
+	dev->mode_config.max_width = 8192;
+	dev->mode_config.max_height = 8192;
 
 	dev->mode_config.funcs = &omap_mode_config_funcs;
 	dev->mode_config.helper_private = &omap_mode_config_helper_funcs;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 22/24] drm/omap: fix maximum sizes

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:52 EET Tomi Valkeinen wrote:
> We define max width and height in mode_config to 2048. These maximums
> affect many things, which are independent and depend on platform. We
> need to do more fine grained checks in the code paths for each
> component, and so the maximum values in mode_config should just be "big
> enough" to cover all use cases.
> 
> Change the maximum width & height to 8192.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 5a27a47b628e..2df125369781
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -412,11 +412,14 @@ static int omap_modeset_init(struct drm_device *dev)
>  	dev->mode_config.min_width = 8;
>  	dev->mode_config.min_height = 2;
> 
> -	/* note: eventually will need some cpu_is_omapXYZ() type stuff here
> -	 * to fill in these limits properly on different OMAP generations..
> +	/*
> +	 * Note: these values are used for multiple independent things:
> +	 * connector mode filtering, buffer sizes, crtc sizes...
> +	 * Use big enough values here to cover all use cases, and do more
> +	 * specific checking in the respective code paths.
>  	 */
> -	dev->mode_config.max_width = 2048;
> -	dev->mode_config.max_height = 2048;
> +	dev->mode_config.max_width = 8192;
> +	dev->mode_config.max_height = 8192;

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

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

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 22/24] drm/omap: fix maximum sizes

[Tomi Valkeinen]

On 27/02/18 15:42, Laurent Pinchart wrote:

>> +	/*
>> +	 * Note: these values are used for multiple independent things:
>> +	 * connector mode filtering, buffer sizes, crtc sizes...
>> +	 * Use big enough values here to cover all use cases, and do more
>> +	 * specific checking in the respective code paths.
>>  	 */
>> -	dev->mode_config.max_width = 2048;
>> -	dev->mode_config.max_height = 2048;
>> +	dev->mode_config.max_width = 8192;
>> +	dev->mode_config.max_height = 8192;
> 
> This makes me ponder on the usefulness of the max_width and max_height fields. 
> If we end up having to set them to very large values so they don't get in the 
> way, and implement independent checks manually, the fields should then be 
> split into finer-grained parameters to make DRM core checks useful.

I agree.

 Tomi

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

[PATCH 23/24] drm/omap: Allow HDMI audio setup even if we do not have video configured

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

Allow HDMI audio setup even if we do not have video configured. Audio
will get configured at the same time with video if the video is
configured soon enough. If it is not the audio DMA will timeout in
couple of seconds and audio playback will be aborted.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4.c | 33 ++++++++++++++-------------------
 drivers/gpu/drm/omapdrm/dss/hdmi5.c | 37 ++++++++++++++++---------------------
 2 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index ae6401c569c4..9d5c921cbf7b 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -598,21 +598,16 @@ static int hdmi_audio_startup(struct device *dev,
 			      void (*abort_cb)(struct device *dev))
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
-	int ret = 0;
 
 	mutex_lock(&hd->lock);
 
-	if (!hdmi_mode_has_audio(&hd->cfg) || !hd->display_enabled) {
-		ret = -EPERM;
-		goto out;
-	}
+	WARN_ON(hd->audio_abort_cb != NULL);
 
 	hd->audio_abort_cb = abort_cb;
 
-out:
 	mutex_unlock(&hd->lock);
 
-	return ret;
+	return 0;
 }
 
 static int hdmi_audio_shutdown(struct device *dev)
@@ -633,12 +628,14 @@ static int hdmi_audio_start(struct device *dev)
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
-
 	spin_lock_irqsave(&hd->audio_playing_lock, flags);
 
-	if (hd->display_enabled)
+	if (hd->display_enabled) {
+		if (!hdmi_mode_has_audio(&hd->cfg))
+			DSSERR("%s: Video mode does not support audio\n",
+			       __func__);
 		hdmi_start_audio_stream(hd);
+	}
 	hd->audio_playing = true;
 
 	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
@@ -669,17 +666,15 @@ static int hdmi_audio_config(struct device *dev,
 
 	mutex_lock(&hd->lock);
 
-	if (!hdmi_mode_has_audio(&hd->cfg) || !hd->display_enabled) {
-		ret = -EPERM;
-		goto out;
+	if (hd->display_enabled) {
+		ret = hdmi4_audio_config(&hd->core, &hd->wp, dss_audio,
+					 hd->cfg.vm.pixelclock);
+		if (ret)
+			goto out;
 	}
 
-	ret = hdmi4_audio_config(&hd->core, &hd->wp, dss_audio,
-				 hd->cfg.vm.pixelclock);
-	if (!ret) {
-		hd->audio_configured = true;
-		hd->audio_config = *dss_audio;
-	}
+	hd->audio_configured = true;
+	hd->audio_config = *dss_audio;
 out:
 	mutex_unlock(&hd->lock);
 
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index 9571be938d81..33297d282a61 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -593,21 +593,16 @@ static int hdmi_audio_startup(struct device *dev,
 			      void (*abort_cb)(struct device *dev))
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
-	int ret = 0;
 
 	mutex_lock(&hd->lock);
 
-	if (!hdmi_mode_has_audio(&hd->cfg) || !hd->display_enabled) {
-		ret = -EPERM;
-		goto out;
-	}
+	WARN_ON(hd->audio_abort_cb != NULL);
 
 	hd->audio_abort_cb = abort_cb;
 
-out:
 	mutex_unlock(&hd->lock);
 
-	return ret;
+	return 0;
 }
 
 static int hdmi_audio_shutdown(struct device *dev)
@@ -628,12 +623,14 @@ static int hdmi_audio_start(struct device *dev)
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
-
 	spin_lock_irqsave(&hd->audio_playing_lock, flags);
 
-	if (hd->display_enabled)
+	if (hd->display_enabled) {
+		if (!hdmi_mode_has_audio(&hd->cfg))
+			DSSERR("%s: Video mode does not support audio\n",
+			       __func__);
 		hdmi_start_audio_stream(hd);
+	}
 	hd->audio_playing = true;
 
 	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
@@ -645,7 +642,8 @@ static void hdmi_audio_stop(struct device *dev)
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
+	if (!hdmi_mode_has_audio(&hd->cfg))
+		DSSERR("%s: Video mode does not support audio\n", __func__);
 
 	spin_lock_irqsave(&hd->audio_playing_lock, flags);
 
@@ -664,18 +662,15 @@ static int hdmi_audio_config(struct device *dev,
 
 	mutex_lock(&hd->lock);
 
-	if (!hdmi_mode_has_audio(&hd->cfg) || !hd->display_enabled) {
-		ret = -EPERM;
-		goto out;
+	if (hd->display_enabled) {
+		ret = hdmi5_audio_config(&hd->core, &hd->wp, dss_audio,
+					 hd->cfg.vm.pixelclock);
+		if (ret)
+			goto out;
 	}
 
-	ret = hdmi5_audio_config(&hd->core, &hd->wp, dss_audio,
-				 hd->cfg.vm.pixelclock);
-
-	if (!ret) {
-		hd->audio_configured = true;
-		hd->audio_config = *dss_audio;
-	}
+	hd->audio_configured = true;
+	hd->audio_config = *dss_audio;
 out:
 	mutex_unlock(&hd->lock);
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 24/24] drm/omap: cleanup color space conversion

[Tomi Valkeinen]

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

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

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 59 +++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index ff09e2be470f..697274317f7c 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -345,11 +345,6 @@ static const struct {
 	},
 };
 
-struct color_conv_coef {
-	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
-	int full_range;
-};
-
 static unsigned long dispc_fclk_rate(void);
 static unsigned long dispc_core_clk_rate(void);
 static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
@@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id plane, int fir_hinc,
 	}
 }
 
+struct csc_coef_yuv2rgb {
+	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
+	bool full_range;
+};
+
+struct csc_coef_rgb2yuv {
+	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
+	bool full_range;
+};
 
 static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
-		const struct color_conv_coef *ct)
+		const struct csc_coef_yuv2rgb *ct)
 {
 #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
 
@@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
 	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->bcr, ct->by));
 	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->bcb));
 
-	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
+	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
+
+#undef CVAL
+}
+
+static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv *ct)
+{
+	const enum omap_plane_id plane = OMAP_DSS_WB;
+
+#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
+
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
+
+	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
 
 #undef CVAL
 }
@@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void)
 {
 	int i;
 	int num_ovl = dispc_get_num_ovls();
-	const struct color_conv_coef ctbl_bt601_5_ovl = {
-		/* YUV -> RGB */
-		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
+
+	/* YUV -> RGB, ITU-R BT.601, limited range */
+	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
+		298,    0,  409,	/* ry, rcb, rcr */
+		298, -100, -208,	/* gy, gcb, gcr */
+		298,  516,    0,	/* by, bcb, bcr */
+		false,			/* limited range */
 	};
-	const struct color_conv_coef ctbl_bt601_5_wb = {
-		/* RGB -> YUV */
-		66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
+
+	/* RGB -> YUV, ITU-R BT.601, limited range */
+	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
+		 66, 129,  25,		/* yr,   yg,  yb */
+		-38, -74, 112,		/* cbr, cbg, cbb */
+		112, -94, -18,		/* crr, crg, crb */
+		false,			/* limited range */
 	};
 
 	for (i = 1; i < num_ovl; i++)
-		dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
+		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
 
 	if (dispc.feat->has_writeback)
-		dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, &ctbl_bt601_5_wb);
+		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
 }
 
 static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 24/24] drm/omap: cleanup color space conversion

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:54 EET Tomi Valkeinen wrote:
> The setup code for color space conversion is a bit messy. This patch
> cleans it up.
> 
> For some reason the TRM uses values in YCrCb order, which is also used
> in the current driver, whereas everywhere else it's YCbCr (which also
> matches YUV order). This patch changes the tables to use the common
> order to avoid confusion.
> 
> The tables are split into separate lines, and comments added for
> clarity.
> 
> WB color conversion registers are similar but different than non-WB, but
> the same function was used to write both. It worked fine because the
> coef table was adjusted accordingly, but that was rather confusing. This
> patch adds a separate function to write the WB values so that the coef
> table can be written in an understandable way.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 59 ++++++++++++++++++++++++++--------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index ff09e2be470f..697274317f7c
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -345,11 +345,6 @@ static const struct {
>  	},
>  };
> 
> -struct color_conv_coef {
> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
> -	int full_range;
> -};
> -
>  static unsigned long dispc_fclk_rate(void);
>  static unsigned long dispc_core_clk_rate(void);
>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
> @@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id
> plane, int fir_hinc, }
>  }
> 
> +struct csc_coef_yuv2rgb {
> +	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;

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

> +	bool full_range;
> +};
> +
> +struct csc_coef_rgb2yuv {
> +	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
> +	bool full_range;
> +};
> 
>  static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
> -		const struct color_conv_coef *ct)
> +		const struct csc_coef_yuv2rgb *ct)
>  {
>  #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> 
> @@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum
> omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
> CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
> CVAL(0, ct->bcb));
> 
> -	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);

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

> +
> +#undef CVAL
> +}
> +
> +static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
> *ct)
> +{
> +	const enum omap_plane_id plane = OMAP_DSS_WB;
> +
> +#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> +
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> +
> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> 
>  #undef CVAL
>  }
> @@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void)
>  {
>  	int i;
>  	int num_ovl = dispc_get_num_ovls();
> -	const struct color_conv_coef ctbl_bt601_5_ovl = {
> -		/* YUV -> RGB */
> -		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
> +
> +	/* YUV -> RGB, ITU-R BT.601, limited range */
> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> +		298,    0,  409,	/* ry, rcb, rcr */
> +		298, -100, -208,	/* gy, gcb, gcr */
> +		298,  516,    0,	/* by, bcb, bcr */

The 517 turned into 516, was that intentional ?

> +		false,			/* limited range */
>  	};
> -	const struct color_conv_coef ctbl_bt601_5_wb = {
> -		/* RGB -> YUV */
> -		66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
> +
> +	/* RGB -> YUV, ITU-R BT.601, limited range */
> +	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> +		 66, 129,  25,		/* yr,   yg,  yb */
> +		-38, -74, 112,		/* cbr, cbg, cbb */
> +		112, -94, -18,		/* crr, crg, crb */
> +		false,			/* limited range */
>  	};
> 
>  	for (i = 1; i < num_ovl; i++)
> -		dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
> +		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
> 
>  	if (dispc.feat->has_writeback)
> -		dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, &ctbl_bt601_5_wb);
> +		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
>  }
> 
>  static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 24/24] drm/omap: cleanup color space conversion

[Tomi Valkeinen]

On 27/02/18 16:08, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Monday, 12 February 2018 11:44:54 EET Tomi Valkeinen wrote:
>> The setup code for color space conversion is a bit messy. This patch
>> cleans it up.
>>
>> For some reason the TRM uses values in YCrCb order, which is also used
>> in the current driver, whereas everywhere else it's YCbCr (which also
>> matches YUV order). This patch changes the tables to use the common
>> order to avoid confusion.
>>
>> The tables are split into separate lines, and comments added for
>> clarity.
>>
>> WB color conversion registers are similar but different than non-WB, but
>> the same function was used to write both. It worked fine because the
>> coef table was adjusted accordingly, but that was rather confusing. This
>> patch adds a separate function to write the WB values so that the coef
>> table can be written in an understandable way.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c | 59 ++++++++++++++++++++++++++--------
>>  1 file changed, 44 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index ff09e2be470f..697274317f7c
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -345,11 +345,6 @@ static const struct {
>>  	},
>>  };
>>
>> -struct color_conv_coef {
>> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
>> -	int full_range;
>> -};
>> -
>>  static unsigned long dispc_fclk_rate(void);
>>  static unsigned long dispc_core_clk_rate(void);
>>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
>> @@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id
>> plane, int fir_hinc, }
>>  }
>>
>> +struct csc_coef_yuv2rgb {
>> +	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
> 
> If you made this a 3x3 matrix without explicit names for the fields I think 
> you wouldn't need two structure and two functions.

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

>> +	bool full_range;
>> +};
>> +
>> +struct csc_coef_rgb2yuv {
>> +	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
>> +	bool full_range;
>> +};
>>
>>  static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
>> -		const struct color_conv_coef *ct)
>> +		const struct csc_coef_yuv2rgb *ct)
>>  {
>>  #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
>>
>> @@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum
>> omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
>> CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
>> CVAL(0, ct->bcb));
>>
>> -	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
>> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> 
> true and false should be equal to 1 and 0 respectively, so the !! shouldn't be 
> needed.

Yep.

>> +
>> +#undef CVAL
>> +}
>> +
>> +static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
>> *ct)
>> +{
>> +	const enum omap_plane_id plane = OMAP_DSS_WB;
>> +
>> +#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
>> +
>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
>> +
>> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
>>
>>  #undef CVAL
>>  }
>> @@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void)
>>  {
>>  	int i;
>>  	int num_ovl = dispc_get_num_ovls();
>> -	const struct color_conv_coef ctbl_bt601_5_ovl = {
>> -		/* YUV -> RGB */
>> -		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
>> +
>> +	/* YUV -> RGB, ITU-R BT.601, limited range */
>> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
>> +		298,    0,  409,	/* ry, rcb, rcr */
>> +		298, -100, -208,	/* gy, gcb, gcr */
>> +		298,  516,    0,	/* by, bcb, bcr */
> 
> The 517 turned into 516, was that intentional ?

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

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

 Tomi

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

Re: [PATCH 24/24] drm/omap: cleanup color space conversion

[Laurent Pinchart]

Hi Tomi,

On Wednesday, 28 February 2018 12:09:35 EET Tomi Valkeinen wrote:
> On 27/02/18 16:08, Laurent Pinchart wrote:
> > On Monday, 12 February 2018 11:44:54 EET Tomi Valkeinen wrote:
> >> The setup code for color space conversion is a bit messy. This patch
> >> cleans it up.
> >> 
> >> For some reason the TRM uses values in YCrCb order, which is also used
> >> in the current driver, whereas everywhere else it's YCbCr (which also
> >> matches YUV order). This patch changes the tables to use the common
> >> order to avoid confusion.
> >> 
> >> The tables are split into separate lines, and comments added for
> >> clarity.
> >> 
> >> WB color conversion registers are similar but different than non-WB, but
> >> the same function was used to write both. It worked fine because the
> >> coef table was adjusted accordingly, but that was rather confusing. This
> >> patch adds a separate function to write the WB values so that the coef
> >> table can be written in an understandable way.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/dss/dispc.c | 59 ++++++++++++++++++++++++-------
> >>  1 file changed, 44 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> b/drivers/gpu/drm/omapdrm/dss/dispc.c index ff09e2be470f..697274317f7c
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -345,11 +345,6 @@ static const struct {
> >>  	},
 >>  };
> >> 
> >> -struct color_conv_coef {
> >> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
> >> -	int full_range;
> >> -};
> >> -
> >>  static unsigned long dispc_fclk_rate(void);
> >>  static unsigned long dispc_core_clk_rate(void);
> >>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
> >> @@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum
> >> omap_plane_id plane, int fir_hinc, }
> >> 
> >>  }
> >> 
> >> +struct csc_coef_yuv2rgb {
> >> +	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
> > 
> > If you made this a 3x3 matrix without explicit names for the fields I
> > think you wouldn't need two structure and two functions.
> 
> That is true, but I think it would also make the code more difficult to
> follow. It's not exactly obvious which coefficients go where.

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

> >> +	bool full_range;
> >> +};
> >> +
> >> +struct csc_coef_rgb2yuv {
> >> +	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
> >> +	bool full_range;
> >> +};
> >> 
> >>  static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
> >> 
> >> -		const struct color_conv_coef *ct)
> >> +		const struct csc_coef_yuv2rgb *ct)
> >> 
> >>  {
> >>  #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> >> 
> >> @@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum
> >> omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
> >> CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
> >> CVAL(0, ct->bcb));
> >> 
> >> -	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> >> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> > 
> > true and false should be equal to 1 and 0 respectively, so the !!
> > shouldn't be needed.
> 
> Yep.
> 
> >> +
> >> +#undef CVAL
> >> +}
> >> +
> >> +static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
> >> *ct)
> >> +{
> >> +	const enum omap_plane_id plane = OMAP_DSS_WB;
> >> +
> >> +#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> >> +
> >> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
> >> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
> >> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
> >> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
> >> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> >> +
> >> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> >>  #undef CVAL
> >>  }
> >> 
> >> @@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void)
> >>  {
> >>  	int i;
> >>  	int num_ovl = dispc_get_num_ovls();
> >> -	const struct color_conv_coef ctbl_bt601_5_ovl = {
> >> -		/* YUV -> RGB */
> >> -		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
> >> +
> >> +	/* YUV -> RGB, ITU-R BT.601, limited range */
> >> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> >> +		298,    0,  409,	/* ry, rcb, rcr */
> >> +		298, -100, -208,	/* gy, gcb, gcr */
> >> +		298,  516,    0,	/* by, bcb, bcr */
> > 
> > The 517 turned into 516, was that intentional ?
> 
> Good point. It's been a while, but I did recalculate these, and with
> rounding, all the other values match except the 517, so I changed it
> according to my calculations.
> 
> That said, I'm not sure if the original calculations are correct, as
> they seem to use 256 as multiplier, which would not result 0xff from 1.0
> value. In any case, I changed the single value that seemed to be off
> from the other ones.

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

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 24/24] drm/omap: cleanup color space conversion

[Tomi Valkeinen]

On 28/02/18 12:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wednesday, 28 February 2018 12:09:35 EET Tomi Valkeinen wrote:
>> On 27/02/18 16:08, Laurent Pinchart wrote:
>>> On Monday, 12 February 2018 11:44:54 EET Tomi Valkeinen wrote:
>>>> The setup code for color space conversion is a bit messy. This patch
>>>> cleans it up.
>>>>
>>>> For some reason the TRM uses values in YCrCb order, which is also used
>>>> in the current driver, whereas everywhere else it's YCbCr (which also
>>>> matches YUV order). This patch changes the tables to use the common
>>>> order to avoid confusion.
>>>>
>>>> The tables are split into separate lines, and comments added for
>>>> clarity.
>>>>
>>>> WB color conversion registers are similar but different than non-WB, but
>>>> the same function was used to write both. It worked fine because the
>>>> coef table was adjusted accordingly, but that was rather confusing. This
>>>> patch adds a separate function to write the WB values so that the coef
>>>> table can be written in an understandable way.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/omapdrm/dss/dispc.c | 59 ++++++++++++++++++++++++-------
>>>>  1 file changed, 44 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index ff09e2be470f..697274317f7c
>>>> 100644
>>>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>>>> @@ -345,11 +345,6 @@ static const struct {
>>>>  	},
>  >>  };
>>>>
>>>> -struct color_conv_coef {
>>>> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
>>>> -	int full_range;
>>>> -};
>>>> -
>>>>  static unsigned long dispc_fclk_rate(void);
>>>>  static unsigned long dispc_core_clk_rate(void);
>>>>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
>>>> @@ -841,9 +836,18 @@ static void dispc_ovl_set_scale_coef(enum
>>>> omap_plane_id plane, int fir_hinc, }
>>>>
>>>>  }
>>>>
>>>> +struct csc_coef_yuv2rgb {
>>>> +	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
>>>
>>> If you made this a 3x3 matrix without explicit names for the fields I
>>> think you wouldn't need two structure and two functions.
>>
>> That is true, but I think it would also make the code more difficult to
>> follow. It's not exactly obvious which coefficients go where.
> 
> I don't expect this code to be often read, and I think that with appropriate 
> comments in the source code and proper formatting of the table, as you do 
> below, it should be clear enough, but I'll leave it up to you. We're really 
> programming a 3x3 matrix in the hardware, so I think that modeling it as a 3x3 
> matrix in the driver makes sense :-)

Yep. Well, another reason to not change this here is that Jyri has made
a patch to add full range and bt.709 conversions, based on this one.
Changing the structs here would break his patch, and verifying these
conversions is somewhat time consuming.

I'm fine with revisiting the struct after Jyri's patch is also merged.

>>>> +	bool full_range;
>>>> +};
>>>> +
>>>> +struct csc_coef_rgb2yuv {
>>>> +	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
>>>> +	bool full_range;
>>>> +};
>>>>
>>>>  static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
>>>>
>>>> -		const struct color_conv_coef *ct)
>>>> +		const struct csc_coef_yuv2rgb *ct)
>>>>
>>>>  {
>>>>  #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
>>>>
>>>> @@ -853,7 +857,24 @@ static void dispc_ovl_write_color_conv_coef(enum
>>>> omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
>>>> CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
>>>> CVAL(0, ct->bcb));
>>>>
>>>> -	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
>>>> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
>>>
>>> true and false should be equal to 1 and 0 respectively, so the !!
>>> shouldn't be needed.
>>
>> Yep.
>>
>>>> +
>>>> +#undef CVAL
>>>> +}
>>>> +
>>>> +static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
>>>> *ct)
>>>> +{
>>>> +	const enum omap_plane_id plane = OMAP_DSS_WB;
>>>> +
>>>> +#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
>>>> +
>>>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
>>>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
>>>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
>>>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
>>>> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
>>>> +
>>>> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
>>>>  #undef CVAL
>>>>  }
>>>>
>>>> @@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void)
>>>>  {
>>>>  	int i;
>>>>  	int num_ovl = dispc_get_num_ovls();
>>>> -	const struct color_conv_coef ctbl_bt601_5_ovl = {
>>>> -		/* YUV -> RGB */
>>>> -		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
>>>> +
>>>> +	/* YUV -> RGB, ITU-R BT.601, limited range */
>>>> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
>>>> +		298,    0,  409,	/* ry, rcb, rcr */
>>>> +		298, -100, -208,	/* gy, gcb, gcr */
>>>> +		298,  516,    0,	/* by, bcb, bcr */
>>>
>>> The 517 turned into 516, was that intentional ?
>>
>> Good point. It's been a while, but I did recalculate these, and with
>> rounding, all the other values match except the 517, so I changed it
>> according to my calculations.
>>
>> That said, I'm not sure if the original calculations are correct, as
>> they seem to use 256 as multiplier, which would not result 0xff from 1.0
>> value. In any case, I changed the single value that seemed to be off
>> from the other ones.
> 
> I have no issue with that, but could you please mention it in the commit 
> message ?

Agreed, done.

 Tomi

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