[PATCH 1/2] drm: fix possible_crtc's type

[PATCH 1/2] drm: fix possible_crtc's type

[Tomi Valkeinen]

drm_universal_plane_init() and drm_plane_init() take "unsigned long
possible_crtcs" parameter, but then stuff it into uint32_t. Change the
parameter to uint32_t.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 4 ++--
 include/drm/drm_plane.h     | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 419ac313c36f..3bf9276f5bcf 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -79,7 +79,7 @@ static unsigned int drm_num_planes(struct drm_device *dev)
  * Zero on success, error code on failure.
  */
 int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
-			     unsigned long possible_crtcs,
+			     uint32_t possible_crtcs,
 			     const struct drm_plane_funcs *funcs,
 			     const uint32_t *formats, unsigned int format_count,
 			     enum drm_plane_type type,
@@ -196,7 +196,7 @@ void drm_plane_unregister_all(struct drm_device *dev)
  * Zero on success, error code on failure.
  */
 int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
-		   unsigned long possible_crtcs,
+		   uint32_t possible_crtcs,
 		   const struct drm_plane_funcs *funcs,
 		   const uint32_t *formats, unsigned int format_count,
 		   bool is_primary)
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 5b38eb94783b..db3bbdeb36d5 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -513,7 +513,7 @@ struct drm_plane {
 extern __printf(8, 9)
 int drm_universal_plane_init(struct drm_device *dev,
 			     struct drm_plane *plane,
-			     unsigned long possible_crtcs,
+			     uint32_t possible_crtcs,
 			     const struct drm_plane_funcs *funcs,
 			     const uint32_t *formats,
 			     unsigned int format_count,
@@ -521,7 +521,7 @@ int drm_universal_plane_init(struct drm_device *dev,
 			     const char *name, ...);
 extern int drm_plane_init(struct drm_device *dev,
 			  struct drm_plane *plane,
-			  unsigned long possible_crtcs,
+			  uint32_t possible_crtcs,
 			  const struct drm_plane_funcs *funcs,
 			  const uint32_t *formats, unsigned int format_count,
 			  bool is_primary);
-- 
2.7.4

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

[PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Tomi Valkeinen]

We set the possible_crtc for all planes to "(1 << priv->num_crtcs) - 1",
which is fine as the HW planes can be used fro all crtcs. However, when
we're doing that, we are still incrementing 'num_crtcs', and we'll end
up with bad possible_crtcs, preventing the use of the primary planes.

This patch passes a possible_crtcs mask to plane init function so that
we get correct possible_crtc.

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

v2: use correct possible_crtcs value

 drivers/gpu/drm/omapdrm/omap_drv.c   | 17 ++++++++++++-----
 drivers/gpu/drm/omapdrm/omap_drv.h   |  3 ++-
 drivers/gpu/drm/omapdrm/omap_plane.c |  6 +++---
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 39c5312b466c..fdc83cbcde61 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -267,13 +267,15 @@ static int omap_connect_dssdevs(void)
 }
 
 static int omap_modeset_create_crtc(struct drm_device *dev, int id,
-				    enum omap_channel channel)
+				    enum omap_channel channel,
+				    u32 possible_crtcs)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 
-	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY);
+	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY,
+		possible_crtcs);
 	if (IS_ERR(plane))
 		return PTR_ERR(plane);
 
@@ -309,6 +311,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_crtcs;
 	int i, id = 0;
 	int ret;
+	u32 possible_crtcs;
 
 	drm_mode_config_init(dev);
 
@@ -325,6 +328,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	 * We use the num_crtc argument to limit the number of crtcs we create.
 	 */
 	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);
+	possible_crtcs = (1 << num_crtcs) - 1;
 
 	dssdev = NULL;
 
