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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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