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

[PATCH] 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.

We should have all crtcs in 'possible_crtc', but apparently it's not as
easy to set as you would think. We create crtcs rather dynamically, and
when creating the primary planes, we don't know how many crtcs we're
going to have. This is mostly a problem with the way omapdrm creates
crtcs and planes, and how it connects those to display outputs.

So, this patch fixes the problem the easy way, and sets the
possible_crtcs for primary planes only to the crtc in question, which in
practice should cover all normal use cases.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 9c43cb481e62..fc1822870b26 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -361,6 +361,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_plane *plane;
 	struct omap_plane *omap_plane;
+	unsigned long possible_crtcs;
 	int ret;
 
 	DBG("%s: type=%d", plane_names[id], type);
@@ -381,7 +382,12 @@ 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,
+	if (type == DRM_PLANE_TYPE_PRIMARY)
+		possible_crtcs = 1 << id;
+	else
+		possible_crtcs = (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] drm/omap: fix primary-plane's possible_crtcs

[Ville Syrjälä]

On Wed, Nov 30, 2016 at 12:38:51PM +0200, 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.
> 
> We should have all crtcs in 'possible_crtc', but apparently it's not as
> easy to set as you would think. We create crtcs rather dynamically, and
> when creating the primary planes, we don't know how many crtcs we're
> going to have. This is mostly a problem with the way omapdrm creates
> crtcs and planes, and how it connects those to display outputs.
> 
> So, this patch fixes the problem the easy way, and sets the
> possible_crtcs for primary planes only to the crtc in question, which in
> practice should cover all normal use cases.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_plane.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 9c43cb481e62..fc1822870b26 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -361,6 +361,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>  	struct omap_drm_private *priv = dev->dev_private;
>  	struct drm_plane *plane;
>  	struct omap_plane *omap_plane;
> +	unsigned long possible_crtcs;

Drive-by-comment:
unsigned long? Hmm. Apparently that is what this guy wants, and then
it proceeds to stuff it into a u32. Maybe someone could fix the
function to take a u32 instead...

>  	int ret;
>  
>  	DBG("%s: type=%d", plane_names[id], type);
> @@ -381,7 +382,12 @@ 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,
> +	if (type == DRM_PLANE_TYPE_PRIMARY)
> +		possible_crtcs = 1 << id;
> +	else
> +		possible_crtcs = (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

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

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

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 30 Nov 2016 12:38:51 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.
> 
> We should have all crtcs in 'possible_crtc', but apparently it's not as
> easy to set as you would think. We create crtcs rather dynamically, and
> when creating the primary planes, we don't know how many crtcs we're
> going to have. This is mostly a problem with the way omapdrm creates
> crtcs and planes, and how it connects those to display outputs.
> 
> So, this patch fixes the problem the easy way, and sets the
> possible_crtcs for primary planes only to the crtc in question, which in
> practice should cover all normal use cases.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_plane.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 9c43cb481e62..fc1822870b26
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -361,6 +361,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev, struct omap_drm_private *priv = dev->dev_private;
>  	struct drm_plane *plane;
>  	struct omap_plane *omap_plane;
> +	unsigned long possible_crtcs;
>  	int ret;
> 
>  	DBG("%s: type=%d", plane_names[id], type);
> @@ -381,7 +382,12 @@ 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,
> +	if (type == DRM_PLANE_TYPE_PRIMARY)
> +		possible_crtcs = 1 << id;
> +	else
> +		possible_crtcs = (1 << priv->num_crtcs) - 1;

The omap_modeset_init() function computes the number of CRTCs before creating 
any plane with

	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);

and the rest of the function then creates CRTCs, as far as I understand always 
up to num_crtcs. Can't we just use that value to compute possible_crtcs as (1 
<< 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: [PATCH] drm/omap: fix primary-plane's possible_crtcs

[Tomi Valkeinen]

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

On 30/11/16 16:36, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wednesday 30 Nov 2016 12:38:51 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.
>>
>> We should have all crtcs in 'possible_crtc', but apparently it's not as
>> easy to set as you would think. We create crtcs rather dynamically, and
>> when creating the primary planes, we don't know how many crtcs we're
>> going to have. This is mostly a problem with the way omapdrm creates
>> crtcs and planes, and how it connects those to display outputs.
>>
>> So, this patch fixes the problem the easy way, and sets the
>> possible_crtcs for primary planes only to the crtc in question, which in
>> practice should cover all normal use cases.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_plane.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>> b/drivers/gpu/drm/omapdrm/omap_plane.c index 9c43cb481e62..fc1822870b26
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -361,6 +361,7 @@ struct drm_plane *omap_plane_init(struct drm_device
>> *dev, struct omap_drm_private *priv = dev->dev_private;
>>  	struct drm_plane *plane;
>>  	struct omap_plane *omap_plane;
>> +	unsigned long possible_crtcs;
>>  	int ret;
>>
>>  	DBG("%s: type=%d", plane_names[id], type);
>> @@ -381,7 +382,12 @@ 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,
>> +	if (type == DRM_PLANE_TYPE_PRIMARY)
>> +		possible_crtcs = 1 << id;
>> +	else
>> +		possible_crtcs = (1 << priv->num_crtcs) - 1;
> 
> The omap_modeset_init() function computes the number of CRTCs before creating 
> any plane with
> 
> 	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);
> 
> and the rest of the function then creates CRTCs, as far as I understand always 
> up to num_crtcs. Can't we just use that value to compute possible_crtcs as (1 
> << num_crtcs ) - 1 ?

Yes, I think you're correct. It's not exactly obvious from the code =).
I'll change the patch to use the calculated num-crtcs.

 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