@@ -388,7 +392,8 @@ static int omap_modeset_init(struct drm_device *dev)
 		 * allocated crtc, we create a new crtc for it
 		 */
 		if (!channel_used(dev, channel)) {
-			ret = omap_modeset_create_crtc(dev, id, channel);
+			ret = omap_modeset_create_crtc(dev, id, channel,
+				possible_crtcs);
 			if (ret < 0) {
 				dev_err(dev->dev,
 					"could not create CRTC (channel %u)\n",
@@ -418,7 +423,8 @@ static int omap_modeset_init(struct drm_device *dev)
 			return -ENOMEM;
 		}
 
-		ret = omap_modeset_create_crtc(dev, id, i);
+		ret = omap_modeset_create_crtc(dev, id, i,
+			possible_crtcs);
 		if (ret < 0) {
 			dev_err(dev->dev,
 				"could not create CRTC (channel %u)\n", i);
@@ -432,7 +438,8 @@ static int omap_modeset_init(struct drm_device *dev)
 	for (; id < num_ovls; id++) {
 		struct drm_plane *plane;
 
-		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY);
+		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY,
+			possible_crtcs);
 		if (IS_ERR(plane))
 			return PTR_ERR(plane);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 4c51135eb9a6..7d9dd5400cef 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -157,7 +157,8 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 int omap_crtc_wait_pending(struct drm_crtc *crtc);
 
 struct drm_plane *omap_plane_init(struct drm_device *dev,
-		int id, enum drm_plane_type type);
+		int id, enum drm_plane_type type,
+		u32 possible_crtcs);
 void omap_plane_install_properties(struct drm_plane *plane,
 		struct drm_mode_object *obj);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 9c43cb481e62..82b2c23d6769 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -356,9 +356,9 @@ static const uint32_t error_irqs[] = {
 
 /* initialize plane */
 struct drm_plane *omap_plane_init(struct drm_device *dev,
-		int id, enum drm_plane_type type)
+		int id, enum drm_plane_type type,
+		u32 possible_crtcs)
 {
-	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_plane *plane;
 	struct omap_plane *omap_plane;
 	int ret;
@@ -381,7 +381,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	omap_plane->error_irq.irq = omap_plane_error_irq;
 	omap_irq_register(dev, &omap_plane->error_irq);
 
-	ret = drm_universal_plane_init(dev, plane, (1 << priv->num_crtcs) - 1,
+	ret = drm_universal_plane_init(dev, plane, possible_crtcs,
 				       &omap_plane_funcs, omap_plane->formats,
 				       omap_plane->nformats, type, NULL);
 	if (ret < 0)
-- 
2.7.4

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

Re: [PATCH 1/2] drm: fix possible_crtc's type

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 02 Dec 2016 16:07:10 Tomi Valkeinen wrote:
> drm_universal_plane_init() and drm_plane_init() take "unsigned long
> possible_crtcs" parameter, but then stuff it into uint32_t. Change the
> parameter to uint32_t.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/drm_plane.c | 4 ++--
>  include/drm/drm_plane.h     | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 419ac313c36f..3bf9276f5bcf 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -79,7 +79,7 @@ static unsigned int drm_num_planes(struct drm_device *dev)
> * Zero on success, error code on failure.
>   */
>  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane
> *plane, -			     unsigned long possible_crtcs,
> +			     uint32_t possible_crtcs,
>  			     const struct drm_plane_funcs *funcs,
>  			     const uint32_t *formats, unsigned int 
format_count,
>  			     enum drm_plane_type type,
> @@ -196,7 +196,7 @@ void drm_plane_unregister_all(struct drm_device *dev)
>   * Zero on success, error code on failure.
>   */
>  int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
> -		   unsigned long possible_crtcs,
> +		   uint32_t possible_crtcs,
>  		   const struct drm_plane_funcs *funcs,
>  		   const uint32_t *formats, unsigned int format_count,
>  		   bool is_primary)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 5b38eb94783b..db3bbdeb36d5 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -513,7 +513,7 @@ struct drm_plane {
>  extern __printf(8, 9)
>  int drm_universal_plane_init(struct drm_device *dev,
>  			     struct drm_plane *plane,
> -			     unsigned long possible_crtcs,
> +			     uint32_t possible_crtcs,
>  			     const struct drm_plane_funcs *funcs,
>  			     const uint32_t *formats,
>  			     unsigned int format_count,
> @@ -521,7 +521,7 @@ int drm_universal_plane_init(struct drm_device *dev,
>  			     const char *name, ...);
>  extern int drm_plane_init(struct drm_device *dev,
>  			  struct drm_plane *plane,
> -			  unsigned long possible_crtcs,
> +			  uint32_t possible_crtcs,
>  			  const struct drm_plane_funcs *funcs,
>  			  const uint32_t *formats, unsigned int format_count,
>  			  bool is_primary);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] drm: fix possible_crtc's type

[Ville Syrjälä]

On Fri, Dec 02, 2016 at 04:07:10PM +0200, Tomi Valkeinen wrote:
> drm_universal_plane_init() and drm_plane_init() take "unsigned long
> possible_crtcs" parameter, but then stuff it into uint32_t. Change the
> parameter to uint32_t.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_plane.c | 4 ++--
>  include/drm/drm_plane.h     | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 419ac313c36f..3bf9276f5bcf 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -79,7 +79,7 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>   * Zero on success, error code on failure.
>   */
>  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> -			     unsigned long possible_crtcs,
> +			     uint32_t possible_crtcs,
>  			     const struct drm_plane_funcs *funcs,
>  			     const uint32_t *formats, unsigned int format_count,
>  			     enum drm_plane_type type,
> @@ -196,7 +196,7 @@ void drm_plane_unregister_all(struct drm_device *dev)
>   * Zero on success, error code on failure.
>   */
>  int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
> -		   unsigned long possible_crtcs,
> +		   uint32_t possible_crtcs,
>  		   const struct drm_plane_funcs *funcs,
>  		   const uint32_t *formats, unsigned int format_count,
>  		   bool is_primary)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 5b38eb94783b..db3bbdeb36d5 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -513,7 +513,7 @@ struct drm_plane {
>  extern __printf(8, 9)
>  int drm_universal_plane_init(struct drm_device *dev,
>  			     struct drm_plane *plane,
> -			     unsigned long possible_crtcs,
> +			     uint32_t possible_crtcs,
>  			     const struct drm_plane_funcs *funcs,
>  			     const uint32_t *formats,
>  			     unsigned int format_count,
> @@ -521,7 +521,7 @@ int drm_universal_plane_init(struct drm_device *dev,
>  			     const char *name, ...);
>  extern int drm_plane_init(struct drm_device *dev,
>  			  struct drm_plane *plane,
> -			  unsigned long possible_crtcs,
> +			  uint32_t possible_crtcs,
>  			  const struct drm_plane_funcs *funcs,
>  			  const uint32_t *formats, unsigned int format_count,
>  			  bool is_primary);
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 02 Dec 2016 16:07:11 Tomi Valkeinen wrote:
> We set the possible_crtc for all planes to "(1 << priv->num_crtcs) - 1",
> which is fine as the HW planes can be used fro all crtcs. However, when
> we're doing that, we are still incrementing 'num_crtcs', and we'll end
> up with bad possible_crtcs, preventing the use of the primary planes.
> 
> This patch passes a possible_crtcs mask to plane init function so that
> we get correct possible_crtc.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> 
> v2: use correct possible_crtcs value
> 
>  drivers/gpu/drm/omapdrm/omap_drv.c   | 17 ++++++++++++-----
>  drivers/gpu/drm/omapdrm/omap_drv.h   |  3 ++-
>  drivers/gpu/drm/omapdrm/omap_plane.c |  6 +++---
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 39c5312b466c..fdc83cbcde61
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -267,13 +267,15 @@ static int omap_connect_dssdevs(void)
>  }
> 
>  static int omap_modeset_create_crtc(struct drm_device *dev, int id,
> -				    enum omap_channel channel)
> +				    enum omap_channel channel,
> +				    u32 possible_crtcs)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc;
> 
> -	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY);
> +	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY,
> +		possible_crtcs);
>  	if (IS_ERR(plane))
>  		return PTR_ERR(plane);

