[PATCH v4] drm/omap: plane zpos/zorder management improvements

[Peter Ujfalusi]

Hi Laurent,

On 2018-01-05 16:04, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch and sorry for the late review.
> 
> On Friday, 5 January 2018 13:30:37 EET Peter Ujfalusi wrote:
>> Use the plane index as default zpos for all planes. Even if the
>> application is not setting zpos/zorder explicitly we will have unique zpos
>> for each plane.
>>
>> Enforce that all planes must have unique zpos on the given crtc.
> 
> Could you explain the rationale for that in the commit message, what's wrong 
> with duplicate zpos values ?

Planes with identical zpos is only 'valid' _if_ they are not
overlapping, if they do overlap then it is - imho - not a valid
configuration anyway (which one should be on top?).
Based on my tests the plane with lower planeID is going to disappear
from the screen if we have duplicated zpos.

> Isn't there a risk of breaking the non-atomic userspace with this ? Without 
> atomic commits userspace can't change the zpos of multiple planes in one go, 
> so it might be impossible to reorder planes without going through a state that 
> has duplicated zpos values.

Two planes occupying the same position on the screen is not valid
(again, imho). If application wants to swap two planes, then it must
disable one, move the other to the new position, then enable and move
the first plane.

> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi at ti.com>
>> ---
>> Hi,
>>
>> Changes since v3:
>> - Use drm_plane_index() instead of storing the same index wothin omap_plane
>>   struct
>> - Optimize the zpos validation loop so we avoid extra checks.
>>
>> Changes since v2:
>> - The check for duplicate zpos is moved to omap_crtc
>>
>> Changes since v1:
>> - Dropped the zpos normalization related patches
>> - New patch based on the discussion, see commit message.
>>
>> Regards,
>> Peter
>>
>>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 36 ++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/omapdrm/omap_plane.c | 15 ++++-----------
>>  2 files changed, 39 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index cc85c16cbc2a..c68e3a4f5b7d
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -452,6 +452,40 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc
>> *crtc) }
>>  }
>>
>> +static int omap_crtc_validate_zpos(struct drm_crtc *crtc,
>> +				   struct drm_crtc_state *state)
>> +{
>> +	struct drm_device *ddev = crtc->dev;
>> +	struct drm_plane *p1;
>> +	const struct drm_plane_state *pstate1;
>> +
>> +	/* Check the crts's planes against duplicated zpos value */
>> +	drm_atomic_crtc_state_for_each_plane_state(p1, pstate1, state) {
>> +		struct drm_plane *p2 = p1;
>> +
>> +		list_for_each_entry_continue(p2, &ddev->mode_config.plane_list,
>> +					     head) {
>> +			const struct drm_plane_state *pstate2;
>> +
>> +			if (!(state->plane_mask & (1 << drm_plane_index(p2))))
>> +				continue;
>> +
>> +			pstate2 = __drm_atomic_get_current_plane_state(
>> +							state->state, p2);
>> +			if (!pstate2)
>> +				continue;
>> +
>> +			if (pstate1->zpos == pstate2->zpos) {
>> +				DBG("crtc%u: zpos must be unique (zpos: %u)",
>> +				crtc->index, pstate1->zpos);
>> +				return -EINVAL;
>> +			}
>> +		}
>> +	}
> 
> That seems a bit complicated to me. Couldn't we use a single loop and a zpos 
> bitfield ? Something like the following (untested) code.
> 
> 	struct drm_device *ddev = crtc->dev;
> 	const struct drm_plane_state *plane_state;
> 	struct drm_plane *plane;
> 	unsigned int zpos_mask = 0;
> 
> 	/* Check the crts's planes against duplicated zpos value */
> 	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state) {
> 		if (zpos_mask & BIT(plane_state->zpos)) {
> 			DBG("crtc%u: zpos must be unique (zpos: %u)",
> 			    crtc->index, plane_state->zpos);
> 			return -EINVAL;
> 		}
> 
> 		zpos_mask |= BIT(plane_state->zpos); 
> 	}
> 
> 	return 0;

Yes, it is much simpler.

> 
>> +	return 0;
>> +}
>> +
>>  static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>>  				struct drm_crtc_state *state)
>>  {
>> @@ -475,7 +509,7 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>> omap_crtc_state->rotation = pri_state->rotation;
>>  	}
>>
>> -	return 0;
>> +	return omap_crtc_validate_zpos(crtc, state);
>>  }
>>
>>  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>> b/drivers/gpu/drm/omapdrm/omap_plane.c index 15e5d5d325c6..d0057a23a5ac
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -99,8 +99,7 @@ static void omap_plane_atomic_disable(struct drm_plane
>> *plane, struct omap_plane *omap_plane = to_omap_plane(plane);
>>
>>  	plane->state->rotation = DRM_MODE_ROTATE_0;
>> -	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>> -			   ? 0 : omap_plane->id;
>> +	plane->state->zpos = drm_plane_index(plane);
>>
>>  	priv->dispc_ops->ovl_enable(omap_plane->id, false);
>>  }
>> @@ -186,18 +185,12 @@ void omap_plane_install_properties(struct drm_plane
>> *plane,
>>
>>  static void omap_plane_reset(struct drm_plane *plane)
>>  {
>> -	struct omap_plane *omap_plane = to_omap_plane(plane);
>> -
>>  	drm_atomic_helper_plane_reset(plane);
>>  	if (!plane->state)
>>  		return;
>>
>> -	/*
>> -	 * Set the zpos default depending on whether we are a primary or overlay
>> -	 * plane.
>> -	 */
>> -	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>> -			   ? 0 : omap_plane->id;
>> +	/* Set the zpos to the plane index. */
>> +	plane->state->zpos = drm_plane_index(plane);
>>  }
>>
>>  static int omap_plane_atomic_set_property(struct drm_plane *plane,
>> @@ -297,7 +290,7 @@ struct drm_plane *omap_plane_init(struct drm_device
>> *dev, drm_plane_helper_add(plane, &omap_plane_helper_funcs);
>>
>>  	omap_plane_install_properties(plane, &plane->base);
>> -	drm_plane_create_zpos_property(plane, 0, 0, num_planes - 1);
>> +	drm_plane_create_zpos_property(plane, idx, 0, num_planes - 1);
>>
>>  	return plane;
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki