Re: [PATCH 2/8] drm: Don't update property values for atomic drivers

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 2/8] drm: Don't update property values for atomic drivers
Date: Mon, 14 Aug 2017 16:09:28 +0200	[thread overview]
Message-ID: <20170814140928.jbxue53vsxky7j5s@phenom.ffwll.local> (raw)
In-Reply-To: <5344664.Flp6JA0AKa@avalon>

On Mon, Aug 14, 2017 at 01:32:07PM +0300, Laurent Pinchart wrote:
> On Monday 14 Aug 2017 09:25:47 Daniel Vetter wrote:
> > On Sat, Aug 12, 2017 at 12:20 AM, Laurent Pinchart
> > 
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > On Tuesday 25 Jul 2017 10:01:16 Daniel Vetter wrote:
> > >> diff --git a/drivers/gpu/drm/drm_mode_object.c
> > >> b/drivers/gpu/drm/drm_mode_object.c index da9a9adbcc98..92743a796bf0
> > >> 100644
> > >> --- a/drivers/gpu/drm/drm_mode_object.c
> > >> +++ b/drivers/gpu/drm/drm_mode_object.c
> > >> @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct
> > >> drm_mode_object *obj, {
> > >> 
> > >>       int i;
> > >> 
> > >> +     WARN_ON(drm_drv_uses_atomic_modeset(property->dev) &&
> > >> +             !(property->flags & DRM_MODE_PROP_IMMUTABLE));
> > > 
> > > It would have been nice to remove the calls to
> > > drm_object_property_set_value() for the dpms property from drivers before
> > > adding this :-/ Three drivers (rcar- du, shmobile and fsl-dcu) initialize
> > > the connector's DPMS property to OFF using this call (the default being
> > > ON).
> > > 
> > > Following the DPMS code paths always give me a headache, so if you know by
> > > heart how I should replace the set property call, I'm all ears :-)
> > 
> > Remove, it doesn't do anything for kms drivers. The value these calls
> > update was never consulted when reading the property, instead
> > drm_atomic_connector_get_property() directly looks at
> > drm_connector->dpms.
> 
> But DRM_IOCTL_MODE_OBJ_GETPROPERTIES doesn't, it gets the property value 
> directly, doesn't it ?

It redirects to the atomic machinery, so should all work out.

> > These calls where only needed for legacy modeset. Also note that the default
> > _ON is the right one, since DPMS state is in addition to modeset state. dpms
> > should actually be set to _ON when you enable the display using a modeset
> > call, atomic takes care of that in
> > drm_atomic_helper_update_legacy_modeset_state().
> 
> Sure, but the connectors should start disabled, shouldn't they ?

Sure, and they are, because connector_state->crtc == NULL. DPMS only
matters if your connector is connected, which at boot-up/reset state it
isnt. Yes DPMS is one of those thing which with hindsight I'd like to
never have happened, it's probably the most quirky uabi we have :-/ 

If you have userspace that gets confused by this we need to:
a) either fix your userspace
b) if we decide to fix the kernel, fix it for everyone (i.e. in
drm_connector_init).

Given that the "standard kms driver" (aka i915.ko) doesn't do that, I
don't think we need b).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx