On 2020-09-01 3:54 a.m., Daniel Vetter wrote: > On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote: >> On Tue, 25 Aug 2020 12:58:19 -0400 >> "Kazlauskas, Nicholas" <nicholas.kazlauskas@xxxxxxx> wrote: >> >>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote: >>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote: >>>>> On 2020-08-21 12:57 p.m., Michel Dänzer wrote: >>>>>> From: Michel Dänzer <mdaenzer@xxxxxxxxxx> >>>>>> >>>>>> Don't check drm_crtc_state::active for this either, per its >>>>>> documentation in include/drm/drm_crtc.h: >>>>>> >>>>>> * Hence drivers must not consult @active in their various >>>>>> * &drm_mode_config_funcs.atomic_check callback to reject an atomic >>>>>> * commit. >>>>>> >>>>>> The atomic helpers disable the CRTC as needed for disabling the primary >>>>>> plane. >>>>>> >>>>>> This prevents at least the following problems if the primary plane gets >>>>>> disabled (e.g. due to destroying the FB assigned to the primary plane, >>>>>> as happens e.g. with mutter in Wayland mode): >>>>>> >>>>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled >>>>>> (e.g. via legacy DPMS property & cursor ioctl). >>>>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl. >>>>> >>>>> We previously had the requirement that the primary plane must be enabled >>>>> but some userspace expects that they can enable just the overlay plane >>>>> without anything else. >>>>> >>>>> I think the chromuiumos atomictest validates that this works as well: >>>>> >>>>> So is DRM going forward then with the expectation that this is wrong >>>>> behavior from userspace? >>>>> >>>>> We require at least one plane to be enabled to display a cursor, but it >>>>> doesn't necessarily need to be the primary. >>>> >>>> It's a "pick your poison" situation: >>>> >>>> 1) Currently the checks are invalid (atomic_check must not decide based >>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to >>>> accidentally hit errors trying to enable/move the cursor or switch DPMS >>>> off → on. >>>> >>>> 2) Accurately rejecting only atomic states where the cursor plane is >>>> enabled but all other planes are off would break the KMS helper code, >>>> which can only deal with the "CRTC on & primary plane off is not >>>> allowed" case specifically. >>>> >>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace >>>> which wants to enable an overlay plane while disabling the primary plane. >>>> >>>> >>>> I do think in principle atomic userspace is expected to handle case 3) >>>> and leave the primary plane enabled. However, this is not ideal from an >>>> energy consumption PoV. Therefore, here's another idea for a possible >>>> way out of this quagmire: >>>> >>>> amdgpu_dm does not reject any atomic states based on which planes are >>>> enabled in it. If the cursor plane is enabled but all other planes are >>>> off, amdgpu_dm internally either: >>>> >>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a >>>> minimum size FB with alpha = 0. >>>> >>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to >>>> the required size) containing all black, possibly using compression. >>>> (Trying to minimize the memory bandwidth) >>>> >>>> >>>> Does either of these seem feasible? If both do, which one would be >>>> preferable? >>>> >>>> >>> >>> It's really the same solution since DCN doesn't make a distinction >>> between primary or overlay planes in hardware. DCE doesn't have overlay >>> planes enabled so this is not relevant there. >>> >>> The old behavior (pre 5.1?) was to silently accept the commit even >>> though the screen would be completely black instead of outright >>> rejecting the commit. >>> >>> I almost wonder if that makes more sense in the short term here since >>> the only "userspace" affected here is IGT. We'll fail the CRC checks, >>> but no userspace actually tries to actively use a cursor with no primary >>> plane enabled from my understanding. >> >> Hi, >> >> I believe that there exists userspace that will *accidentally* attempt >> to update the cursor plane while primary plane or whole CRTC is off. >> Some versions of Mutter might do that on racy conditions, I suspect. >> These are legacy KMS users, not atomic KMS. >> >> However, I do not believe there exists any userspace that would >> actually expect the display to show the cursor plane alone without a >> primary plane. Therefore I'd be ok with legacy cursor ioctls silently >> succeeding. Atomic commits not. So the difference has to be in the >> translation from legacy UAPI to kernel internal atomic interface. >> >>> In the long term I think we can work on getting cursor actually on the >>> screen in this case, though I can't say I really like having to reserve >>> some small buffer (eg. 16x16) for allowing lightup on this corner case. >> >> Why would you bother implementing that? >> >> Is there really an IGT test that unconditionally demands cursor plane >> to be usable without any other planes? > > The cursor plane isn't anything else than any other plane, aside from the > legacy uapi implication that it's used for the legacy cursor ioctls. > > Which means the cursor plane could actually be a full-featured plane, and > it's totally legit to use just that without anything else enabled. > > So yeah if you allow that, it better show something :-) > > Personally I'd lean towards merging this patch to close the gap (oldest > regressions wins and all that) and then implement the black plane hack on > top. Not sure I'm a big fan of the black plane hack. Is there any way we could allow the (non-displayed) cursor for the legacy IOCTL but not for the atomic IOCTL? I assume that would require a change to core code in the atomic helpers that convert legacy IOCTLs to atomic for drivers. Harry > -Daniel > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