If you removed the priv->num_crtcs++ a bit below in this function...

> @@ -309,6 +311,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  	int num_crtcs;
>  	int i, id = 0;
>  	int ret;
> +	u32 possible_crtcs;
> 
>  	drm_mode_config_init(dev);
> 
> @@ -325,6 +328,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  	 * We use the num_crtc argument to limit the number of crtcs we 
create.
>  	 */
>  	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);

and assigned priv->num_crtcs here and replaced the channel_used() function 
with a simple bitmask private to omap_modeset_init() you would end up with a 
much simpler implementation that wouldn't require passing possible_crtcs 
through a bunch of functions.

> +	possible_crtcs = (1 << num_crtcs) - 1;
> 
>  	dssdev = NULL;
> 
> @@ -388,7 +392,8 @@ static int omap_modeset_init(struct drm_device *dev)
>  		 * allocated crtc, we create a new crtc for it
>  		 */
>  		if (!channel_used(dev, channel)) {
> -			ret = omap_modeset_create_crtc(dev, id, channel);
> +			ret = omap_modeset_create_crtc(dev, id, channel,
> +				possible_crtcs);
>  			if (ret < 0) {
>  				dev_err(dev->dev,
>  					"could not create CRTC (channel 
%u)\n",
> @@ -418,7 +423,8 @@ static int omap_modeset_init(struct drm_device *dev)
>  			return -ENOMEM;
>  		}
> 
> -		ret = omap_modeset_create_crtc(dev, id, i);
> +		ret = omap_modeset_create_crtc(dev, id, i,
> +			possible_crtcs);
>  		if (ret < 0) {
>  			dev_err(dev->dev,
>  				"could not create CRTC (channel %u)\n", i);
> @@ -432,7 +438,8 @@ static int omap_modeset_init(struct drm_device *dev)
>  	for (; id < num_ovls; id++) {
>  		struct drm_plane *plane;
> 
> -		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY);
> +		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY,
> +			possible_crtcs);
>  		if (IS_ERR(plane))
>  			return PTR_ERR(plane);
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> b/drivers/gpu/drm/omapdrm/omap_drv.h index 4c51135eb9a6..7d9dd5400cef
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -157,7 +157,8 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  int omap_crtc_wait_pending(struct drm_crtc *crtc);
> 
>  struct drm_plane *omap_plane_init(struct drm_device *dev,
> -		int id, enum drm_plane_type type);
> +		int id, enum drm_plane_type type,
> +		u32 possible_crtcs);
>  void omap_plane_install_properties(struct drm_plane *plane,
>  		struct drm_mode_object *obj);
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 9c43cb481e62..82b2c23d6769
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -356,9 +356,9 @@ static const uint32_t error_irqs[] = {
> 
>  /* initialize plane */
>  struct drm_plane *omap_plane_init(struct drm_device *dev,
> -		int id, enum drm_plane_type type)
> +		int id, enum drm_plane_type type,
> +		u32 possible_crtcs)
>  {
> -	struct omap_drm_private *priv = dev->dev_private;
>  	struct drm_plane *plane;
>  	struct omap_plane *omap_plane;
>  	int ret;
> @@ -381,7 +381,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev, omap_plane->error_irq.irq = omap_plane_error_irq;
>  	omap_irq_register(dev, &omap_plane->error_irq);
> 
> -	ret = drm_universal_plane_init(dev, plane, (1 << priv->num_crtcs) - 1,
> +	ret = drm_universal_plane_init(dev, plane, possible_crtcs,
>  				       &omap_plane_funcs, omap_plane->formats,
>  				       omap_plane->nformats, type, NULL);
>  	if (ret < 0)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 3007 bytes --]

On 02/12/16 16:16, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 02 Dec 2016 16:07:11 Tomi Valkeinen wrote:
>> We set the possible_crtc for all planes to "(1 << priv->num_crtcs) - 1",
>> which is fine as the HW planes can be used fro all crtcs. However, when
>> we're doing that, we are still incrementing 'num_crtcs', and we'll end
>> up with bad possible_crtcs, preventing the use of the primary planes.
>>
>> This patch passes a possible_crtcs mask to plane init function so that
>> we get correct possible_crtc.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>
>> v2: use correct possible_crtcs value
>>
>>  drivers/gpu/drm/omapdrm/omap_drv.c   | 17 ++++++++++++-----
>>  drivers/gpu/drm/omapdrm/omap_drv.h   |  3 ++-
>>  drivers/gpu/drm/omapdrm/omap_plane.c |  6 +++---
>>  3 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> b/drivers/gpu/drm/omapdrm/omap_drv.c index 39c5312b466c..fdc83cbcde61
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -267,13 +267,15 @@ static int omap_connect_dssdevs(void)
>>  }
>>
>>  static int omap_modeset_create_crtc(struct drm_device *dev, int id,
>> -				    enum omap_channel channel)
>> +				    enum omap_channel channel,
>> +				    u32 possible_crtcs)
>>  {
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	struct drm_plane *plane;
>>  	struct drm_crtc *crtc;
>>
>> -	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY);
>> +	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY,
>> +		possible_crtcs);
>>  	if (IS_ERR(plane))
>>  		return PTR_ERR(plane);
> 
> If you removed the priv->num_crtcs++ a bit below in this function...
> 
>> @@ -309,6 +311,7 @@ static int omap_modeset_init(struct drm_device *dev)
>>  	int num_crtcs;
>>  	int i, id = 0;
>>  	int ret;
>> +	u32 possible_crtcs;
>>
>>  	drm_mode_config_init(dev);
>>
>> @@ -325,6 +328,7 @@ static int omap_modeset_init(struct drm_device *dev)
>>  	 * We use the num_crtc argument to limit the number of crtcs we 
> create.
>>  	 */
>>  	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);
> 
> and assigned priv->num_crtcs here and replaced the channel_used() function 
> with a simple bitmask private to omap_modeset_init() you would end up with a 
> much simpler implementation that wouldn't require passing possible_crtcs 
> through a bunch of functions.

