-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 2020-08-26 10:24 a.m., 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. This would be my case 2) above, so still requires figuring out how the atomic KMS helpers should deal with corner cases such as: * CRTC on, primary plane off, overlay & cursor planes on * RmFB of FB assigned to overlay plane → need to disable overlay plane, but driver rejects that (because it would leave only the cursor plane on) - -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0YkIwAKCRBaga+Oatuy AMWrAJ9IDMIj2eGXERYDZfwraOCHebwE9QCfR/nOaoLGfIOii6r1H4JLn9sraFI= =D/LX -----END PGP SIGNATURE----- _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
