| Message ID | 1480502331-6635-1-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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)
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
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)
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(-)