Message-ID: <CAFNQBQyfWmfu5T7bgZDZFGfyhsxQi7YXmY_wPc9Y+mm5iSspXQ@mail.gmail.com>
Date: Tue, 16 Jan 2024 14:11:43 +0000
From: Andri Yngvason <andri@...vason.is>
To: Sebastian Wick <sebastian.wick@...hat.com>
Cc: Harry Wentland <harry.wentland@....com>, Leo Li <sunpeng.li@....com>, 
	Rodrigo Siqueira <Rodrigo.Siqueira@....com>, Alex Deucher <alexander.deucher@....com>, 
	Christian König <christian.koenig@....com>, 
	"Pan, Xinhui" <Xinhui.Pan@....com>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, Jani Nikula <jani.nikula@...ux.intel.com>, 
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>, 
	Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>, amd-gfx@...ts.freedesktop.org, 
	intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Werner Sembach <wse@...edocomputers.com>, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as
 setting for userspace

þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick
<sebastian.wick@...hat.com>:
>
> On Tue, Jan 16, 2024 at 01:13:13PM +0000, Andri Yngvason wrote:
[...]
> > şri., 16. jan. 2024 kl. 11:42 skrifaği Sebastian Wick
> > <sebastian.wick@...hat.com>:
> > >
> > > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> > > > From: Werner Sembach <wse@...edocomputers.com>
> > > >
> > > > Add a new general drm property "force color format" which can be used
> > > > by userspace to tell the graphics driver which color format to use.
> > >
> > > I don't like the "force" in the name. This just selects the color
> > > format, let's just call it "color format" then.
> > >
> >
> > In previous revisions, this was "preferred color format" and "actual
> > color format", of which the latter has been dropped. I recommend
> > reading the discussion for previous revisions.
>
> Please don't imply that I didn't read the thread I'm answering to.
>
> > There are arguments for adding "actual color format" later and if it
> > is added later, we'd end up with "color format" and "actual color
> > format", which might be confusing, and it is why I chose to call it
> > "force color format" because it clearly communicates intent and
> > disambiguates it from "actual color format".
>
> There is no such thing as "actual color format" in upstream though.
> Basing your naming on discarded ideas is not useful. The thing that sets
> the color space for example is called "Colorspace", not "force
> colorspace".
>

Sure, I'm happy with calling it whatever people want. Maybe we can
have a vote on it?

> > [...]
> > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> > > >   *   drm_connector_attach_max_bpc_property() to create and attach the
> > > >   *   property to the connector during initialization.
> > > >   *
> > > > + * force color format:
> > > > + *   This property is used by userspace to change the used color format. When
> > > > + *   used the driver will use the selected format if valid for the hardware,
> > >
> > > All properties are always "used", they just can have different values.
> > > You probably want to talk about the auto mode here.
> >
> > Maybe we can say something like: If userspace does not set the
> > property or if it is explicitly set to zero, the driver will select
> > the appropriate color format based on other constraints.
>
> The property can be in any state without involvement from user space.
> Don't talk about setting it, talk about the state it is in:
>
>   When the color format is auto, the driver will select a format.
>

Ok.

> > >
> > > > + *   sink, and current resolution and refresh rate combination. Drivers to
> > >
> > > If valid? So when a value is not actually supported user space can still
> > > set it? What happens then? How should user space figure out if the
> > > driver and the sink support the format?
> >
> > The kernel does not expose this property unless it's implemented in the driver.
>
> If the driver simply doesn't support *one format*, the enum value for
> that format should not be exposed, period. This isn't about the property
> on its own.

Right, understood. You mean that enum should only contain values that
are supported by the driver.

>
> > This was originally "preferred color format". Perhaps the
> > documentation should better reflect that it is now a mandatory
> > constraint which fails the modeset if not satisfied.
>
> That would definitely help.
>
> > >
> > > For the Colorspace prop, the kernel just exposes all formats it supports
> > > (independent of the sink) and then makes it the job of user space to
> > > figure out if the sink supports it.
> > >
> > > The same could be done here. Property value is exposed if the driver
> > > supports it in general, commits can fail if the driver can't support it
> > > for a specific commit because e.g. the resolution or refresh rate. User
> > > space must look at the EDID/DisplayID/mode to figure out the supported
> > > format for the sink.
> >
> > Yes, we can make it possible for userspace to discover which modes are
> > supported by the monitor, but there are other constraints that need to
> > be satisfied. This was discussed in the previous revision.
>
> I mean, yes, that's what I said. User space would then only be
> responsible for checking the sink capabilities and the atomic check
> would take into account other (non-sink) constraints.

Since we need to probe using TEST_ONLY anyway, we'll end up with two
mechanisms to do the same thing where one of them depends on the other
for completeness.

>
> > In any case, these things can be added later and need not be a part of
> > this change set.
>
> No, this is the contract between the kernel and user space and has to be
> figured out before we can merge new uAPI.
>
> >
> > [...]
> >

Thanks,
Andri

Powered by blists - more mailing lists