þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick <sebastian.wick@xxxxxxxxxx>: > > 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@xxxxxxxxxx>: > > > > > > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote: > > > > From: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> > > > > > > > > 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
