All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matt Roper <matthew.d.roper@intel.com>
To: Jessica Zhang <quic_jesszhan@quicinc.com>
Cc: robdclark@chromium.org, petri.latvala@intel.com,
	igt-dev@lists.freedesktop.org, quic_aravindh@quicinc.com
Subject: Re: [igt-dev] [PATCH i-g-t v1] tests/kms_universal_plane: Change num_primary and num_cursor asserts into warnings
Date: Thu, 21 Apr 2022 16:14:42 -0700	[thread overview]
Message-ID: <YmHlYkSG2fsRbJL9@mdroper-desk1.amr.corp.intel.com> (raw)
In-Reply-To: <20220421213442.366-1-quic_jesszhan@quicinc.com>

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@intel.com>

> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@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

  parent reply	other threads:[~2022-04-21 23:14 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-21 21:34 [igt-dev] [PATCH i-g-t v1] tests/kms_universal_plane: Change num_primary and num_cursor asserts into warnings Jessica Zhang
2022-04-21 22:34 ` [igt-dev] ✗ Fi.CI.BAT: failure for " Patchwork
2022-04-21 23:14 ` Matt Roper [this message]
2022-04-22 18:47   ` [igt-dev] [PATCH i-g-t v1] " Abhinav Kumar
2022-04-21 23:37 ` [igt-dev] ✗ GitLab.Pipeline: warning for tests/kms_universal_plane: Change num_primary and num_cursor asserts into warnings (rev2) Patchwork
2022-04-22  0:07 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
2022-04-22  3:26 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YmHlYkSG2fsRbJL9@mdroper-desk1.amr.corp.intel.com \
    --to=matthew.d.roper@intel.com \
    --cc=igt-dev@lists.freedesktop.org \
    --cc=petri.latvala@intel.com \
    --cc=quic_aravindh@quicinc.com \
    --cc=quic_jesszhan@quicinc.com \
    --cc=robdclark@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.