Yes, I almost did that.

But priv-num_crtcs tells the amount of crtcs in priv->crtcs. If we set
priv->num_crtcs before actually creating those crtcs, I fear we could
easily create a bug. The crtc+plane creation code is not the shortest
one, so even if we wouldn't have a bug right now, I imagine it could be
easy to write a helper func or such later, which uses priv->num_crtcs,
and which gets called at some point when creating crtcs and planes...

So I went the safe way.

 Tomi


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

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

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

Re: [PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Laurent Pinchart]

Hi Tomi,

On Friday 02 Dec 2016 16:22:18 Tomi Valkeinen wrote:
> On 02/12/16 16:16, Laurent Pinchart wrote:
> > On Friday 02 Dec 2016 16:07:11 Tomi Valkeinen wrote:
> >> We set the possible_crtc for all planes to "(1 << priv->num_crtcs) - 1",
> >> which is fine as the HW planes can be used fro all crtcs. However, when
> >> we're doing that, we are still incrementing 'num_crtcs', and we'll end
> >> up with bad possible_crtcs, preventing the use of the primary planes.
> >> 
> >> This patch passes a possible_crtcs mask to plane init function so that
> >> we get correct possible_crtc.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >> v2: use correct possible_crtcs value
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_drv.c   | 17 ++++++++++++-----
> >>  drivers/gpu/drm/omapdrm/omap_drv.h   |  3 ++-
> >>  drivers/gpu/drm/omapdrm/omap_plane.c |  6 +++---
> >>  3 files changed, 17 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> b/drivers/gpu/drm/omapdrm/omap_drv.c index 39c5312b466c..fdc83cbcde61
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> @@ -267,13 +267,15 @@ static int omap_connect_dssdevs(void)
> >>  }
> >>  
> >>  static int omap_modeset_create_crtc(struct drm_device *dev, int id,
> >> -				    enum omap_channel channel)
> >> +				    enum omap_channel channel,
> >> +				    u32 possible_crtcs)
> >>  {
> >>  	struct omap_drm_private *priv = dev->dev_private;
> >>  	struct drm_plane *plane;
> >>  	struct drm_crtc *crtc;
> >> 
> >> -	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY);
> >> +	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY,
> >> +		possible_crtcs);
> >>  	if (IS_ERR(plane))
> >>  		return PTR_ERR(plane);
> > 
> > If you removed the priv->num_crtcs++ a bit below in this function...
> > 
> >> @@ -309,6 +311,7 @@ static int omap_modeset_init(struct drm_device *dev)
> >>  	int num_crtcs;
> >>  	int i, id = 0;
> >>  	int ret;
> >> +	u32 possible_crtcs;
> >> 
> >>  	drm_mode_config_init(dev);
> >> 
> >> @@ -325,6 +328,7 @@ static int omap_modeset_init(struct drm_device *dev)
> >>  	 * We use the num_crtc argument to limit the number of crtcs we
> >> create.
> >>  	 */
> >>  	
> >>  	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);
> > 
> > and assigned priv->num_crtcs here and replaced the channel_used() function
> > with a simple bitmask private to omap_modeset_init() you would end up with
> > a much simpler implementation that wouldn't require passing
> > possible_crtcs through a bunch of functions.
> 
> Yes, I almost did that.
> 
> But priv-num_crtcs tells the amount of crtcs in priv->crtcs. If we set
> priv->num_crtcs before actually creating those crtcs, I fear we could
> easily create a bug. The crtc+plane creation code is not the shortest
> one, so even if we wouldn't have a bug right now, I imagine it could be
> easy to write a helper func or such later, which uses priv->num_crtcs,
> and which gets called at some point when creating crtcs and planes...
> 
> So I went the safe way.

