Received-SPF: pass (google.com: domain of linux-kernel+bounces-22229-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240109181104.1670304-1-andri@yngvason.is> <20240109181104.1670304-3-andri@yngvason.is>
 <CAPj87rNan8B5urDFkmD_Vti4to6p3NmvXYsTFQTNg-Ue2ieDug@mail.gmail.com>
 <CAFNQBQwiqqSRqzXAnC035UWCGF3=GGFR5SpDd=biPTOEA+cWbQ@mail.gmail.com> <ZZ509L_kmVC4IUBW@phenom.ffwll.local>
In-Reply-To: <ZZ509L_kmVC4IUBW@phenom.ffwll.local>
From: Daniel Stone <daniel@fooishbar.org>
Date: Wed, 10 Jan 2024 13:26:46 +0000
Message-ID: <CAPj87rOiS8F=oDW3iE=bgFyfeJnYhy8kPF2v-uYOq3xgYtVPAg@mail.gmail.com>
Subject: Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as
 feedback for userspace
To: Andri Yngvason <andri@yngvason.is>, Harry Wentland <harry.wentland@amd.com>, 
	Leo Li <sunpeng.li@amd.com>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>, 
	Alex Deucher <alexander.deucher@amd.com>, =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, 
	"Pan, Xinhui" <Xinhui.Pan@amd.com>, David Airlie <airlied@gmail.com>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, Jani Nikula <jani.nikula@linux.intel.com>, 
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>, Rodrigo Vivi <rodrigo.vivi@intel.com>, 
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>, amd-gfx@lists.freedesktop.org, 
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, 
	intel-gfx@lists.freedesktop.org, Simon Ser <contact@emersion.fr>, 
	Werner Sembach <wse@tuxedocomputers.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, 10 Jan 2024 at 10:44, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jan 09, 2024 at 11:12:11PM +0000, Andri Yngvason wrote:
> > =C5=A3ri., 9. jan. 2024 kl. 22:32 skrifa=C4=91i Daniel Stone <daniel@fo=
oishbar.org>:
> > > How does userspace determine what's happened without polling? Will it
> > > only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> > > updated after the commit has completed and the event being sent?
> > > Should it send a HOTPLUG event? Other?
> > >
> >
> > Userspace does not determine what's happened without polling. The purpo=
se
> > of this property is not for programmatic verification that the preferre=
d
> > property was applied. It is my understanding that it's mostly intended =
for
> > debugging purposes. It should only change as a consequence of modesetti=
ng,
> > although I didn't actually look into what happens if you set the "prefe=
rred
> > color format" outside of a modeset.
>
> This feels a bit irky to me, since we don't have any synchronization and
> it kinda breaks how userspace gets to know about stuff.
>
> For context the current immutable properties are all stuff that's derived
> from the sink (like edid, or things like that). Userspace is guaranteed t=
o
> get a hotplug event (minus driver bugs as usual) if any of these change,
> and we've added infrastructure so that the hotplug event even contains th=
e
> specific property so that userspace can avoid re-read (which can cause
> some costly re-probing) them all.

Right.

> [...]
>
> This thing here works entirely differently, and I think we need somewhat
> new semantics for this:
>
> - I agree it should be read-only for userspace, so immutable sounds right=
.
>
> - But I also agree with Daniel Stone that this should be tied more
>   directly to the modeset state.
>
> So I think the better approach would be to put the output type into
> drm_connector_state, require that drivers compute it in their
> ->atomic_check code (which in the future would allow us to report it out
> for TEST_ONLY commits too), and so guarantee that the value is updated
> right after the kms ioctl returns (and not somewhen later for non-blockin=
g
> commits).

That's exactly the point. Whether userspace gets an explicit
notification or it has to 'know' when to read isn't much of an issue -
just as long as it's well defined. I think the suggestion of 'do it in
atomic_check and then it's guaranteed to be readable when the commit
completes' is a good one.

I do still have some reservations - for instance, why do we have the
fallback to auto when userspace has explicitly requested a certain
type? - but they may have been covered previously.

Cheers,
Daniel