[PATCH] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is

Tue Sep 1 07:57:19 UTC 2020

On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> > On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer
> > <michel at daenzer.net> 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 at redhat.com>
> >>>>
> >>>> 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.
> >
> > Hi,
> >
> > my opinion as a userspace developer is that enabling a CRTC
> > without a primary plane has traditionally not worked, so userspace
> > cannot *rely* on it ever working. I think legacy KMS API does not
> > even allow to express that really, or has built-in assumptions that
> > it doesn't work - you can call the legacy cursor ioctls, they
> > don't fail, but also the CRTC remains off. Correct me if this is
> > not true.
> >
> > Atomic userspace OTOH is required to test the strange (and all!)
> > cases like this to see if they work or not, and carry on with a
> > fallback if they don't. Userspace that wants to use a CRTC without
> > a primary plane likely needs other CRTC properties as well, like
> > background color.
> >
> > I would guess that when two regressions conflict, the older
> > userspace would win. Hence legacy KMS using Mutter should keep
> > working, while atomic userspace is left with increased power
> > consumption. Not using a hardware cursor (Mutter) is much more
> > easily an end-user visible regression than slightly shorter
> > battery life.
> >
> > Atomic test commits are allowed to fail at any time for any reason
> > and something that previously worked can fail on the next frame or
> > on the next kernel, as far as I understand.
> 
> All of this basically matches my understanding.
> 
> 
> > Sounds like the helpers you refer to are inadequate for your case.
> > Can't you fix the helpers in the long run and land this patch as an
> > immediate fix?
> 
> I'd rather leave working on those helpers to KMS developers. :)

I thought the issue is the rmfb ioctl trickery, which has this assumption
fully backed in. The wiggle room in there for semantic changes is iirc
pretty much nil, it took us a pile of patches to just get to the current
state. So it's not helper code, it's core legacy ioctl code for atomic
drivers.
-Daniel

> 
> 
> - -- 
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer
> -----BEGIN PGP SIGNATURE-----
> 
> iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy
> AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs=
> =Pbd2
> -----END PGP SIGNATURE-----
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch