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