[igt-dev] [PATCH i-g-t v1] tests/kms_universal_plane: Change num_primary and num_cursor asserts into warnings

[Matt Roper]

On Thu, Apr 21, 2022 at 02:34:42PM -0700, Jessica Zhang wrote:
> This check was originally put in place to prevent accidental calls to
> drm_plane_init instead of drm_universal_plane_init to go uncaught due to
> similarities in parameters.
> 
> However, both method signatures have now diverged enough so that
> accidental calls will be caught by the compiler. In addition, DRM allows
> drivers to support multiple primary planes, so demoting the assert to a
> warning instead will prevent subtests from being blocked for drivers
> where multiple primary planes are supported.

I think the _intent_ of the DRM core is still that there's only ever one
DRM_PLANE_TYPE_PRIMARY per CRTC (since the meaning of "primary" is "this
is the specific plane that the legacy ioctls like SET_CRTC will
update").  That information isn't as terribly important to modern
userspace that's fully atomic and doesn't use legacy ioctls, and it
sounds like that's true for all of the userspace that's every run on
your platform, so the multiple primary planes per pipe doesn't actually
cause any confusion or bugs in your setting.

Since MSM has already been allowing multiple primaries per pipe for a
while (due to the way your planes can migrate between pipes) we don't
want to break your existing ABI behavior, so we should continue to allow
this going forward.  Demoting the IGT assert below to a warning seems
reasonable to me given that justification.

I think we should still make it clear that multiple primary planes per
pipe isn't really the intent of the api and we're just allowing it for
compatibility, so maybe adding a comment to that effect above those
igt_warn_on()'s would be a good idea.  For hardware like yours where
planes can migrate between pipes and you'll never have to deal with any
userspace relying on non-atomic ioctls, I think it would actually be
more natural to have zero primary planes (and just return an error from
legacy ioctls like setcrtc that need one designated).

Anway, I'm fine with the change here so,

Acked-by: Matt Roper <matthew.d.roper at intel.com>

> 
> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
> ---
>  tests/kms_universal_plane.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> index 2e8958979dd6..3cb6d704a6ef 100644
> --- a/tests/kms_universal_plane.c
> +++ b/tests/kms_universal_plane.c
> @@ -154,8 +154,8 @@ functional_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
>  		else if (display->pipes[pipe].planes[i].type == DRM_PLANE_TYPE_CURSOR)
>  			num_cursor++;
>  
> -	igt_assert_eq(num_primary, 1);
> -	igt_assert_lte(num_cursor, 1);
> +	igt_warn_on(num_primary != 1);
> +	igt_warn_on(num_cursor > 1);
>  
>  	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>  	sprite = igt_output_get_plane_type(output, DRM_PLANE_TYPE_OVERLAY);
> -- 
> 2.31.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795