I can understand that (even if I'm not sure it's really an issue, and we 
should really clean up the CRTC creation code at some point), but how about 
adding a possible_crtcs field to the priv structure then ? I don't really like 
having to pass it around through a bunch of functions.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 675 bytes --]

On 02/12/16 17:42, Laurent Pinchart wrote:

> I can understand that (even if I'm not sure it's really an issue, and we 
> should really clean up the CRTC creation code at some point), but how about 
> adding a possible_crtcs field to the priv structure then ? I don't really like 
> having to pass it around through a bunch of functions.

It is passed to two functions, I'm not sure if that's a bunch =).

I can do as you suggest, but I don't like adding fields to structs for
things that we only need once. I think local variables and function
parameters are for that.

But I agree that the patch would be quite a bit smaller with the field,
so...

 Tomi


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

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

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

Re: [PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Laurent Pinchart]

Hi Tomi,

On Friday 02 Dec 2016 17:55:10 Tomi Valkeinen wrote:
> On 02/12/16 17:42, Laurent Pinchart wrote:
> > I can understand that (even if I'm not sure it's really an issue, and we
> > should really clean up the CRTC creation code at some point), but how
> > about adding a possible_crtcs field to the priv structure then ? I don't
> > really like having to pass it around through a bunch of functions.
> 
> It is passed to two functions, I'm not sure if that's a bunch =).
> 
> I can do as you suggest, but I don't like adding fields to structs for
> things that we only need once.

