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

[Tomi Valkeinen]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161202/b0084dd5/attachment.sig>