I'm not too fond of that either, hence my first suggestion :-)

> I think local variables and function parameters are for that.
> 
> But I agree that the patch would be quite a bit smaller with the field,
> so...

I won't nack any solution you end up selecting even if I have my preferences.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2 2/2] drm/omap: fix primary-plane's possible_crtcs

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1192 bytes --]

On 02/12/16 17:56, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Friday 02 Dec 2016 17:55:10 Tomi Valkeinen wrote:
>> On 02/12/16 17:42, Laurent Pinchart wrote:
>>> I can understand that (even if I'm not sure it's really an issue, and we
>>> should really clean up the CRTC creation code at some point), but how
>>> about adding a possible_crtcs field to the priv structure then ? I don't
>>> really like having to pass it around through a bunch of functions.
>>
>> It is passed to two functions, I'm not sure if that's a bunch =).
>>
>> I can do as you suggest, but I don't like adding fields to structs for
>> things that we only need once.
> 
> I'm not too fond of that either, hence my first suggestion :-)
> 
>> I think local variables and function parameters are for that.
>>
>> But I agree that the patch would be quite a bit smaller with the field,
>> so...
> 
> I won't nack any solution you end up selecting even if I have my preferences.

I'll go with the posted v2. I agree it's not perfect, but every other
option I've tried also feels bad =). I think the only good solution is
to rewrite the init parts, but I don't want to do that for a fix.

 Tomi


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

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

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