Re: Call for proposals for the next governance meeting
Personally I would say we could give it a go, but I don't believe that this will significantly change how long some protocols take to get merged. The group transaction is taking so long because the subsurface sync and desync modes together with content update queues are hard to get right and the protocol makes everything much worse. The color management protocol took so long because it evolved with our understanding of color and colorimetry which changed significantly over the years. Some protocols are just stuck because stakeholders can't agree and most of the time the problem is just time and priorities. On Wed, Apr 17, 2024 at 5:09 PM Austin Shafer wrote: > > Hi all, > > Not a protocol, but I think it would be good to discuss the possibility > of regular Wayland Governance meetings at a decided frequency. Currently > meetings are scheduled on demand to discuss a particular subject or > protocol, but I believe routine discussions could be very beneficial in > progressing protocol designs. > > One issue we currently have is that many protocol proposals turn into > multi year endeavors. Explicit Sync [1] is a recent example of this > which was merged after two years, and surface group transactions [2] are > still in review after four years. While these proposals are full of > excellent discussions, if the time is measured in years I think that > means there's room for improvement regarding how long it takes us to > make forward progress. It can also be unclear who is interested in a > protocol and for what reasons, or who depends on it to ship features in > a particular release. > > As more distros switch to Wayland by default, I believe having more > frequent/routine meetings would be a good investment to avoid > indefinitely blocking new desktop features. Less formal conversations > can also provide opportunities to see how implementations are > progressing, ask for reviews, and get an idea of when protocols might be > ready to land. All of these could be beneficial for handling growing > pains: more Wayland users means more feature requests. My hope is this > could reduce the social burden of proposing a protocol or tracking its > progress. > > That being said there are many open questions to answer: > - Is there interest in formally making meetings at a certain time > interval, would the community find this useful? > - How to decide on a time? Poll before every meeting? > - How frequent should the regular meetings be? Monthly? Biweekly? > - How far in advance would we decide on agenda/topics? Tentative agenda > sent out a week before with a call for topics? > - Pain-points in the existing protocol approval process: would this help > them? > - Should we track action items from the previous meeting and follow up > on their status? > - Should there be "status updates"/pings for long-lived protocol proposals? > - Possible agenda items for regular meetings. I have some initial ideas > but would appreciate more suggestions if there are any pressing > topics? > > Non-goals which I don't want to accidentally accomplish with this: > - Rush discussions or rush protocols out the door > - Force a schedule onto projects or contributors > > As always I'm open to any suggestions. I'm happy to drive the discussion > on this in the next governance meeting, and also shoulder the > organizational burden of doing these if we go forward with it. > > [1] > https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90 > [2] > https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/26 > > Thanks! > Austin > > On 4/17/24 8:37 AM, Vlad Zahorodnii wrote: > > Hello, > > > > The Wayland Governance Meeting is semi-regular meeting to drive > > discussion on wayland-protocols forward. > > > > We are looking for the proposals for the next meeting as well as people > > who can lead/drive the discussion. If there is a protocol that you would > > like to be on the agenda, please submit your proposals here. > > > > Regards, > > Vlad > > > > >
Re: what are the protocol rules about uniqueness of event and request names?
On Fri, Dec 8, 2023 at 12:44 AM jleivent wrote: > > On Thu, 7 Dec 2023 22:06:07 + > David Edmundson wrote: > > > The generated C code be full of conflicts. The > > MY_PROTOCOL_REQUESTEVENT_SINCE_VERSION define for a start. > > > > I think it might compile in C, but other generators exist that might > > not and it's making life much more confusing than it needs to be. I > > would strongly avoid it. > > > > David > > To be clear, I wasn't intending it to sound like I wanted to add an > event and a request with the same name myself. I'm writing some > middleware that sits between a Wayland compositor and some of its > clients, and I would like to know if it might encounter an interface > that has an event and a request with the same name. > > I think you've answered that it's not a good idea for a protocol author > to do that, but it also sounds like it's a possibility that someone > could do it anyway because there's no direct rule against it. So > maybe I should take the necessary precautions. I think a more useful thing to do would be to add this restriction (an interface cannot have an event and a request with the same name) to the documentation and to wayland-scanner. > > Thanks, > Jonathan >
Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Wed, Nov 8, 2023 at 11:16 AM Pekka Paalanen wrote: > > On Tue, 7 Nov 2023 11:58:26 -0500 > Harry Wentland wrote: > > > On 2023-11-07 04:55, Pekka Paalanen wrote: > > > On Mon, 6 Nov 2023 11:19:27 -0500 > > > Harry Wentland wrote: > > > > > >> On 2023-10-20 06:36, Pekka Paalanen wrote: > > >>> On Thu, 19 Oct 2023 10:56:40 -0400 > > >>> Harry Wentland wrote: > > >>> > > On 2023-10-10 12:13, Melissa Wen wrote: > > > O 09/08, Harry Wentland wrote: > > >> Signed-off-by: Harry Wentland > > >>> > > >>> ... > > >>> > > > Also, with this new plane API in place, I understand that we will > > > already need think on how to deal with the mixing between old drm > > > color > > > properties (color encoding and color range) and these new way of > > > setting > > > plane color properties. IIUC, Pekka asked a related question about it > > > when talking about CRTC automatic RGB->YUV (?) > > > > > > > We'll still need to confirm whether we'll want to deprecate these > > existing properties. If we do that we'd want a client prop. Things > > should still work without deprecating them, if drivers just pick up > > after the initial encoding and range CSC. > > > > But realistically it might be better to deprecate them and turn them > > into explicit colorops. > > >>> > > >>> The existing properties would need to be explicitly reflected in the > > >>> new pipelines anyway, otherwise there would always be doubt at which > > >>> point of a pipeline the old properties apply, and they might even > > >>> need to change positions between pipelines. > > >>> > > >>> I think it is simply easier to just hide all old color related > > >>> properties when userspace sets the client-cap to enable pipelines. The > > >>> problem is to make sure to hide all old properties on all drivers that > > >>> support the client-cap. > > >>> > > >>> As a pipeline must be complete (describe everything that happens to > > >>> pixel values), it's going to be a flag day per driver. > > >>> > > >>> Btw. the plane FB YUV->RGB conversion needs a colorop in every pipeline > > >>> as well. Maybe it's purely informative and non-configurable, keyed by > > >>> FB pixel format, but still. > > >>> > > >>> We also need a colorop to represent sample filtering, e.g. bilinear, > > >>> like I think Sebastian may have mentioned in the past. Everything > > >>> before the sample filter happens "per tap" as Joshua Ashton put it, and > > >>> everything after it happens on the sample that was computed as a > > >>> weighted average of the filter tap inputs (texels). > > >>> > > >>> There could be colorops other than sample filtering that operate on > > >>> more than one sample at a time, like blur or sharpness. There could > > >>> even be colorops that change the image size like adding padding that > > >>> the following colorop hardware requires, and then yet another colorop > > >>> that clips that padding away. For an example, see > > >>> https://lists.freedesktop.org/archives/dri-devel/2023-October/427015.html > > >>> > > >>> If that padding and its color can affect the pipeline results of the > > >>> pixels near the padding (e.g. some convolution is applied with them, > > >>> which may be the reason why padding is necessary to begin with), then > > >>> it would be best to model it. > > >>> > > >> > > >> I hear you but I'm also somewhat shying away from defining this at this > > >> point. > > > > > > Would you define them before the new UAPI is released though? > > > > > > I agree there is no need to have them in this patch series, but I think > > > we'd hit the below problems if the UAPI is released without them. > > > > > >> There are already too many things that need to happen and I will focus > > >> on the > > >> actual color blocks (LUTs, matrices) first. We'll always be able to add > > >> a new > > >> (read-only) colorop type to define scaling and tap behavior at any point > > >> and > > >> a client is free to ignore a color pipeline if it doesn't find any > > >> tap/scale > > >> info in it. > > > > > > How would userspace know to look for tap/scale info, if there is no > > > upstream definition even on paper? > > > > > > > So far OSes did not care about this. Whether that's good or bad is > > something everyone can answer for themselves. > > > > If you write a compositor and really need this you can just ignore > > color pipelines that don't have this, i.e., you'll probably want > > to wait with implementing color pipeline support until you have what > > you need from DRM/KMS. > > > > This is not to say I don't want to have support for Weston. But I'm > > wondering if we place too much importance on getting every little > > thing figured out whereas we could be making forward progress and > > address more aspects of a color pipeline in the future. There is a > > reason gamescope has made a huge difference in driving the color > > management work forward. > > > > > And the
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Tue, Nov 07, 2023 at 11:52:11AM -0500, Harry Wentland wrote: > > > On 2023-10-26 13:30, Sebastian Wick wrote: > > On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: > >> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) > >> Alex Goins wrote: > >> > >>> Thank you Harry and all other contributors for your work on this. > >>> Responses > >>> inline - > >>> > >>> On Mon, 23 Oct 2023, Pekka Paalanen wrote: > >>> > >>>> On Fri, 20 Oct 2023 11:23:28 -0400 > >>>> Harry Wentland wrote: > >>>> > >>>>> On 2023-10-20 10:57, Pekka Paalanen wrote: > >>>>>> On Fri, 20 Oct 2023 16:22:56 +0200 > >>>>>> Sebastian Wick wrote: > >>>>>> > >>>>>>> Thanks for continuing to work on this! > >>>>>>> > >>>>>>> On Thu, Oct 19, 2023 at 05:21:22PM -0400, Harry Wentland wrote: > > snip > > >> > >>>>>> I think we also need a definition of "informational". > >>>>>> > >>>>>> Counter-example 1: a colorop that represents a non-configurable > >>>>> > >>>>> Not sure what's "counter" for these examples? > >>>>> > >>>>>> YUV<->RGB conversion. Maybe it determines its operation from FB pixel > >>>>>> format. It cannot be set to bypass, it cannot be configured, and it > >>>>>> will alter color values. > >>> > >>> Would it be reasonable to expose this is a 3x4 matrix with a read-only > >>> blob and > >>> no BYPASS property? I already brought up a similar idea at the XDC HDR > >>> Workshop > >>> based on the principle that read-only blobs could be used to express some > >>> static > >>> pipeline elements without the need to define a new type, but got mixed > >>> opinions. > >>> I think this demonstrates the principle further, as clients could detect > >>> this > >>> programmatically instead of having to special-case the informational > >>> element. > >> > > > > I'm all for exposing fixed color ops but I suspect that most of those > > follow some standard and in those cases instead of exposing the matrix > > values one should prefer to expose a named matrix (e.g. BT.601, BT.709, > > BT.2020). > > > > Agreed. > > > As a general rule: always expose the highest level description. Going > > from a name to exact values is trivial, going from values to a name is > > much harder. > > > >> If the blob depends on the pixel format (i.e. the driver automatically > >> chooses a different blob per pixel format), then I think we would need > >> to expose all the blobs and how they correspond to pixel formats. > >> Otherwise ok, I guess. > >> > >> However, do we want or need to make a color pipeline or colorop > >> conditional on pixel formats? For example, if you use a YUV 4:2:0 type > >> of pixel format, then you must use this pipeline and not any other. Or > >> floating-point type of pixel format. I did not anticipate this before, > >> I assumed that all color pipelines and colorops are independent of the > >> framebuffer pixel format. A specific colorop might have a property that > >> needs to agree with the framebuffer pixel format, but I didn't expect > >> further limitations. > > > > We could simply fail commits when the pipeline and pixel format don't > > work together. We'll probably need some kind of ingress no-op node > > anyway and maybe could list pixel formats there if required to make it > > easier to find a working configuration. > > > > The problem with failing commits is that user-space has no idea why it > failed. If this means that userspace falls back to SW composition for > NV12 and P010 it would avoid HW offloading in one of the most important > use-cases on AMD HW for power-saving purposes. Exposing which pixel formats work with a pipeline should be uncontroversial, and so should be an informative scaler op. Both can be added without a problem at a later time, so let's not make any of that mandatory for the first version. One step after the other. > > snip > > >>> Despite being programmable, the LUTs are updated in a manner that is less > >>> efficient as compared to e.g. the non-static "degamma" LUT. Would it be >
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Fri, Oct 27, 2023 at 10:59:25AM +0200, Michel Dänzer wrote: > On 10/26/23 21:25, Alex Goins wrote: > > On Thu, 26 Oct 2023, Sebastian Wick wrote: > >> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: > >>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) > >>> Alex Goins wrote: > >>> > >>>> Despite being programmable, the LUTs are updated in a manner that is less > >>>> efficient as compared to e.g. the non-static "degamma" LUT. Would it be > >>>> helpful > >>>> if there was some way to tag operations according to their performance, > >>>> for example so that clients can prefer a high performance one when they > >>>> intend to do an animated transition? I recall from the XDC HDR workshop > >>>> that this is also an issue with AMD's 3DLUT, where updates can be too > >>>> slow to animate. > >>> > >>> I can certainly see such information being useful, but then we need to > >>> somehow quantize the performance. > > > > Right, which wouldn't even necessarily be universal, could depend on the > > given > > host, GPU, etc. It could just be a relative performance indication, to give > > an > > order of preference. That wouldn't tell you if it can or can't be animated, > > but > > when choosing between two LUTs to animate you could prefer the higher > > performance one. > > > >>> > >>> What I was left puzzled about after the XDC workshop is that is it > >>> possible to pre-load configurations in the background (slow), and then > >>> quickly switch between them? Hardware-wise I mean. > > > > This works fine for our "fast" LUTs, you just point them to a surface in > > video > > memory and they flip to it. You could keep multiple surfaces around and flip > > between them without having to reprogram them in software. We can easily do > > that > > with enumerated curves, populating them when the driver initializes instead > > of > > waiting for the client to request them. You can even point multiple hardware > > LUTs to the same video memory surface, if they need the same curve. > > > >> > >> We could define that pipelines with a lower ID are to be preferred over > >> higher IDs. > > > > Sure, but this isn't just an issue with a pipeline as a whole, but the > > individual elements within it and how to use them in a given context. > > > >> > >> The issue is that if programming a pipeline becomes too slow to be > >> useful it probably should just not be made available to user space. > > > > It's not that programming the pipeline is overall too slow. The LUTs we have > > that are relatively slow to program are meant to be set infrequently, or > > even > > just once, to allow the scaler and tone mapping operator to operate in fixed > > point PQ space. You might still want the tone mapper, so you would choose a > > pipeline that includes them, but when it comes to e.g. animating a night > > light, > > you would want to choose a different LUT for that purpose. > > > >> > >> The prepare-commit idea for blob properties would help to make the > >> pipelines usable again, but until then it's probably a good idea to just > >> not expose those pipelines. > > > > The prepare-commit idea actually wouldn't work for these LUTs, because they > > are > > programmed using methods instead of pointing them to a surface. I'm > > actually not > > sure how slow it actually is, would need to benchmark it. I think not > > exposing > > them at all would be overkill, since it would mean you can't use the > > preblending > > scaler or tonemapper, and animation isn't necessary for that. > > > > The AMD 3DLUT is another example of a LUT that is slow to update, and it > > would > > obviously be a major loss if that wasn't exposed. There just needs to be > > some > > way for clients to know if they are going to kill performance by trying to > > change it every frame. > > Might a first step be to require the ALLOW_MODESET flag to be set when > changing the values for a colorop which is too slow to be updated per refresh > cycle? > > This would tell the compositor: You can use this colorop, but you can't > change its values on the fly. I argued before that changing any color op to passthrough should never require ALLOW_MODESET and while this is really hard to guarantee from a driver perspective I still believe that
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote:
> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT)
> Alex Goins wrote:
>
> > Thank you Harry and all other contributors for your work on this. Responses
> > inline -
> >
> > On Mon, 23 Oct 2023, Pekka Paalanen wrote:
> >
> > > On Fri, 20 Oct 2023 11:23:28 -0400
> > > Harry Wentland wrote:
> > >
> > > > On 2023-10-20 10:57, Pekka Paalanen wrote:
> > > > > On Fri, 20 Oct 2023 16:22:56 +0200
> > > > > Sebastian Wick wrote:
> > > > >
> > > > >> Thanks for continuing to work on this!
> > > > >>
> > > > >> On Thu, Oct 19, 2023 at 05:21:22PM -0400, Harry Wentland wrote:
> > > > >>> v2:
> > > > >>> - Update colorop visualizations to match reality (Sebastian, Alex
> > > > >>> Hung)
> > > > >>> - Updated wording (Pekka)
> > > > >>> - Change BYPASS wording to make it non-mandatory (Sebastian)
> > > > >>> - Drop cover-letter-like paragraph from COLOR_PIPELINE Plane
> > > > >>> Property
> > > > >>>section (Pekka)
> > > > >>> - Use PQ EOTF instead of its inverse in Pipeline Programming
> > > > >>> example (Melissa)
> > > > >>> - Add "Driver Implementer's Guide" section (Pekka)
> > > > >>> - Add "Driver Forward/Backward Compatibility" section (Sebastian,
> > > > >>> Pekka)
> > > > >
> > > > > ...
> > > > >
> > > > >>> +An example of a drm_colorop object might look like one of these::
> > > > >>> +
> > > > >>> +/* 1D enumerated curve */
> > > > >>> +Color operation 42
> > > > >>> +├─ "TYPE": immutable enum {1D enumerated curve, 1D LUT, 3x3
> > > > >>> matrix, 3x4 matrix, 3D LUT, etc.} = 1D enumerated curve
> > > > >>> +├─ "BYPASS": bool {true, false}
> > > > >>> +├─ "CURVE_1D_TYPE": enum {sRGB EOTF, sRGB inverse EOTF, PQ
> > > > >>> EOTF, PQ inverse EOTF, …}
> > > > >>> +└─ "NEXT": immutable color operation ID = 43
> >
> > I know these are just examples, but I would also like to suggest the
> > possibility
> > of an "identity" CURVE_1D_TYPE. BYPASS = true might get different results
> > compared to setting an identity in some cases depending on the hardware. See
> > below for more on this, RE: implicit format conversions.
> >
> > Although NVIDIA hardware doesn't use a ROM for enumerated curves, it came
> > up in
> > offline discussions that it would nonetheless be helpful to expose
> > enumerated
> > curves in order to hide the vendor-specific complexities of programming
> > segmented LUTs from clients. In that case, we would simply refer to the
> > enumerated curve when calculating/choosing segmented LUT entries.
>
> That's a good idea.
>
> > Another thing that came up in offline discussions is that we could use
> > multiple
> > color operations to program a single operation in hardware. As I understand
> > it,
> > AMD has a ROM-defined LUT, followed by a custom 4K entry LUT, followed by an
> > "HDR Multiplier". On NVIDIA we don't have these as separate hardware
> > stages, but
> > we could combine them into a singular LUT in software, such that you can
> > combine
> > e.g. segmented PQ EOTF with night light. One caveat is that you will lose
> > precision from the custom LUT where it overlaps with the linear section of
> > the
> > enumerated curve, but that is unavoidable and shouldn't be an issue in most
> > use-cases.
>
> Indeed.
>
> > Actually, the current examples in the proposal don't include a multiplier
> > color
> > op, which might be useful. For AMD as above, but also for NVIDIA as the
> > following issue arises:
> >
> > As discussed further below, the NVIDIA "degamma" LUT performs an implicit
> > fixed
> > point to FP16 conversion. In that conversion, what fixed point 0x
> > maps
> > to in floating point varies depending on the source content. If it's SDR
> > content, we want the max value in FP16 to be 1.0 (80 nits), subject to a
> > potential boost multiplier if we want SDR content to be brighter. If it's
> > HDR PQ
> &
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
Thanks for continuing to work on this! On Thu, Oct 19, 2023 at 05:21:22PM -0400, Harry Wentland wrote: > v2: > - Update colorop visualizations to match reality (Sebastian, Alex Hung) > - Updated wording (Pekka) > - Change BYPASS wording to make it non-mandatory (Sebastian) > - Drop cover-letter-like paragraph from COLOR_PIPELINE Plane Property >section (Pekka) > - Use PQ EOTF instead of its inverse in Pipeline Programming example > (Melissa) > - Add "Driver Implementer's Guide" section (Pekka) > - Add "Driver Forward/Backward Compatibility" section (Sebastian, Pekka) > > Signed-off-by: Harry Wentland > Cc: Ville Syrjala > Cc: Pekka Paalanen > Cc: Simon Ser > Cc: Harry Wentland > Cc: Melissa Wen > Cc: Jonas Ådahl > Cc: Sebastian Wick > Cc: Shashank Sharma > Cc: Alexander Goins > Cc: Joshua Ashton > Cc: Michel Dänzer > Cc: Aleix Pol > Cc: Xaver Hugl > Cc: Victoria Brekenfeld > Cc: Sima > Cc: Uma Shankar > Cc: Naseer Ahmed > Cc: Christopher Braga > Cc: Abhinav Kumar > Cc: Arthur Grillo > Cc: Hector Martin > Cc: Liviu Dudau > Cc: Sasha McIntosh > --- > Documentation/gpu/rfc/color_pipeline.rst | 347 +++ > 1 file changed, 347 insertions(+) > create mode 100644 Documentation/gpu/rfc/color_pipeline.rst > > diff --git a/Documentation/gpu/rfc/color_pipeline.rst > b/Documentation/gpu/rfc/color_pipeline.rst > new file mode 100644 > index ..af5f2ea29116 > --- /dev/null > +++ b/Documentation/gpu/rfc/color_pipeline.rst > @@ -0,0 +1,347 @@ > + > +Linux Color Pipeline API > + > + > +What problem are we solving? > + > + > +We would like to support pre-, and post-blending complex color > +transformations in display controller hardware in order to allow for > +HW-supported HDR use-cases, as well as to provide support to > +color-managed applications, such as video or image editors. > + > +It is possible to support an HDR output on HW supporting the Colorspace > +and HDR Metadata drm_connector properties, but that requires the > +compositor or application to render and compose the content into one > +final buffer intended for display. Doing so is costly. > + > +Most modern display HW offers various 1D LUTs, 3D LUTs, matrices, and other > +operations to support color transformations. These operations are often > +implemented in fixed-function HW and therefore much more power efficient than > +performing similar operations via shaders or CPU. > + > +We would like to make use of this HW functionality to support complex color > +transformations with no, or minimal CPU or shader load. > + > + > +How are other OSes solving this problem? > + > + > +The most widely supported use-cases regard HDR content, whether video or > +gaming. > + > +Most OSes will specify the source content format (color gamut, encoding > transfer > +function, and other metadata, such as max and average light levels) to a > driver. > +Drivers will then program their fixed-function HW accordingly to map from a > +source content buffer's space to a display's space. > + > +When fixed-function HW is not available the compositor will assemble a > shader to > +ask the GPU to perform the transformation from the source content format to > the > +display's format. > + > +A compositor's mapping function and a driver's mapping function are usually > +entirely separate concepts. On OSes where a HW vendor has no insight into > +closed-source compositor code such a vendor will tune their color management > +code to visually match the compositor's. On other OSes, where both mapping > +functions are open to an implementer they will ensure both mappings match. > + > +This results in mapping algorithm lock-in, meaning that no-one alone can > +experiment with or introduce new mapping algorithms and achieve > +consistent results regardless of which implementation path is taken. > + > +Why is Linux different? > +=== > + > +Unlike other OSes, where there is one compositor for one or more drivers, on > +Linux we have a many-to-many relationship. Many compositors; many drivers. > +In addition each compositor vendor or community has their own view of how > +color management should be done. This is what makes Linux so beautiful. > + > +This means that a HW vendor can now no longer tune their driver to one > +compositor, as tuning it to one could make it look fairly different from > +another compositor's color mapping. > + > +We need a better solution. > + > + > +Descriptive API > +=== >
Re: [PATCH RFC v6 01/10] drm: Introduce pixel_source DRM plane property
On Tue, Aug 29, 2023 at 10:48:16AM +0300, Pekka Paalanen wrote: > On Mon, 28 Aug 2023 17:05:07 -0700 > Jessica Zhang wrote: > > > Add support for pixel_source property to drm_plane and related > > documentation. In addition, force pixel_source to > > DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break > > legacy userspace. > > > > This enum property will allow user to specify a pixel source for the > > plane. Possible pixel sources will be defined in the > > drm_plane_pixel_source enum. > > > > Currently, the only pixel sources are DRM_PLANE_PIXEL_SOURCE_FB (the > > default value) and DRM_PLANE_PIXEL_SOURCE_NONE. > > > > Signed-off-by: Jessica Zhang > > --- > > drivers/gpu/drm/drm_atomic_state_helper.c | 1 + > > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ > > drivers/gpu/drm/drm_blend.c | 90 > > +++ > > drivers/gpu/drm/drm_plane.c | 19 +-- > > include/drm/drm_blend.h | 2 + > > include/drm/drm_plane.h | 21 > > 6 files changed, 133 insertions(+), 4 deletions(-) > > ... > > > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > > index 6e74de833466..c3c57bae06b7 100644 > > --- a/drivers/gpu/drm/drm_blend.c > > +++ b/drivers/gpu/drm/drm_blend.c > > @@ -185,6 +185,21 @@ > > * plane does not expose the "alpha" property, then this is > > * assumed to be 1.0 > > * > > + * pixel_source: > > + * pixel_source is set up with drm_plane_create_pixel_source_property(). > > + * It is used to toggle the active source of pixel data for the plane. > > + * The plane will only display data from the set pixel_source -- any > > + * data from other sources will be ignored. > > + * > > + * Possible values: > > + * > > + * "NONE": > > + * No active pixel source. > > + * Committing with a NONE pixel source will disable the plane. > > + * > > + * "FB": > > + * Framebuffer source set by the "FB_ID" property. > > + * > > * Note that all the property extensions described here apply either to the > > * plane or the CRTC (e.g. for the background color, which currently is not > > * exposed and assumed to be black). > > This UAPI: > Acked-by: Pekka Paalanen Thanks Jessica, same for me Acked-by: Sebastian Wick > > > Thanks, > pq
Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Wed, Oct 11, 2023 at 12:20 PM Pekka Paalanen wrote:
>
> On Tue, 10 Oct 2023 15:13:46 -0100
> Melissa Wen wrote:
>
> > O 09/08, Harry Wentland wrote:
> > > Signed-off-by: Harry Wentland
> > > Cc: Ville Syrjala
> > > Cc: Pekka Paalanen
> > > Cc: Simon Ser
> > > Cc: Harry Wentland
> > > Cc: Melissa Wen
> > > Cc: Jonas Ådahl
> > > Cc: Sebastian Wick
> > > Cc: Shashank Sharma
> > > Cc: Alexander Goins
> > > Cc: Joshua Ashton
> > > Cc: Michel Dänzer
> > > Cc: Aleix Pol
> > > Cc: Xaver Hugl
> > > Cc: Victoria Brekenfeld
> > > Cc: Daniel Vetter
> > > Cc: Uma Shankar
> > > Cc: Naseer Ahmed
> > > Cc: Christopher Braga
> > > ---
> > > Documentation/gpu/rfc/color_pipeline.rst | 278 +++
> > > 1 file changed, 278 insertions(+)
> > > create mode 100644 Documentation/gpu/rfc/color_pipeline.rst
> > >
> > > diff --git a/Documentation/gpu/rfc/color_pipeline.rst
> > > b/Documentation/gpu/rfc/color_pipeline.rst
> > > new file mode 100644
> > > index ..bfa4a8f12087
> > > --- /dev/null
> > > +++ b/Documentation/gpu/rfc/color_pipeline.rst
>
> ...
>
> > > +Color Pipeline Discovery
> > > +
> > > +
> > > +A DRM client wanting color management on a drm_plane will:
> > > +
> > > +1. Read all drm_colorop objects
> > > +2. Get the COLOR_PIPELINE property of the plane
> > > +3. iterate all COLOR_PIPELINE enum values
> > > +4. for each enum value walk the color pipeline (via the NEXT pointers)
> > > + and see if the available color operations are suitable for the
> > > + desired color management operations
> > > +
> > > +An example of chained properties to define an AMD pre-blending color
> > > +pipeline might look like this::
> >
> > Hi Harry,
> >
> > Thanks for sharing this proposal. Overall I think it's very aligned with
> > Simon's description of the generic KMS color API. I think it's a good
> > start point and we can refine over iterations. My general questions have
> > already been pointed out by Sebastian and Pekka (mainly regarding the
> > BYPASS property).
> >
> > I still have some doubts on how to fit these set of colorops with some
> > AMD corners cases as below:
> >
> > > +
> > > +Plane 10
> > > +├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
> > > +└─ "color_pipeline": enum {0, 42} = 0
> > > +Color operation 42 (input CSC)
> > > +├─ "type": enum {Bypass, Matrix} = Matrix
> > > +├─ "matrix_data": blob
> > > +└─ "next": immutable color operation ID = 43
> >
> > IIUC, for input CSC, there are currently two possiblities, or we use
> > `drm_plane_color_encoding` and `drm_plane_color range` to get
> > pre-defined coefficients or we set a custom matrix here, right? If so, I
> > think we need some kind of pre-defined matrix option?
Seems reasonable. If they are mutually exclusive you might want to
expose 2 different pipelines for it.
> > Also, with this new plane API in place, I understand that we will
> > already need think on how to deal with the mixing between old drm color
> > properties (color encoding and color range) and these new way of setting
> > plane color properties. IIUC, Pekka asked a related question about it
> > when talking about CRTC automatic RGB->YUV (?)
Indeed, good catch! I listed some of them in my proposal more than one
year ago but completely forgot about them already.
>
> I didn't realize color encoding and color range KMS plane properties
> even existed. There is even color space on rockchip!
>
> https://drmdb.emersion.fr/properties?object-type=4008636142
>
> That list has even more conflicts: DEGAMMA_MODE, EOTF, FEATURE,
> NV_INPUT_COLORSPACE, SCALING_FILTER, WATERMARK, alpha, GLOBAL_ALPHA,
> brightness, colorkey, contrast, and more. I hope most of them are
> actually from downstream drivers.
>
> I think they should be forbidden to be used together with the new
> pipeline UAPI. Mixing does not work in the long run, it would be
> undefined at what position do the old properties apply in a pipeline.
>
> Apparently, we already need a DRM client cap for the new color pipeline
> UAPI to hide these legacy things.
Agreed. We'll need one cap for planes and one in the future for CRTCs then.
>
> This is different from "CRTC automatic RGB->YUV&quo
Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed
Hey Harry, Thank you and Simon for this great document. Really happy about it, but obviously I've got a few notes and questions inline. On Fri, Sep 08, 2023 at 11:02:26AM -0400, Harry Wentland wrote: > Signed-off-by: Harry Wentland > Cc: Ville Syrjala > Cc: Pekka Paalanen > Cc: Simon Ser > Cc: Harry Wentland > Cc: Melissa Wen > Cc: Jonas Ådahl > Cc: Sebastian Wick > Cc: Shashank Sharma > Cc: Alexander Goins > Cc: Joshua Ashton > Cc: Michel Dänzer > Cc: Aleix Pol > Cc: Xaver Hugl > Cc: Victoria Brekenfeld > Cc: Daniel Vetter > Cc: Uma Shankar > Cc: Naseer Ahmed > Cc: Christopher Braga > --- > Documentation/gpu/rfc/color_pipeline.rst | 278 +++ > 1 file changed, 278 insertions(+) > create mode 100644 Documentation/gpu/rfc/color_pipeline.rst > > diff --git a/Documentation/gpu/rfc/color_pipeline.rst > b/Documentation/gpu/rfc/color_pipeline.rst > new file mode 100644 > index ..bfa4a8f12087 > --- /dev/null > +++ b/Documentation/gpu/rfc/color_pipeline.rst > @@ -0,0 +1,278 @@ > + > +Linux Color Pipeline API > + > + > +What problem are we solving? > + > + > +We would like to support pre-, and post-blending complex color > transformations > +in order to allow for HW-supported HDR use-cases, as well as to provide > support > +to color-managed applications, such as video or image editors. > + > +While it is possible to support an HDR output on HW supporting the Colorspace > +and HDR Metadata drm_connector properties that requires the compositor or > +application to render and compose the content into one final buffer intended > for > +display. Doing so is costly. > + > +Most modern display HW offers various 1D LUTs, 3D LUTs, matrices, and other > +operations to support color transformations. These operations are often > +implemented in fixed-function HW and therefore much more power efficient than > +performing similar operations via shaders or CPU. > + > +We would like to make use of this HW functionality to support complex color > +transformations with no, or minimal CPU or shader load. > + > + > +How are other OSes solving this problem? > + > + > +The most widely supported use-cases regard HDR content, whether video or > +gaming. > + > +Most OSes will specify the source content format (color gamut, encoding > transfer > +function, and other metadata, such as max and average light levels) to a > driver. > +Drivers will then program their fixed-function HW accordingly to map from a > +source content buffer's space to a display's space. > + > +When fixed-function HW is not available the compositor will assemble a > shader to > +ask the GPU to perform the transformation from the source content format to > the > +display's format. > + > +A compositor's mapping function and a driver's mapping function are usually > +entirely separate concepts. On OSes where a HW vendor has no insight into > +closed-source compositor code such a vendor will tune their color management > +code to visually match the compositor's. On other OSes, where both mapping > +functions are open to an implementer they will ensure both mappings match. > + > + > +Why is Linux different? > +=== > + > +Unlike other OSes, where there is one compositor for one or more drivers, on > +Linux we have a many-to-many relationship. Many compositors; many drivers. > +In addition each compositor vendor or community has their own view of how > +color management should be done. This is what makes Linux so beautiful. > + > +This means that a HW vendor can now no longer tune their driver to one > +compositor, as tuning it to one will almost inevitably make it look very > +different from another compositor's color mapping. > + > +We need a better solution. > + > + > +Descriptive API > +=== > + > +An API that describes the source and destination colorspaces is a descriptive > +API. It describes the input and output color spaces but does not describe > +how precisely they should be mapped. Such a mapping includes many minute > +design decision that can greatly affect the look of the final result. > + > +It is not feasible to describe such mapping with enough detail to ensure the > +same result from each implementation. In fact, these mappings are a very > active > +research area. > + > + > +Prescriptive API > + > + > +A prescriptive API describes not the source and destination colorspaces. It > +instead prescribes a recipe for how to manipulate pixel values to arrive at > the > +desired o
Re: [RFC 00/33] Add Support for Plane Color Pipeline
On Tue, Sep 05, 2023 at 02:33:04PM +0200, Sebastian Wick wrote: > On Tue, Sep 05, 2023 at 02:33:26PM +0300, Pekka Paalanen wrote: > > On Mon, 4 Sep 2023 14:29:56 + > > "Shankar, Uma" wrote: > > > > > > -Original Message- > > > > From: Sebastian Wick > > > > Sent: Thursday, August 31, 2023 2:46 AM > > > > To: Shankar, Uma > > > > Cc: Harry Wentland ; intel- > > > > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; wayland- > > > > de...@lists.freedesktop.org; Ville Syrjala > > > > ; Pekka > > > > Paalanen ; Simon Ser > > > > ; > > > > Melissa Wen ; Jonas Ådahl ; Shashank > > > > Sharma ; Alexander Goins ; > > > > Naseer Ahmed ; Christopher Braga > > > > > > > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > > > > > > > On Wed, Aug 30, 2023 at 08:47:37AM +, Shankar, Uma wrote: > > > > > > > > > > > > > > > > -Original Message- > > > > > > From: Harry Wentland > > > > > > Sent: Wednesday, August 30, 2023 12:56 AM > > > > > > To: Shankar, Uma ; > > > > > > intel-...@lists.freedesktop.org; dri- de...@lists.freedesktop.org > > > > > > Cc: wayland-devel@lists.freedesktop.org; Ville Syrjala > > > > > > ; Pekka Paalanen > > > > > > ; Simon Ser ; > > > > > > Melissa Wen ; Jonas Ådahl ; > > > > > > Sebastian Wick ; Shashank Sharma > > > > > > ; Alexander Goins ; > > > > > > Naseer Ahmed ; Christopher Braga > > > > > > > > > > > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > > > > > > > > > > > +CC Naseer and Chris, FYI > > > > > > > > > > > > See https://patchwork.freedesktop.org/series/123024/ for whole > > > > > > series. > > > > > > > > > > > > On 2023-08-29 12:03, Uma Shankar wrote: > > > > > > > Introduction > > > > > > > > > > > > > > > > > > > > > Modern hardwares have various color processing capabilities both > > > > > > > at pre-blending and post-blending phases in the color pipeline. > > > > > > > The current drm implementation exposes only the post-blending > > > > > > > color hardware blocks. Support for pre-blending hardware is > > > > > > > missing. > > > > > > > There are multiple use cases where pre-blending color hardware > > > > > > > will be > > > > > > > useful: > > > > > > > a) Linearization of input buffers encoded in various transfer > > > > > > > functions. > > > > > > > b) Color Space conversion > > > > > > > c) Tone mapping > > > > > > > d) Frame buffer format conversion > > > > > > > e) Non-linearization of buffer(apply transfer function) > > > > > > > f) 3D Luts > > > > > > > > > > > > > > and other miscellaneous color operations. > > > > > > > > > > > > > > Hence, there is a need to expose the color capabilities of the > > > > > > > hardware to user-space. This will help userspace/middleware to use > > > > > > > display hardware for color processing and blending instead of > > > > > > > doing it through GPU shaders. > > > > > > > > > > > > > > > > > > > Thanks, Uma, for sending this. I've been working on something > > > > > > similar but you beat me to it. :) > > > > > > > > > > Thanks Harry for the useful feedback and overall collaboration on > > > > > this so far. > > > > > > > > > > > > > > > > > > > Work done so far and relevant references > > > > > > > > > > > > > > > > > > > > > Some implementation is done by Intel and AMD/Igalia to address > > > > > > > the same. > > > > > > > Broad consensus is there that we need a generic API at drm core to > > > > > > > suf
Re: [RFC 00/33] Add Support for Plane Color Pipeline
On Tue, Sep 05, 2023 at 02:33:26PM +0300, Pekka Paalanen wrote: > On Mon, 4 Sep 2023 14:29:56 + > "Shankar, Uma" wrote: > > > > -Original Message- > > > From: Sebastian Wick > > > Sent: Thursday, August 31, 2023 2:46 AM > > > To: Shankar, Uma > > > Cc: Harry Wentland ; intel- > > > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; wayland- > > > de...@lists.freedesktop.org; Ville Syrjala > > > ; Pekka > > > Paalanen ; Simon Ser ; > > > Melissa Wen ; Jonas Ådahl ; Shashank > > > Sharma ; Alexander Goins ; > > > Naseer Ahmed ; Christopher Braga > > > > > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > > > > > On Wed, Aug 30, 2023 at 08:47:37AM +, Shankar, Uma wrote: > > > > > > > > > > > > > -Original Message- > > > > > From: Harry Wentland > > > > > Sent: Wednesday, August 30, 2023 12:56 AM > > > > > To: Shankar, Uma ; > > > > > intel-...@lists.freedesktop.org; dri- de...@lists.freedesktop.org > > > > > Cc: wayland-devel@lists.freedesktop.org; Ville Syrjala > > > > > ; Pekka Paalanen > > > > > ; Simon Ser ; > > > > > Melissa Wen ; Jonas Ådahl ; > > > > > Sebastian Wick ; Shashank Sharma > > > > > ; Alexander Goins ; > > > > > Naseer Ahmed ; Christopher Braga > > > > > > > > > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > > > > > > > > > +CC Naseer and Chris, FYI > > > > > > > > > > See https://patchwork.freedesktop.org/series/123024/ for whole series. > > > > > > > > > > On 2023-08-29 12:03, Uma Shankar wrote: > > > > > > Introduction > > > > > > > > > > > > > > > > > > Modern hardwares have various color processing capabilities both > > > > > > at pre-blending and post-blending phases in the color pipeline. > > > > > > The current drm implementation exposes only the post-blending > > > > > > color hardware blocks. Support for pre-blending hardware is missing. > > > > > > There are multiple use cases where pre-blending color hardware > > > > > > will be > > > > > > useful: > > > > > > a) Linearization of input buffers encoded in various transfer > > > > > >functions. > > > > > > b) Color Space conversion > > > > > > c) Tone mapping > > > > > > d) Frame buffer format conversion > > > > > > e) Non-linearization of buffer(apply transfer function) > > > > > > f) 3D Luts > > > > > > > > > > > > and other miscellaneous color operations. > > > > > > > > > > > > Hence, there is a need to expose the color capabilities of the > > > > > > hardware to user-space. This will help userspace/middleware to use > > > > > > display hardware for color processing and blending instead of > > > > > > doing it through GPU shaders. > > > > > > > > > > > > > > > > Thanks, Uma, for sending this. I've been working on something > > > > > similar but you beat me to it. :) > > > > > > > > Thanks Harry for the useful feedback and overall collaboration on this > > > > so far. > > > > > > > > > > > > > > > > Work done so far and relevant references > > > > > > > > > > > > > > > > > > Some implementation is done by Intel and AMD/Igalia to address the > > > > > > same. > > > > > > Broad consensus is there that we need a generic API at drm core to > > > > > > suffice the use case of various HW vendors. Below are the links > > > > > > capturing the discussion so far. > > > > > > > > > > > > Intel's Plane Color Implementation: > > > > > > https://patchwork.freedesktop.org/series/90825/ > > > > > > AMD's Plane Color Implementation: > > > > > > https://patchwork.freedesktop.org/series/116862/ > > > > > > > > > > > > > > > > > > Hackfest conclus
Re: [RFC 00/33] Add Support for Plane Color Pipeline
On Wed, Aug 30, 2023 at 08:47:37AM +, Shankar, Uma wrote: > > > > -Original Message- > > From: Harry Wentland > > Sent: Wednesday, August 30, 2023 12:56 AM > > To: Shankar, Uma ; intel-...@lists.freedesktop.org; > > dri- > > de...@lists.freedesktop.org > > Cc: wayland-devel@lists.freedesktop.org; Ville Syrjala > > ; Pekka Paalanen > > ; > > Simon Ser ; Melissa Wen ; Jonas Ådahl > > ; Sebastian Wick ; Shashank > > Sharma ; Alexander Goins ; > > Naseer Ahmed ; Christopher Braga > > > > Subject: Re: [RFC 00/33] Add Support for Plane Color Pipeline > > > > +CC Naseer and Chris, FYI > > > > See https://patchwork.freedesktop.org/series/123024/ for whole series. > > > > On 2023-08-29 12:03, Uma Shankar wrote: > > > Introduction > > > > > > > > > Modern hardwares have various color processing capabilities both at > > > pre-blending and post-blending phases in the color pipeline. > > > The current drm implementation exposes only the post-blending color > > > hardware blocks. Support for pre-blending hardware is missing. > > > There are multiple use cases where pre-blending color hardware will be > > > useful: > > > a) Linearization of input buffers encoded in various transfer > > > functions. > > > b) Color Space conversion > > > c) Tone mapping > > > d) Frame buffer format conversion > > > e) Non-linearization of buffer(apply transfer function) > > > f) 3D Luts > > > > > > and other miscellaneous color operations. > > > > > > Hence, there is a need to expose the color capabilities of the > > > hardware to user-space. This will help userspace/middleware to use > > > display hardware for color processing and blending instead of doing it > > > through GPU shaders. > > > > > > > Thanks, Uma, for sending this. I've been working on something similar but > > you beat > > me to it. :) > > Thanks Harry for the useful feedback and overall collaboration on this so far. > > > > > > > Work done so far and relevant references > > > > > > > > > Some implementation is done by Intel and AMD/Igalia to address the same. > > > Broad consensus is there that we need a generic API at drm core to > > > suffice the use case of various HW vendors. Below are the links > > > capturing the discussion so far. > > > > > > Intel's Plane Color Implementation: > > > https://patchwork.freedesktop.org/series/90825/ > > > AMD's Plane Color Implementation: > > > https://patchwork.freedesktop.org/series/116862/ > > > > > > > > > Hackfest conclusions > > > > > > > > > HDR/Color Hackfest was organised by Redhat to bring all the industry > > > stakeholders together and converge on a common uapi expectations. > > > Participants from Intel, AMD, Nvidia, Collabora, Redhat, Igalia and > > > other prominent user-space developers and maintainers. > > > > > > Discussions happened on the uapi expectations, opens, nature of > > > hardware of multiple hardware vendors, challenges in generalizing the > > > same and the path forward. Consensus was made that drm core should > > > implement descriptive APIs and not go with prescriptive APIs. DRM core > > > should just expose the hardware capabilities; enabling, customizing > > > and programming the same should be done by the user-space. Driver should > > > just > > honor the user space request without doing any operations internally. > > > > > > Thanks to Simon Ser, for nicely documenting the design consensus and > > > an UAPI RFC which can be referred to here: > > > > > > https://lore.kernel.org/dri-devel/QMers3awXvNCQlyhWdTtsPwkp5ie9bze_hD5 > > > > > nAccFW7a_RXlWjYB7MoUW_8CKLT2bSQwIXVi5H6VULYIxCdgvryZoAoJnC5lZgyK1Q > > Wn48 > > > 8=@emersion.fr/ > > > > > > > > > Design considerations > > > = > > > > > > Following are the important aspects taken into account while designing > > > the current RFC > > > proposal: > > > > > > 1. Individual HW blocks can be muxed. (e.g. out of two HW blocks only > > > one > > can be used) > > > 2. Position of the HW block in the pipeline can be programmable > > > 3. LUTs can be one dimention
Re: [PATCH RFC v6 00/10] Support for Solid Fill Planes
On Mon, Aug 28, 2023 at 05:05:06PM -0700, Jessica Zhang wrote: > Some drivers support hardware that have optimizations for solid fill > planes. This series aims to expose these capabilities to userspace as > some compositors have a solid fill flag (ex. SOLID_COLOR in the Android > hardware composer HAL) that can be set by apps like the Android Gears > app. > > In order to expose this capability to userspace, this series will: > > - Introduce solid_fill and pixel_source properties to allow userspace to > toggle between FB and solid fill sources > - Loosen NULL FB checks within the DRM atomic commit callstack to allow > for NULL FB when solid fill is enabled. > - Add NULL FB checks in methods where FB was previously assumed to be > non-NULL > - Have MSM DPU driver use drm_plane_state.solid_fill instead of > dpu_plane_state.color_fill > > Note: The solid fill planes feature depends on both the solid_fill *and* > pixel_source properties. > > To use this feature, userspace can set the solid_fill property to a blob > containing the appropriate version number and solid fill color (in > RGB323232 format) and and setting the pixel_source property to > DRM_PLANE_PIXEL_SOURCE_COLOR. This will disable memory fetch and the > resulting plane will display the color specified by the solid_fill blob. > > Currently, there's only one version of the solid_fill blob property. > However if other drivers want to support a similar feature, but require > more than just the solid fill color, they can extend this feature by > creating additional versions of the drm_solid_fill struct. > > This 2 property approach was chosen because passing in a special 1x1 FB > with the necessary color information would have unecessary overhead that > does not reflect the behavior of the solid fill feature. In addition, > assigning the solid fill blob to FB_ID would require loosening some core > drm_property checks that might cause unwanted side effects elsewhere. The cover letter is a bit outdated by now. Anyway, with Pekkas issues addressed the core drm parts are Acked-by: Sebastian Wick > --- > Changes in v6: > - Have _dpu_plane_color_fill() take in a single ABGR color instead > of having separate alpha and BGR color parameters (Dmitry) > - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check > in SetPlane ioctl (Dmitry) > - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian) > - Dropped versioning from solid fill property blob (Dmitry) > - Use DRM_ENUM_NAME_FN (Dmitry) > - Use drm_atomic_replace_property_blob_from_id() (Dmitry) > - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry) > - Group redundant NULL FB checks (Dmitry) > - Squashed drm_plane_needs_disable() implementation with > DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian) > - Add comment to support RGBA solid fill color in the future (Dmitry) > - Link to v5: > https://lore.kernel.org/r/20230728-solid-fill-v5-0-053dbefa9...@quicinc.com > > Changes in v5: > - Added support for PIXEL_SOURCE_NONE (Sebastian) > - Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't > set (Dmitry) > - Added debugfs support for both properties (Dmitry) > - Corrected u32 to u8 conversion (Pekka) > - Moved drm_solid_fill_info struct and related documentation to > include/uapi (Pekka) > - Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka) > - Added more detailed UAPI and kernel documentation (Pekka) > - Reordered patch series so that the pixel_source property is introduced > before solid_fill (Dmitry) > - Fixed inconsistent ABGR/RGBA format declaration (Pekka) > - Reset pixel_source to FB in drm_mode_setplane() (Dmitry) > - Rename supported_sources to extra_sources (Dmitry) > - Only destroy old solid_fill blob state if new state is valid (Pekka) > - Link to v4: > https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa7...@quicinc.com > > Changes in v4: > - Rebased onto latest kernel > - Reworded cover letter for clarity (Dmitry) > - Reworded commit messages for clarity > - Split existing changes into smaller commits > - Added pixel_source enum property (Dmitry, Pekka, Ville) > - Updated drm-kms comment docs with pixel_source and solid_fill > properties (Dmitry) > - Inlined drm_atomic_convert_solid_fill_info() (Dmitry) > - Passed in plane state alpha value to _dpu_plane_color_fill_pipe() > - Link to v3: > https://lore.kernel.org/r/20230104234036.636-1-quic_jessz...@quicinc.com > > Changes in v3: > - Fixed some logic errors in atomic checks (Dmitry) > - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper > methods (Dmitry) > - Fixed typo in drm_solid_fill struct documentation &
Re: [PATCH RFC v6 02/10] drm: Introduce solid fill DRM plane property
On Mon, Aug 28, 2023 at 05:05:08PM -0700, Jessica Zhang wrote:
> Document and add support for solid_fill property to drm_plane. In
> addition, add support for setting and getting the values for solid_fill.
>
> To enable solid fill planes, userspace must assign a property blob to
> the "solid_fill" plane property containing the following information:
>
> struct drm_mode_solid_fill {
> u32 r, g, b;
> };
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 9
> drivers/gpu/drm/drm_atomic_uapi.c | 26 ++
> drivers/gpu/drm/drm_blend.c | 30 ++
> include/drm/drm_blend.h | 1 +
> include/drm/drm_plane.h | 36
> +++
> include/uapi/drm/drm_mode.h | 24 +
> 6 files changed, 126 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 01638c51ce0a..86fb876efbe6 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
>
> + if (plane_state->solid_fill_blob) {
> + drm_property_blob_put(plane_state->solid_fill_blob);
> + plane_state->solid_fill_blob = NULL;
> + }
> +
> if (plane->color_encoding_property) {
> if (!drm_object_property_get_default_value(>base,
>
> plane->color_encoding_property,
> @@ -336,6 +341,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct
> drm_plane *plane,
> if (state->fb)
> drm_framebuffer_get(state->fb);
>
> + if (state->solid_fill_blob)
> + drm_property_blob_get(state->solid_fill_blob);
> +
> state->fence = NULL;
> state->commit = NULL;
> state->fb_damage_clips = NULL;
> @@ -385,6 +393,7 @@ void __drm_atomic_helper_plane_destroy_state(struct
> drm_plane_state *state)
> drm_crtc_commit_put(state->commit);
>
> drm_property_blob_put(state->fb_damage_clips);
> + drm_property_blob_put(state->solid_fill_blob);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 454f980e16c9..1cae596ab693 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -316,6 +316,20 @@ drm_atomic_set_crtc_for_connector(struct
> drm_connector_state *conn_state,
> }
> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>
> +static void drm_atomic_set_solid_fill_prop(struct drm_plane_state *state)
> +{
> + struct drm_mode_solid_fill *user_info;
> +
> + if (!state->solid_fill_blob)
> + return;
> +
> + user_info = (struct drm_mode_solid_fill *)state->solid_fill_blob->data;
> +
> + state->solid_fill.r = user_info->r;
> + state->solid_fill.g = user_info->g;
> + state->solid_fill.b = user_info->b;
> +}
> +
> static void set_out_fence_for_crtc(struct drm_atomic_state *state,
> struct drm_crtc *crtc, s32 __user *fence_ptr)
> {
> @@ -546,6 +560,15 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> state->src_h = val;
> } else if (property == plane->pixel_source_property) {
> state->pixel_source = val;
> + } else if (property == plane->solid_fill_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >solid_fill_blob,
> + val, sizeof(struct drm_mode_solid_fill),
> + -1, );
> + if (ret)
> + return ret;
> +
> + drm_atomic_set_solid_fill_prop(state);
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -620,6 +643,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->src_h;
> } else if (property == plane->pixel_source_property) {
> *val = state->pixel_source;
> + } else if (property == plane->solid_fill_property) {
> + *val = state->solid_fill_blob ?
> + state->solid_fill_blob->base.id : 0;
> } else if (property == plane->alpha_property) {
> *val = state->alpha;
> } else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index c3c57bae06b7..273021cc21c8 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -200,6 +200,10 @@
Re: [PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes
On Tue, Aug 15, 2023 at 03:57:09PM -0300, André Almeida wrote:
> Given that prop changes may lead to modesetting, which would defeat the
> fast path of the async flip, refuse any atomic prop change for async
> flips in atomic API. The only exceptions are the framebuffer ID to flip
> to and the mode ID, that could be referring to an identical mode.
FYI, the solid fill series adds an enum drm_plane_pixel_source and and a
new solid fill pixel source. Changing the solid fill color would be
effectively the same as changing the FB_ID. On the other hand, changing
the FB_ID no longer necessarily results in an update when the pixel
source is set to solid fill.
> Signed-off-by: André Almeida
> ---
> v5: no changes
> v4: new patch
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 5 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 52 +++--
> drivers/gpu/drm/drm_crtc_internal.h | 2 +-
> drivers/gpu/drm/drm_mode_object.c | 2 +-
> 4 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 292e38eb6218..b34e3104afd1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -629,6 +629,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> WARN_ON(!drm_modeset_is_locked(>mutex));
>
> if (!drm_mode_equal(_crtc_state->mode,
> _crtc_state->mode)) {
> + if (new_crtc_state->async_flip) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] no mode
> changes allowed during async flip\n",
> +crtc->base.id, crtc->name);
> + return -EINVAL;
> + }
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a15121e75a0a..6c423a7e8c7b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1006,13 +1006,28 @@ int drm_atomic_connector_commit_dpms(struct
> drm_atomic_state *state,
> return ret;
> }
>
> +static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t
> prop_value,
> + struct drm_property *prop)
> +{
> + if (ret != 0 || old_val != prop_value) {
> + drm_dbg_atomic(prop->dev,
> +"[PROP:%d:%s] No prop can be changed during
> async flip\n",
> +prop->base.id, prop->name);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> int drm_atomic_set_property(struct drm_atomic_state *state,
> struct drm_file *file_priv,
> struct drm_mode_object *obj,
> struct drm_property *prop,
> - uint64_t prop_value)
> + uint64_t prop_value,
> + bool async_flip)
> {
> struct drm_mode_object *ref;
> + uint64_t old_val;
> int ret;
>
> if (!drm_property_change_valid_get(prop, prop_value, ))
> @@ -1029,6 +1044,13 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + if (async_flip) {
> + ret = drm_atomic_connector_get_property(connector,
> connector_state,
> + prop, _val);
> + ret = drm_atomic_check_prop_changes(ret, old_val,
> prop_value, prop);
> + break;
> + }
> +
> ret = drm_atomic_connector_set_property(connector,
> connector_state, file_priv,
> prop, prop_value);
> @@ -1037,6 +1059,7 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> case DRM_MODE_OBJECT_CRTC: {
> struct drm_crtc *crtc = obj_to_crtc(obj);
> struct drm_crtc_state *crtc_state;
> + struct drm_mode_config *config = >dev->mode_config;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state)) {
> @@ -1044,6 +1067,18 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + /*
> + * We allow mode_id changes here for async flips, because we
> + * check later on drm_atomic_helper_check_modeset() callers if
> + * there are modeset changes or they are equal
> + */
> + if (async_flip && prop != config->prop_mode_id) {
> + ret = drm_atomic_crtc_get_property(crtc, crtc_state,
> +
Re: [PATCH RFC v5 01/10] drm: Introduce pixel_source DRM plane property
On Mon, Aug 7, 2023 at 7:52 PM Jessica Zhang wrote:
>
>
>
> On 8/4/2023 6:15 AM, Sebastian Wick wrote:
> > On Fri, Jul 28, 2023 at 7:03 PM Jessica Zhang
> > wrote:
> >>
> >> Add support for pixel_source property to drm_plane and related
> >> documentation. In addition, force pixel_source to
> >> DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
> >> legacy userspace.
> >>
> >> This enum property will allow user to specify a pixel source for the
> >> plane. Possible pixel sources will be defined in the
> >> drm_plane_pixel_source enum.
> >>
> >> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_NONE and
> >> DRM_PLANE_PIXEL_SOURCE_FB with *_PIXEL_SOURCE_FB being the default value.
> >>
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> >> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> >> drivers/gpu/drm/drm_blend.c | 85
> >> +++
> >> drivers/gpu/drm/drm_plane.c | 3 ++
> >> include/drm/drm_blend.h | 2 +
> >> include/drm/drm_plane.h | 21
> >> 6 files changed, 116 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> index 784e63d70a42..01638c51ce0a 100644
> >> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
> >> drm_plane_state *plane_state,
> >>
> >> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> >> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >>
> >> if (plane->color_encoding_property) {
> >> if (!drm_object_property_get_default_value(>base,
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index d867e7f9f2cd..454f980e16c9 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -544,6 +544,8 @@ static int drm_atomic_plane_set_property(struct
> >> drm_plane *plane,
> >> state->src_w = val;
> >> } else if (property == config->prop_src_h) {
> >> state->src_h = val;
> >> + } else if (property == plane->pixel_source_property) {
> >> + state->pixel_source = val;
> >> } else if (property == plane->alpha_property) {
> >> state->alpha = val;
> >> } else if (property == plane->blend_mode_property) {
> >> @@ -616,6 +618,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >> *val = state->src_w;
> >> } else if (property == config->prop_src_h) {
> >> *val = state->src_h;
> >> + } else if (property == plane->pixel_source_property) {
> >> + *val = state->pixel_source;
> >> } else if (property == plane->alpha_property) {
> >> *val = state->alpha;
> >> } else if (property == plane->blend_mode_property) {
> >> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> >> index 6e74de833466..c500310a3d09 100644
> >> --- a/drivers/gpu/drm/drm_blend.c
> >> +++ b/drivers/gpu/drm/drm_blend.c
> >> @@ -185,6 +185,21 @@
> >>* plane does not expose the "alpha" property, then this is
> >>* assumed to be 1.0
> >>*
> >> + * pixel_source:
> >> + * pixel_source is set up with
> >> drm_plane_create_pixel_source_property().
> >> + * It is used to toggle the active source of pixel data for the plane.
> >> + * The plane will only display data from the set pixel_source -- any
> >> + * data from other sources will be ignored.
> >> + *
> >> + * Possible values:
> >> + *
> >> + * "NONE":
> >> + * No active pixel source.
> >> + * Committing with a NONE pixel source will disable the plane.
> >> + *
> >> + * "FB":
> >> + * Framebuffer
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
wrote:
>
> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang wrote:
> >
> > Document and add support for solid_fill property to drm_plane. In
> > addition, add support for setting and getting the values for solid_fill.
> >
> > To enable solid fill planes, userspace must assign a property blob to
> > the "solid_fill" plane property containing the following information:
> >
> > struct drm_mode_solid_fill {
> > u32 version;
> > u32 r, g, b;
> > };
> >
> > Signed-off-by: Jessica Zhang
> > ---
> > drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> > drivers/gpu/drm/drm_atomic_uapi.c | 55
> > +++
> > drivers/gpu/drm/drm_blend.c | 30 +
> > include/drm/drm_blend.h | 1 +
> > include/drm/drm_plane.h | 35
> > include/uapi/drm/drm_mode.h | 24 ++
> > 6 files changed, 154 insertions(+)
> >
>
> [skipped most of the patch]
>
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 43691058d28f..53c8efa5ad7f 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> > char name[DRM_DISPLAY_MODE_LEN];
> > };
> >
> > +/**
> > + * struct drm_mode_solid_fill - User info for solid fill planes
> > + *
> > + * This is the userspace API solid fill information structure.
> > + *
> > + * Userspace can enable solid fill planes by assigning the plane
> > "solid_fill"
> > + * property to a blob containing a single drm_mode_solid_fill struct
> > populated with an RGB323232
> > + * color and setting the pixel source to "SOLID_FILL".
> > + *
> > + * For information on the plane property, see
> > drm_plane_create_solid_fill_property()
> > + *
> > + * @version: Version of the blob. Currently, there is only support for
> > version == 1
> > + * @r: Red color value of single pixel
> > + * @g: Green color value of single pixel
> > + * @b: Blue color value of single pixel
> > + */
> > +struct drm_mode_solid_fill {
> > + __u32 version;
> > + __u32 r;
> > + __u32 g;
> > + __u32 b;
>
> Another thought about the drm_mode_solid_fill uABI. I still think we
> should add alpha here. The reason is the following:
>
> It is true that we have drm_plane_state::alpha and the plane's
> "alpha" property. However it is documented as "the plane-wide opacity
> [...] It can be combined with pixel alpha. The pixel values in the
> framebuffers are expected to not be pre-multiplied by the global alpha
> associated to the plane.".
>
> I can imagine a use case, when a user might want to enable plane-wide
> opacity, set "pixel blend mode" to "Coverage" and then switch between
> partially opaque framebuffer and partially opaque solid-fill without
> touching the plane's alpha value.
The only reason I see against this is that there might be some
hardware which supports only RGB but not alpha on planes and they
could then not use this property. Maybe another COLOR_FILL enum value
with alpha might be better? Maybe just doing the alpha via the alpha
property is good enough.
>
> --
> With best wishes
> Dmitry
>
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Mon, Jul 31, 2023 at 6:01 AM Dmitry Baryshkov
wrote:
>
> On 28/07/2023 20:02, Jessica Zhang wrote:
> > Document and add support for solid_fill property to drm_plane. In
> > addition, add support for setting and getting the values for solid_fill.
> >
> > To enable solid fill planes, userspace must assign a property blob to
> > the "solid_fill" plane property containing the following information:
> >
> > struct drm_mode_solid_fill {
> > u32 version;
> > u32 r, g, b;
> > };
> >
> > Signed-off-by: Jessica Zhang
> > ---
> > drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> > drivers/gpu/drm/drm_atomic_uapi.c | 55
> > +++
> > drivers/gpu/drm/drm_blend.c | 30 +
> > include/drm/drm_blend.h | 1 +
> > include/drm/drm_plane.h | 35
> > include/uapi/drm/drm_mode.h | 24 ++
> > 6 files changed, 154 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> > b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 01638c51ce0a..86fb876efbe6 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> > drm_plane_state *plane_state,
> > plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> > plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >
> > + if (plane_state->solid_fill_blob) {
> > + drm_property_blob_put(plane_state->solid_fill_blob);
> > + plane_state->solid_fill_blob = NULL;
> > + }
> > +
> > if (plane->color_encoding_property) {
> > if (!drm_object_property_get_default_value(>base,
> >
> > plane->color_encoding_property,
> > @@ -336,6 +341,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct
> > drm_plane *plane,
> > if (state->fb)
> > drm_framebuffer_get(state->fb);
> >
> > + if (state->solid_fill_blob)
> > + drm_property_blob_get(state->solid_fill_blob);
> > +
> > state->fence = NULL;
> > state->commit = NULL;
> > state->fb_damage_clips = NULL;
> > @@ -385,6 +393,7 @@ void __drm_atomic_helper_plane_destroy_state(struct
> > drm_plane_state *state)
> > drm_crtc_commit_put(state->commit);
> >
> > drm_property_blob_put(state->fb_damage_clips);
> > + drm_property_blob_put(state->solid_fill_blob);
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 454f980e16c9..039686c78c2a 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct
> > drm_connector_state *conn_state,
> > }
> > EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
> >
> > +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
> > + struct drm_property_blob *blob)
> > +{
> > + int blob_version;
> > +
> > + if (blob == state->solid_fill_blob)
> > + return 0;
> > +
> > + if (blob) {
> > + struct drm_mode_solid_fill *user_info = (struct
> > drm_mode_solid_fill *)blob->data;
> > +
> > + if (blob->length != sizeof(struct drm_mode_solid_fill)) {
> > + drm_dbg_atomic(state->plane->dev,
> > +"[PLANE:%d:%s] bad solid fill blob
> > length: %zu\n",
> > +state->plane->base.id,
> > state->plane->name,
> > +blob->length);
> > + return -EINVAL;
> > + }
> > +
> > + blob_version = user_info->version;
>
> I remember that we had versioning for quite some time. However it stroke
> me while reviewing that we do not a way to negotiate supported
> SOLID_FILL versions. However as we now have support for different
> pixel_source properties, maybe we can drop version completely. If at
> some point a driver needs to support different kind of SOLID_FILL
> property (consider v2), we can add new pixel source to the enum.
Agreed. Let's drop the versioning.
>
> > +
> > + /* Add more versions if necessary */
> > + if (blob_version == 1) {
> > + state->solid_fill.r = user_info->r;
> > + state->solid_fill.g = user_info->g;
> > + state->solid_fill.b = user_info->b;
> > + } else {
> > + drm_dbg_atomic(state->plane->dev,
> > +"[PLANE:%d:%s] unsupported solid fill
> > version (version=%d)\n",
> > +state->plane->base.id,
> > state->plane->name,
> > +
Re: [PATCH RFC v5 01/10] drm: Introduce pixel_source DRM plane property
On Fri, Jul 28, 2023 at 7:03 PM Jessica Zhang wrote:
>
> Add support for pixel_source property to drm_plane and related
> documentation. In addition, force pixel_source to
> DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
> legacy userspace.
>
> This enum property will allow user to specify a pixel source for the
> plane. Possible pixel sources will be defined in the
> drm_plane_pixel_source enum.
>
> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_NONE and
> DRM_PLANE_PIXEL_SOURCE_FB with *_PIXEL_SOURCE_FB being the default value.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_blend.c | 85
> +++
> drivers/gpu/drm/drm_plane.c | 3 ++
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 21
> 6 files changed, 116 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..01638c51ce0a 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
>
> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
>
> if (plane->color_encoding_property) {
> if (!drm_object_property_get_default_value(>base,
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd..454f980e16c9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -544,6 +544,8 @@ static int drm_atomic_plane_set_property(struct drm_plane
> *plane,
> state->src_w = val;
> } else if (property == config->prop_src_h) {
> state->src_h = val;
> + } else if (property == plane->pixel_source_property) {
> + state->pixel_source = val;
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -616,6 +618,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->src_w;
> } else if (property == config->prop_src_h) {
> *val = state->src_h;
> + } else if (property == plane->pixel_source_property) {
> + *val = state->pixel_source;
> } else if (property == plane->alpha_property) {
> *val = state->alpha;
> } else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 6e74de833466..c500310a3d09 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -185,6 +185,21 @@
> * plane does not expose the "alpha" property, then this is
> * assumed to be 1.0
> *
> + * pixel_source:
> + * pixel_source is set up with drm_plane_create_pixel_source_property().
> + * It is used to toggle the active source of pixel data for the plane.
> + * The plane will only display data from the set pixel_source -- any
> + * data from other sources will be ignored.
> + *
> + * Possible values:
> + *
> + * "NONE":
> + * No active pixel source.
> + * Committing with a NONE pixel source will disable the plane.
> + *
> + * "FB":
> + * Framebuffer source set by the "FB_ID" property.
> + *
> * Note that all the property extensions described here apply either to the
> * plane or the CRTC (e.g. for the background color, which currently is not
> * exposed and assumed to be black).
> @@ -615,3 +630,73 @@ int drm_plane_create_blend_mode_property(struct
> drm_plane *plane,
> return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> +
> +/**
> + * drm_plane_create_pixel_source_property - create a new pixel source
> property
> + * @plane: DRM plane
> + * @extra_sources: Bitmask of additional supported pixel_sources for the
> driver.
> + *DRM_PLANE_PIXEL_SOURCE_FB always be enabled as a supported
> + *source.
> + *
> + * This creates a new property describing the current source of pixel data
> for the
> + * plane. The pixel_source will be initialized as DRM_PLANE_PIXEL_SOURCE_FB
> by default.
> + *
> + * Drivers can set a custom default source by overriding the pixel_source
> value in
> + * drm_plane_funcs.reset()
> + *
> + * The property is exposed to userspace as an enumeration property called
> + * "pixel_source" and has the following enumeration values:
> + *
> + * "NONE":
> + * No active pixel source
> + *
> + * "FB":
> + * Framebuffer
Re: [PATCH v4 1/6] drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
On Sat, Jul 1, 2023 at 4:09 AM André Almeida wrote:
>
> From: Simon Ser
>
> If the driver supports it, allow user-space to supply the
> DRM_MODE_PAGE_FLIP_ASYNC flag to request an async page-flip.
> Set drm_crtc_state.async_flip accordingly.
>
> Document that drivers will reject atomic commits if an async
> flip isn't possible. This allows user-space to fall back to
> something else. For instance, Xorg falls back to a blit.
> Another option is to wait as close to the next vblank as
> possible before performing the page-flip to reduce latency.
>
> Signed-off-by: Simon Ser
> Reviewed-by: Alex Deucher
> Co-developed-by: André Almeida
> Signed-off-by: André Almeida
> ---
> v4: no changes
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 28 +---
> include/uapi/drm/drm_mode.h | 9 +
> 2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd..dfd4cf7169df 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1286,6 +1286,18 @@ static void complete_signaling(struct drm_device *dev,
> kfree(fence_state);
> }
>
> +static void
> +set_async_flip(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + int i;
> +
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> + crtc_state->async_flip = true;
> + }
> +}
> +
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
> @@ -1326,9 +1338,16 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> }
>
> if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
> - drm_dbg_atomic(dev,
> - "commit failed: invalid flag
> DRM_MODE_PAGE_FLIP_ASYNC\n");
> - return -EINVAL;
> + if (!dev->mode_config.async_page_flip) {
> + drm_dbg_atomic(dev,
> + "commit failed:
> DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
> + return -EINVAL;
> + }
> + if (dev->mode_config.atomic_async_page_flip_not_supported) {
> + drm_dbg_atomic(dev,
> + "commit failed:
> DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n");
> + return -EINVAL;
> + }
> }
>
> /* can't test and expect an event at the same time. */
> @@ -1426,6 +1445,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> if (ret)
> goto out;
>
> + if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)
> + set_async_flip(state);
> +
> if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> ret = drm_atomic_check_only(state);
> } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 46becedf5b2f..56342ba2c11a 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -949,6 +949,15 @@ struct hdr_output_metadata {
> * Request that the page-flip is performed as soon as possible, ie. with no
> * delay due to waiting for vblank. This may cause tearing to be visible on
> * the screen.
> + *
> + * When used with atomic uAPI, the driver will return an error if the
> hardware
> + * doesn't support performing an asynchronous page-flip for this update.
> + * User-space should handle this, e.g. by falling back to a regular
> page-flip.
> + *
> + * Note, some hardware might need to perform one last synchronous page-flip
> + * before being able to switch to asynchronous page-flips. As an exception,
> + * the driver will return success even though that first page-flip is not
> + * asynchronous.
What would happen if one commits another async KMS update before the
first page flip? Does one receive EAGAIN, does it amend the previous
commit? What happens to the timing feedback?
This seems really risky to include tbh. I would prefer if we would not
add such special cases for now.
> */
> #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> --
> 2.41.0
>
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Fri, Jun 30, 2023 at 11:27 PM Jessica Zhang
wrote:
>
>
>
> On 6/30/2023 7:43 AM, Sebastian Wick wrote:
> > On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang
> > wrote:
> >>
> >> Add support for pixel_source property to drm_plane and related
> >> documentation.
> >>
> >> This enum property will allow user to specify a pixel source for the
> >> plane. Possible pixel sources will be defined in the
> >> drm_plane_pixel_source enum.
> >>
> >> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> >> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
> >>
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> >> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> >> drivers/gpu/drm/drm_blend.c | 81
> >> +++
> >> include/drm/drm_blend.h | 2 +
> >> include/drm/drm_plane.h | 21
> >> 5 files changed, 109 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> index fe14be2bd2b2..86fb876efbe6 100644
> >> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
> >> drm_plane_state *plane_state,
> >>
> >> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> >> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >>
> >> if (plane_state->solid_fill_blob) {
> >> drm_property_blob_put(plane_state->solid_fill_blob);
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index a28b4ee79444..6e59c21af66b 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct
> >> drm_plane *plane,
> >> drm_property_blob_put(solid_fill);
> >>
> >> return ret;
> >> + } else if (property == plane->pixel_source_property) {
> >> + state->pixel_source = val;
> >> } else if (property == plane->alpha_property) {
> >> state->alpha = val;
> >> } else if (property == plane->blend_mode_property) {
> >> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >> } else if (property == plane->solid_fill_property) {
> >> *val = state->solid_fill_blob ?
> >> state->solid_fill_blob->base.id : 0;
> >> + } else if (property == plane->pixel_source_property) {
> >> + *val = state->pixel_source;
> >> } else if (property == plane->alpha_property) {
> >> *val = state->alpha;
> >> } else if (property == plane->blend_mode_property) {
> >> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> >> index 38c3c5d6453a..8c100a957ee2 100644
> >> --- a/drivers/gpu/drm/drm_blend.c
> >> +++ b/drivers/gpu/drm/drm_blend.c
> >> @@ -189,6 +189,18 @@
> >>* solid_fill is set up with drm_plane_create_solid_fill_property().
> >> It
> >>* contains pixel data that drivers can use to fill a plane.
> >>*
> >> + * pixel_source:
> >> + * pixel_source is set up with
> >> drm_plane_create_pixel_source_property().
> >> + * It is used to toggle the source of pixel data for the plane.
> >> + *
> >> + * Possible values:
> >> + *
> >> + * "FB":
> >> + * Framebuffer source
> >> + *
> >> + * "COLOR":
> >> + * solid_fill source
> >> + *
> >>* Note that all the property extensions described here apply either to
> >> the
> >>* plane or the CRTC (e.g. for the background color, which currently is
> >> not
> >>* exposed and assumed to be black).
> >> @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct
> >> drm_plane *plane)
> >> return 0;
> >
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang wrote:
>
> Add support for pixel_source property to drm_plane and related
> documentation.
>
> This enum property will allow user to specify a pixel source for the
> plane. Possible pixel sources will be defined in the
> drm_plane_pixel_source enum.
>
> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_blend.c | 81
> +++
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 21
> 5 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index fe14be2bd2b2..86fb876efbe6 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
>
> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
>
> if (plane_state->solid_fill_blob) {
> drm_property_blob_put(plane_state->solid_fill_blob);
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a28b4ee79444..6e59c21af66b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane
> *plane,
> drm_property_blob_put(solid_fill);
>
> return ret;
> + } else if (property == plane->pixel_source_property) {
> + state->pixel_source = val;
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> } else if (property == plane->solid_fill_property) {
> *val = state->solid_fill_blob ?
> state->solid_fill_blob->base.id : 0;
> + } else if (property == plane->pixel_source_property) {
> + *val = state->pixel_source;
> } else if (property == plane->alpha_property) {
> *val = state->alpha;
> } else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 38c3c5d6453a..8c100a957ee2 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -189,6 +189,18 @@
> * solid_fill is set up with drm_plane_create_solid_fill_property(). It
> * contains pixel data that drivers can use to fill a plane.
> *
> + * pixel_source:
> + * pixel_source is set up with drm_plane_create_pixel_source_property().
> + * It is used to toggle the source of pixel data for the plane.
> + *
> + * Possible values:
> + *
> + * "FB":
> + * Framebuffer source
> + *
> + * "COLOR":
> + * solid_fill source
> + *
> * Note that all the property extensions described here apply either to the
> * plane or the CRTC (e.g. for the background color, which currently is not
> * exposed and assumed to be black).
> @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct
> drm_plane *plane)
> return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
> +
> +/**
> + * drm_plane_create_pixel_source_property - create a new pixel source
> property
> + * @plane: drm plane
> + * @supported_sources: bitmask of supported pixel_sources for the driver (NOT
> + * including DRM_PLANE_PIXEL_SOURCE_FB, as it will be
> supported
> + * by default).
> + *
> + * This creates a new property describing the current source of pixel data
> for the
> + * plane.
> + *
> + * The property is exposed to userspace as an enumeration property called
> + * "pixel_source" and has the following enumeration values:
> + *
> + * "FB":
> + * Framebuffer pixel source
> + *
> + * "COLOR":
> + * Solid fill color pixel source
Can we add a "NONE" value?
I know it has been discussed earlier if we *need* this and I don't
think we do. I just think it would be better API design to disable
planes this way than having to know that a framebuffer pixel source
with a NULL framebuffer disables the plane. Obviously also keep the
old behavior for backwards compatibility.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_plane_create_pixel_source_property(struct drm_plane *plane,
> + unsigned int supported_sources)
> +{
> +
Re: [RFC] Plane color pipeline KMS uAPI
On Fri, May 5, 2023 at 10:40 PM Dave Airlie wrote: > > On Fri, 5 May 2023 at 01:23, Simon Ser wrote: > > > > Hi all, > > > > The goal of this RFC is to expose a generic KMS uAPI to configure the color > > pipeline before blending, ie. after a pixel is tapped from a plane's > > framebuffer and before it's blended with other planes. With this new uAPI we > > aim to reduce the battery life impact of color management and HDR on mobile > > devices, to improve performance and to decrease latency by skipping > > composition on the 3D engine. This proposal is the result of discussions at > > the Red Hat HDR hackfest [1] which took place a few days ago. Engineers > > familiar with the AMD, Intel and NVIDIA hardware have participated in the > > discussion. > > > > This proposal takes a prescriptive approach instead of a descriptive > > approach. > > Drivers describe the available hardware blocks in terms of low-level > > mathematical operations, then user-space configures each block. We decided > > against a descriptive approach where user-space would provide a high-level > > description of the colorspace and other parameters: we want to give more > > control and flexibility to user-space, e.g. to be able to replicate exactly > > the > > color pipeline with shaders and switch between shaders and KMS pipelines > > seamlessly, and to avoid forcing user-space into a particular color > > management > > policy. > > I'm not 100% sold on the prescriptive here, let's see if someone can > get me over the line with some questions later. > > My feeling is color pipeline hw is not a done deal, and that hw > vendors will be revising/evolving/churning the hw blocks for a while > longer, as there is no real standards in the area to aim for, all the > vendors are mostly just doing whatever gets Windows over the line and > keeps hw engineers happy. So I have some concerns here around forwards > compatibility and hence the API design. > > I guess my main concern is if you expose a bunch of hw blocks and > someone comes up with a novel new thing, will all existing userspace > work, without falling back to shaders? > Do we have minimum guarantees on what hardware blocks have to be > exposed to build a useable pipeline? > If a hardware block goes away in a new silicon revision, do I have to > rewrite my compositor? or will it be expected that the kernel will > emulate the old pipelines on top of whatever new fancy thing exists. I think there are two answers to those questions. The first one is that right now KMS already doesn't guarantee that every property is supported on all hardware. The guarantee we have is that properties that are supported on a piece of hardware on a specific kernel will be supported on the same hardware on later kernels. The color pipeline is no different here. For a specific piece of hardware a newer kernel might only change the pipelines in a backwards compatible way and add new pipelines. So to answer your question: if some hardware with a novel pipeline will show up it might not be supported and that's fine. We already have cases where some hardware does not support the gamma lut property but only the CSC property and that breaks night light because we never bothered to write a shader fallback. KMS provides ways to offload work but a generic user space always has to provide a fallback and this doesn't change. Hardware specific user space on the other hand will keep working with the forward compatibility guarantees we want to provide. The second answer is that we want to provide a user space library which takes a description of a color pipeline and tries to map that to the available KMS color pipelines. If there is a novel color operation, adding support in this library would then make it possible to offload compatible color pipelines on this new hardware for all consumers of the library. Obviously there is no guarantee that whatever color pipeline compositors come up with can actually be realized on specific hardware but that's just an inherent hardware issue. > We are not Android, or even Steam OS on a Steamdeck, we have to be > able to independently update the kernel for new hardware and not > require every compositor currently providing HDR to need to support > new hardware blocks and models at the same time. > > Dave. >
Re: [RFC] Plane color pipeline KMS uAPI
On Fri, May 5, 2023 at 5:28 PM Daniel Vetter wrote:
>
> On Thu, May 04, 2023 at 03:22:59PM +, Simon Ser wrote:
> > Hi all,
> >
> > The goal of this RFC is to expose a generic KMS uAPI to configure the color
> > pipeline before blending, ie. after a pixel is tapped from a plane's
> > framebuffer and before it's blended with other planes. With this new uAPI we
> > aim to reduce the battery life impact of color management and HDR on mobile
> > devices, to improve performance and to decrease latency by skipping
> > composition on the 3D engine. This proposal is the result of discussions at
> > the Red Hat HDR hackfest [1] which took place a few days ago. Engineers
> > familiar with the AMD, Intel and NVIDIA hardware have participated in the
> > discussion.
> >
> > This proposal takes a prescriptive approach instead of a descriptive
> > approach.
> > Drivers describe the available hardware blocks in terms of low-level
> > mathematical operations, then user-space configures each block. We decided
> > against a descriptive approach where user-space would provide a high-level
> > description of the colorspace and other parameters: we want to give more
> > control and flexibility to user-space, e.g. to be able to replicate exactly
> > the
> > color pipeline with shaders and switch between shaders and KMS pipelines
> > seamlessly, and to avoid forcing user-space into a particular color
> > management
> > policy.
>
> Ack on the prescriptive approach, but generic imo. Descriptive pretty much
> means you need the shaders at the same api level for fallback purposes,
> and we're not going to have that ever in kms. That would need something
> like hwc in userspace to work.
Which would be nice to have but that would be forcing a specific color
pipeline on everyone and we explicitly want to avoid that. There are
just too many trade-offs to consider.
> And not generic in it's ultimate consquence would mean we just do a blob
> for a crtc with all the vendor register stuff like adf (android display
> framework) does, because I really don't see a point in trying a
> generic-looking-but-not vendor uapi with each color op/stage split out.
>
> So from very far and pure gut feeling, this seems like a good middle
> ground in the uapi design space we have here.
Good to hear!
> > We've decided against mirroring the existing CRTC properties
> > DEGAMMA_LUT/CTM/GAMMA_LUT onto KMS planes. Indeed, the color management
> > pipeline can significantly differ between vendors and this approach cannot
> > accurately abstract all hardware. In particular, the availability, ordering
> > and
> > capabilities of hardware blocks is different on each display engine. So,
> > we've
> > decided to go for a highly detailed hardware capability discovery.
> >
> > This new uAPI should not be in conflict with existing standard KMS
> > properties,
> > since there are none which control the pre-blending color pipeline at the
> > moment. It does conflict with any vendor-specific properties like
> > NV_INPUT_COLORSPACE or the patches on the mailing list adding AMD-specific
> > properties. Drivers will need to either reject atomic commits configuring
> > both
> > uAPIs, or alternatively we could add a DRM client cap which hides the vendor
> > properties and shows the new generic properties when enabled.
> >
> > To use this uAPI, first user-space needs to discover hardware capabilities
> > via
> > KMS objects and properties, then user-space can configure the hardware via
> > an
> > atomic commit. This works similarly to the existing KMS uAPI, e.g. planes.
> >
> > Our proposal introduces a new "color_pipeline" plane property, and a new KMS
> > object type, "COLOROP" (short for color operation). The "color_pipeline"
> > plane
> > property is an enum, each enum entry represents a color pipeline supported
> > by
> > the hardware. The special zero entry indicates that the pipeline is in
> > "bypass"/"no-op" mode. For instance, the following plane properties
> > describe a
> > primary plane with 2 supported pipelines but currently configured in bypass
> > mode:
> >
> > Plane 10
> > ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
> > ├─ …
> > └─ "color_pipeline": enum {0, 42, 52} = 0
>
> A bit confused, why is this an enum, and not just an immutable prop that
> points at the first element? You already can disable elements with the
> bypass thing, also bypassing by changing the pointers to the next node in
> the graph seems a bit confusing and redundant.
We want to allow multiple pipelines to exist and a plane can choose
the pipeline by selecting the first element of the pipeline. The enum
here lists all the possible pipelines that can be attached to the
surface.
> > The non-zero entries describe color pipelines as a linked list of COLOROP
> > KMS
> > objects. The entry value is an object ID pointing to the head of the linked
> > list (the first operation in the color pipeline).
> >
> > The new COLOROP objects also
Re: [ANNOUNCE] pixfmtdb
On Mon, Jan 23, 2023 at 11:43 PM Simon Ser wrote: > > On Monday, January 23rd, 2023 at 21:25, Sebastian Wick > wrote: > > > Why is the TF defined for GL formats and both the primaries and TF for > > Vulkan formats? The only exception here should be sRGB formats. Where > > did you get the information from? > > This is what upstream dfdutils does [1]. Can you explain why you think > it should be undefined instead of linear? The channels have no meaning. You can put whatever you want in there. It doesn't have to be linear, it doesn't have to be colors and especially not colors with specific primaries. It's only when it's used to form an image that the TF and primaries are known. Vulkan specifically requires you to explicitly provide this information to the WSI and YCC samplers (generally everywhere where knowing them is required) and never assumes that certain pixel formats imply certain TFs and primaries (exception being the sRGB variants). (See also https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#_issues_26, Q 23) The problem here seems to be that the Data Format spec describes more than the pixel format. If you want to share an image, the TF and primaries are essential but they are not an inherent part of the pixel format of the image. So yeah, I think what dfdutils does is... probably not wrong but not what you're after. > > I was wondering what to do for DRM formats regarding these. I think it > would be worthwhile to do like Vulkan: set TF = linear, primaries = > BT.709, pre-multiplied alpha = yes. These are the things KMS assume > when there is no override (ie, when there is no KMS property saying > otherwise). Please no. All undefined is absolutely the right thing to do. Adding any more meaning to pixel formats is a horrible idea. The KMS properties are also not an override, they describe the image and the description has default values. > > [1]: > https://github.com/KhronosGroup/dfdutils/blob/5cd41cbdf63e80b00c085c6906a1152709e4c0f2/createdfd.c#L47 >
Re: [ANNOUNCE] pixfmtdb
Why is the TF defined for GL formats and both the primaries and TF for Vulkan formats? The only exception here should be sRGB formats. Where did you get the information from? On Mon, Jan 23, 2023 at 4:51 PM Laurent Pinchart wrote: > > CC'ing the linux-media mailing list. > > On Mon, Jan 23, 2023 at 02:10:58PM +, Simon Ser wrote: > > Hi all, > > > > In the last few days I've been working on a small new project, pixfmtdb [1]. > > It's a Web database of pixel format guides, it can be useful to understand > > how pixels are laid out in memory for a given format and which formats from > > various APIs are compatible with each other. > > > > pixfmtdb relies on the Khronos Data Format Specification [2] to describe > > each format. This means that each format is described with a standardized > > data blob, which can be re-used with other tools for other purposes. > > > > My future plans include adding more formats and format families to pixfmtdb, > > and make it easy to use the data for code generation, in particular for > > automatically generating tables containing metadata about formats, as used > > in Wayland compositors and other projects. > > > > I hope some of you can find this work useful, > > > > Simon > > > > [1]: https://pixfmtdb.emersion.fr > > [2]: https://www.khronos.org/dataformat > > -- > Regards, > > Laurent Pinchart >
Re: [RFC PATCH v2 3/3] drm/msm/dpu: Use color_fill property for DPU planes
On Wed, Jan 4, 2023 at 2:10 AM Jessica Zhang wrote:
>
>
>
> On 12/22/2022 7:12 PM, Dmitry Baryshkov wrote:
> > On 23/12/2022 00:14, Jessica Zhang wrote:
> >> Initialize and use the color_fill properties for planes in DPU driver. In
> >> addition, relax framebuffer requirements within atomic commit path and
> >> add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
> >> as it's unused.
> >>
> >> Changes since V2:
> >> - Fixed dropped 'const' warning
> >> - Dropped use of solid_fill_format
> >> - Switched to using drm_plane_solid_fill_enabled helper method
> >> - Added helper to convert color fill to BGR888 (Rob)
> >> - Added support for solid fill on planes of varying sizes
> >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> >>
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++-
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++-
> >> 2 files changed, 49 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> index 13ce321283ff..0695b70ea1b7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> @@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >> struct drm_plane_state *state;
> >> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> >> struct dpu_plane_state *pstate = NULL;
> >> +const struct msm_format *fmt;
> >> struct dpu_format *format;
> >> struct dpu_hw_ctl *ctl = mixer->lm_ctl;
> >> @@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >> sspp_idx - SSPP_VIG0,
> >> state->fb ? state->fb->base.id : -1);
> >> -format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> >> +if (pstate->base.fb)
> >> +fmt = msm_framebuffer_format(pstate->base.fb);
> >> +else
> >> +fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
> >> +DRM_FORMAT_ABGR, 0);
> >> +
> >> +format = to_dpu_format(fmt);
> >> if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> >> bg_alpha_enable = true;
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> index 86719020afe2..51a7507373f7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> @@ -44,7 +44,6 @@
> >> #define DPU_NAME_SIZE 12
> >> -#define DPU_PLANE_COLOR_FILL_FLAGBIT(31)
> >> #define DPU_ZPOS_MAX 255
> >> /* multirect rect index */
> >> @@ -105,7 +104,6 @@ struct dpu_plane {
> >> enum dpu_sspp pipe;
> >> struct dpu_hw_pipe *pipe_hw;
> >> -uint32_t color_fill;
> >> bool is_error;
> >> bool is_rt_pipe;
> >> const struct dpu_mdss_cfg *catalog;
> >> @@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct
> >> dpu_plane *pdpu,
> >> _cfg);
> >> }
> >> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill
> >> solid_fill)
> >> +{
> >> +uint32_t ret = 0;
> >> +
> >> +ret |= ((uint8_t) solid_fill.b) << 16;
> >> +ret |= ((uint8_t) solid_fill.g) << 8;
> >> +ret |= ((uint8_t) solid_fill.r);
> >> +
> >> +return ret;
> >> +}
> >> +
> >> /**
> >>* _dpu_plane_color_fill - enables color fill on plane
> >>* @pdpu: Pointer to DPU plane object
> >> @@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >> dst = drm_plane_state_dest(new_plane_state);
> >> -fb_rect.x2 = new_plane_state->fb->width;
> >> -fb_rect.y2 = new_plane_state->fb->height;
> >> +if (new_plane_state->fb) {
> >> +fb_rect.x2 = new_plane_state->fb->width;
> >> +fb_rect.y2 = new_plane_state->fb->height;
> >> +}
> >> max_linewidth = pdpu->catalog->caps->max_linewidth;
> >> -fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +if (new_plane_state->fb)
> >> +fmt =
> >> to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +else
> >> +fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
> >> min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >> @@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >> return -EINVAL;
> >> /* check src bounds */
> >> -} else if (!dpu_plane_validate_src(, _rect, min_src_size)) {
> >> +} else if (new_plane_state->fb && !dpu_plane_validate_src(,
> >> _rect, min_src_size)) {
> >> DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
> >> DRM_RECT_ARG());
> >> return -E2BIG;
> >> @@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
> >> if (pdpu->is_error)
> >> /* force white frame with
Re: The state of Quantization Range handling
On Wed, Nov 16, 2022 at 1:34 PM Pekka Paalanen wrote: > > On Tue, 15 Nov 2022 00:11:56 +0100 > Sebastian Wick wrote: > > > There are still regular bug reports about monitors (sinks) and sources > > disagreeing about the quantization range of the pixel data. In > > particular sources sending full range data when the sink expects > > limited range. From a user space perspective, this is all hidden in > > the kernel. We send full range data to the kernel and then hope it > > does the right thing but as the bug reports show: some combinations of > > displays and drivers result in problems. > > > > In general the whole handling of the quantization range on linux is > > not defined or documented at all. User space sends full range data > > because that's what seems to work most of the time but technically > > this is all undefined and user space can not fix those issues. Some > > compositors have resorted to giving users the option to choose the > > quantization range but this really should only be necessary for > > straight up broken hardware. > > > > Quantization Range can be explicitly controlled by AVI InfoFrame or > > HDMI General Control Packets. This is the ideal case and when the > > source uses them there is not a lot that can go wrong. Not all > > displays support those explicit controls in which case the chosen > > video format (IT, CE, SD; details in CTA-861-H 5.1) influences which > > quantization range the sink expects. > > > > This means that we have to expect that sometimes we have to send > > limited and sometimes full range content. The big question however > > that is not answered in the docs: who is responsible for making sure > > the data is in the correct range? Is it the kernel or user space? > > > > If it's the kernel: does user space supply full range or limited range > > content? Each of those has a disadvantage. If we send full range > > content and the driver scales it down to limited range, we can't use > > the out-of-range bits to transfer information. If we send limited > > range content and the driver scales it up we lose information. > > > > Either way, this must be documented. My suggestion is to say that the > > kernel always expects full range data as input and is responsible for > > scaling it to limited range data if the sink expects limited range > > data. > > Hi Sebastian, > > you are proposing the that driver/hardware will do either no range > conversion, or full-to-limited range conversion. Limited-to-full range > conversion would never be supported. > > I still wonder if limited-to-full range conversion could be useful with > video content. > > > Another problem is that some displays do not behave correctly. It must > > be possible to override the kernel when the user detects such a > > situation. This override then controls if the driver converts the full > > range data coming from the client or not (Default, Force Limited, > > Force Full). It does not try to control what range the sink expects. > > Let's call this the Quantization Range Override property which should > > be implemented by all drivers. > > In other words, a CRTC "quantization range conversion" property with > values: > - auto, with the assumption that color pipeline always produces full-range > - identity > - full-to-limited > (- limited-to-full) > > If this property was truly independent of the metadata being sent to > the sink, and of the framebuffer format, it would allow us to do four > ways: both full/limited framebuffer on both full/limited sink. It would > allow us to send sub-blacks and super-whites as well. > > More precisely, framebuffers would always have *undefined* quantization > range. The configuration of the color pipeline then determines how that > data is manipulated into a video signal. > > So I am advocating the same design as with color spaces: do not tell > KMS what your colorspaces are. Instead tell KMS what operations it > needs to do with the pixel data, and what metadata to send to the sink. > > > All drivers should make sure their behavior is correct: > > > > * check that drivers choose the correct default quantization range for > > the selected mode > > Mode implying a quantization range is awkward, but maybe the kernel > established modes should just have a flag for it. Then userspace would > know. Unless the video mode system is extended to communicate > IT/CE/SD/VIC and whatnot to userspace, making the modes better defined. > Then userspace would know too. > > > * whenever explicit control is available, use it and set the > > quan
Re: The state of Quantization Range handling
Hi Yussuf, On Tue, Nov 15, 2022 at 5:26 PM Yussuf Khalil wrote: > > Hello Sebastian, > > I've previously done some work on this topic [1]. My efforts were mostly about > fixing the situation regarding overrides and providing proper means for > userspace. I am affected by the issue myself as I own several DELL U2414H > screens that declare a CE mode as their preferred one, but should receive full > range data nonetheless. They do not have the respective bit set in their EDID > to indicate full range support either. > > My implementation primarily moved the "Broadcast RGB" to DRM core and re-wired > it in i915 and gma500. I further added a flag to indicate CE modes to > userspace > so that apps such as gnome-control-center can clearly communicate whether full > or limited range would be used by default. A v2 branch that I never submitted > is available at [2]. I also have some code lying around locally that adds the > required functionality to mutter and gnome-control-center. Yeah, I now agree that moving the "Broadcast RGB" to DRM core would be a good decision. The slight behavior change I want to see can be done afterwards as well. Not so sure about indicating CE modes because there are other factors (YCC vs RGB, the connector type and version) which influence the default quantization range. > > I had to pause work on the issue back then and never really came around to > picking it up again, however, I would be interested in working on it again if > there is consensus on the direction that my patches laid out. I did not > consider use cases for the out-of-range bits though. I think we can safely ignore out-of-range bits for now and good to know you're on board. > > Regards > Yussuf > > [1] > https://patchwork.kernel.org/project/dri-devel/cover/20200413214024.46500-1-...@pp3345.net/ > [2] https://github.com/pp3345/linux/commits/rgb-quant-range-v2 > > On 15.11.22 00:11, Sebastian Wick wrote: > > There are still regular bug reports about monitors (sinks) and sources > > disagreeing about the quantization range of the pixel data. In > > particular sources sending full range data when the sink expects > > limited range. From a user space perspective, this is all hidden in > > the kernel. We send full range data to the kernel and then hope it > > does the right thing but as the bug reports show: some combinations of > > displays and drivers result in problems. > > > > In general the whole handling of the quantization range on linux is > > not defined or documented at all. User space sends full range data > > because that's what seems to work most of the time but technically > > this is all undefined and user space can not fix those issues. Some > > compositors have resorted to giving users the option to choose the > > quantization range but this really should only be necessary for > > straight up broken hardware. > > > > Quantization Range can be explicitly controlled by AVI InfoFrame or > > HDMI General Control Packets. This is the ideal case and when the > > source uses them there is not a lot that can go wrong. Not all > > displays support those explicit controls in which case the chosen > > video format (IT, CE, SD; details in CTA-861-H 5.1) influences which > > quantization range the sink expects. > > > > This means that we have to expect that sometimes we have to send > > limited and sometimes full range content. The big question however > > that is not answered in the docs: who is responsible for making sure > > the data is in the correct range? Is it the kernel or user space? > > > > If it's the kernel: does user space supply full range or limited range > > content? Each of those has a disadvantage. If we send full range > > content and the driver scales it down to limited range, we can't use > > the out-of-range bits to transfer information. If we send limited > > range content and the driver scales it up we lose information. > > > > Either way, this must be documented. My suggestion is to say that the > > kernel always expects full range data as input and is responsible for > > scaling it to limited range data if the sink expects limited range > > data. > > > > Another problem is that some displays do not behave correctly. It must > > be possible to override the kernel when the user detects such a > > situation. This override then controls if the driver converts the full > > range data coming from the client or not (Default, Force Limited, > > Force Full). It does not try to control what range the sink expects. > > Let's call this the Quantization Range Override property which should > > be implemented by all
Re: The state of Quantization Range handling
Hi Dave, I noticed that I didn't get the Broadcast RGB property thanks to you (more below) On Tue, Nov 15, 2022 at 2:16 PM Dave Stevenson wrote: > > Hi Sebastian > > Thanks for starting the conversation - it's stalled a number of times > previously. > > On Mon, 14 Nov 2022 at 23:12, Sebastian Wick > wrote: > > > > There are still regular bug reports about monitors (sinks) and sources > > disagreeing about the quantization range of the pixel data. In > > particular sources sending full range data when the sink expects > > limited range. From a user space perspective, this is all hidden in > > the kernel. We send full range data to the kernel and then hope it > > does the right thing but as the bug reports show: some combinations of > > displays and drivers result in problems. > > I'll agree that we as Raspberry Pi also get a number of bug reports > where sinks don't always look at the infoframes and misinterpret the > data. > > > In general the whole handling of the quantization range on linux is > > not defined or documented at all. User space sends full range data > > because that's what seems to work most of the time but technically > > this is all undefined and user space can not fix those issues. Some > > compositors have resorted to giving users the option to choose the > > quantization range but this really should only be necessary for > > straight up broken hardware. > > Wowsers! Making userspace worry about limited range data would be a > very weird decision in my view, so compositors should always deal in > full range data. Making this a user space problem is IMO the ideal way to deal with it but that's a bit harder to do (I'll answer that in the reply to Pekka). So let's just assume we all agree that user space only deals with full range data. > How would composition of multiple DRM planes work if some are limited > range and some are full but you want limited range output? Your > hardware needs to have CSC matrices to convert full range down to > limited range, and know that you want to use them to effectively > compose to limited range. > In fact you can't currently tell DRM that an RGB plane is limited > range - the values in enum drm_color_range are > DRM_COLOR_YCBCR_LIMITED_RANGE and DRM_COLOR_YCBCR_FULL_RANGE [1]. > > > Quantization Range can be explicitly controlled by AVI InfoFrame or > > HDMI General Control Packets. This is the ideal case and when the > > source uses them there is not a lot that can go wrong. Not all > > displays support those explicit controls in which case the chosen > > video format (IT, CE, SD; details in CTA-861-H 5.1) influences which > > quantization range the sink expects. > > > > This means that we have to expect that sometimes we have to send > > limited and sometimes full range content. The big question however > > that is not answered in the docs: who is responsible for making sure > > the data is in the correct range? Is it the kernel or user space? > > > > If it's the kernel: does user space supply full range or limited range > > content? Each of those has a disadvantage. If we send full range > > content and the driver scales it down to limited range, we can't use > > the out-of-range bits to transfer information. If we send limited > > range content and the driver scales it up we lose information. > > How often have you encountered the out-of-range bits being used? > Personally I've never come across it. Is it really that common? > If trying to pass non-video data from the client then you need to pray > there is no scaling or filtering during composition as it could > legitimately be corrupted. All true, and personally I've also never encountered this which is why I'd like to ignore all of that for now. > > > Either way, this must be documented. My suggestion is to say that the > > kernel always expects full range data as input and is responsible for > > scaling it to limited range data if the sink expects limited range > > data. > > AIUI That is the current situation. It certainly fits the way that all > our hardware works. > > > Another problem is that some displays do not behave correctly. It must > > be possible to override the kernel when the user detects such a > > situation. This override then controls if the driver converts the full > > range data coming from the client or not (Default, Force Limited, > > Force Full). It does not try to control what range the sink expects. > > Sorry, I'm not clear from the description. Is this a plane, crtc, or > connector property? > > "Data coming from the client" would imply a plane property only - > effectively extendin
The state of Quantization Range handling
There are still regular bug reports about monitors (sinks) and sources disagreeing about the quantization range of the pixel data. In particular sources sending full range data when the sink expects limited range. From a user space perspective, this is all hidden in the kernel. We send full range data to the kernel and then hope it does the right thing but as the bug reports show: some combinations of displays and drivers result in problems. In general the whole handling of the quantization range on linux is not defined or documented at all. User space sends full range data because that's what seems to work most of the time but technically this is all undefined and user space can not fix those issues. Some compositors have resorted to giving users the option to choose the quantization range but this really should only be necessary for straight up broken hardware. Quantization Range can be explicitly controlled by AVI InfoFrame or HDMI General Control Packets. This is the ideal case and when the source uses them there is not a lot that can go wrong. Not all displays support those explicit controls in which case the chosen video format (IT, CE, SD; details in CTA-861-H 5.1) influences which quantization range the sink expects. This means that we have to expect that sometimes we have to send limited and sometimes full range content. The big question however that is not answered in the docs: who is responsible for making sure the data is in the correct range? Is it the kernel or user space? If it's the kernel: does user space supply full range or limited range content? Each of those has a disadvantage. If we send full range content and the driver scales it down to limited range, we can't use the out-of-range bits to transfer information. If we send limited range content and the driver scales it up we lose information. Either way, this must be documented. My suggestion is to say that the kernel always expects full range data as input and is responsible for scaling it to limited range data if the sink expects limited range data. Another problem is that some displays do not behave correctly. It must be possible to override the kernel when the user detects such a situation. This override then controls if the driver converts the full range data coming from the client or not (Default, Force Limited, Force Full). It does not try to control what range the sink expects. Let's call this the Quantization Range Override property which should be implemented by all drivers. All drivers should make sure their behavior is correct: * check that drivers choose the correct default quantization range for the selected mode * whenever explicit control is available, use it and set the quantization range to full * make sure that the hardware converts from full range to limited range whenever the sink expects limited range * implement the Quantization Range Override property I'm volunteering for the documentation, UAPI and maybe even the drm core parts if there is willingness to tackle the issue. Appendix A: Broadcast RGB property A few drivers already implement the Broadcast RGB property to control the quantization range. However, it is pointless: It can be set to Auto, Full and Limited when the sink supports explicitly setting the quantization range. The driver expects full range content and converts it to limited range content when necessary. Selecting limited range never makes any sense: the out-of-range bits can't be used because the input is full range. Selecting Default never makes sense: relying on the default quantization range is risky because sinks often get it wrong and as we established there is no reason to select limited range if not necessary. The limited and full options also are not suitable as an override because the property is not available if the sink does not support explicitly setting the quantization range.
Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On Tue, Nov 8, 2022 at 7:51 PM Simon Ser wrote:
>
> cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
>
> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov
> wrote:
>
> > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > drm_plane. In addition, add support for setting and getting the values
> > > of these properties.
> > >
> > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > represents the format of the color fill. Userspace can set enable solid
> > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > framebuffer to NULL.
> > >
> > > Signed-off-by: Jessica Zhang
> >
> > Planes report supported formats using the drm_mode_getplane(). You'd
> > also need to tell userspace, which formats are supported for color fill.
> > I don't think one supports e.g. YV12.
> >
> > A bit of generic comment for the discussion (this is an RFC anyway).
> > Using color_fill/color_fill_format properties sounds simple, but this
> > might be not generic enough. Limiting color_fill to 32 bits would
> > prevent anybody from using floating point formats (e.g.
> > DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> > e.g. using 64-bit for the color_fill value, but then this doesn't sound
> > extensible too much.
> >
> > So, a question for other hardware maintainers. Do we have hardware that
> > supports such 'color filled' planes? Do we want to support format
> > modifiers for filling color/data? Because what I have in mind is closer
> > to the blob structure, which can then be used for filling the plane:
> >
> > struct color_fill_blob {
> > u32 pixel_format;
> > u64 modifiers4];
> > u32 pixel_data_size; // fixme: is this necessary?
> > u8 pixel_data[];
> > };
> >
> > And then... This sounds a lot like a custom framebuffer.
> >
> > So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> > flag to the framebuffers, which would e.g. mean that the FB gets stamped
> > all over the plane. This would also save us from changing if (!fb)
> > checks all over the drm core.
> >
> > Another approach might be using a format modifier instead of the FB flag.
> >
> > What do you think?
>
> First off, we only need to represent the value of a single pixel here. So I'm
> not quite following why we need format modifiers. Format modifiers describe
> how
> pixels are laid out in memory. Since there's a single pixel described, this
> is non-sensical to me, the format modifier is always LINEAR.
>
> Then, I can understand why putting the pixel_format in there is tempting to
> guarantee future extensibility, but it also adds complexity. For instance, how
> does user-space figure out which formats can be used for COLOR_FILL? Can
> user-space use any format supported by the plane? What does it mean for
> multi-planar formats? Do we really want the kernel to have conversion logic
> for
> all existing formats? Do we need to also add a new read-only blob prop to
> indicate supported COLOR_FILL formats?
>
> We've recently-ish standardized a new Wayland protocol [1] which has the same
> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
> values for each channel (R, G, B, A) would be enough for almost all use-cases.
> The driver can convert these high-precision values to what the hardware
> expects.
> The only concern was about sending values outside of the [0.0, 1.0] range,
> which may have HDR use-cases.
>
> So, there are multiple ways to go about this. I can think of:
>
> - Put "RGBA32" in the name of the prop, and if we ever need a different
> color format, pick a different name.
> - Define a struct with an enum of possible fill kinds:
> #define FILL_COLOR_RGBA32 1
> #define FILL_COLOR_F32 2
> struct color_fill_blob { u32 kind; u8 data[]; };
> - Define a struct with a version and RGBA values:
> struct color_fill_blob { u32 version; u32 rgba[4]; };
> If we need to add more formats later, or new metadata:
> struct color_fill_blob2 { u32 version; /* new fields */ };
> where version must be set to 2.
> - Define a struct with a "pixel_format" prop, but force user-space to use a
> fixed format for now. Later, if we need another format, add a new prop to
> advertise supported formats.
> - More complicated solutions, e.g. advertise the list of supported formats
> from
> the start.
>
> [1]:
> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>
Agreeing with most of what you said here. However, what's the idea
behind a format anyway? The 4 values provided here are fed directly
into the color pipeline which seems to define the color channels it's
working on as RGBA (or doesn't define anything at all). The only
reason I can think of is that hardware might support only ingesting
values either in a format
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen wrote: > > On Fri, 30 Sep 2022 17:44:17 +0300 > Ville Syrjälä wrote: > > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote: > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen > > > wrote: > > > > > > > > On Thu, 29 Sep 2022 20:06:50 +0200 > > > > Sebastian Wick wrote: > > > > > > > > > If it is supposed to be a non-linear luminance curve, which one is it? > > > > > It would be much clearer if user space can control linear luminance > > > > > and use whatever definition of perceived brightness it wants. The > > > > > obvious downside of it is that it requires bits to encode changes that > > > > > users can't perceive. What about backlights which only have a few > > > > > predefined luminance levels? How would user space differentiate > > > > > between the continuous and discrete backlight? What about > > > > > self-emitting displays? They usually increase the dynamic range when > > > > > they increase in brightness because the black level doesn't rise. They > > > > > also probably employ some tonemapping to adjust for that. What about > > > > > the range of the backlight? What about the absolute luminance of the > > > > > backlight? I want to know about that all in user space. > > > > > > > > > > I understand that most of the time the kernel doesn't have answers to > > > > > those questions right now but the API should account for all of that. > > > > > > > > Hi, > > > > > > > > if the API accounts for all that, and the kernel doesn't know, then how > > > > can the API not lie? If the API sometimes lies, how could we ever trust > > > > it at all? > > > > > > Make it possible for the API to say "I don't know". I'd much rather > > > have an API tell me explicitly what it does and doesn't know instead > > > of having to guess what data I can actually rely on. > > > > > > For example if the kernel knows the luminance is linear on one display > > > and doesn't know anything about the other display and it exposes them > > > both in the same way I can not possibly write any code which relies on > > > exact control over the luminance for either display. > > > > > > > > > > > Personally I have the feeling that if we can even get to the level of > > > > "each step in the value is a more or less perceivable change", that > > > > would be good enough. Think of UI, e.g. hotkeys to change brightness. > > > > You'd expect almost every press to change it a bit. > > > > > > The nice thing is that you can have that even if you have no further > > > information about the brightness control and it might be good enough > > > for some use cases but it isn't for others. > > > > > > > If an end user wants defined and controlled luminance, I'd suggest they > > > > need to profile (physically measure) the response of the display at > > > > hand. This is no different from color profiling displays, but you need > > > > a measurement device that produces absolute measurements if absolute > > > > control is what they want. > > > > > > If that's the kind of user experience you're after, good for you. I > > > certainly want things to work out of the box which makes this just a > > > big no-go. > > > > I think if we have the information to make the default behaviour > > better then we should do that. Ie. if the firmaware gives us a > > table to remap the values for a more linear response we should > > make use of that by default. > > But that's only like 20% of what Sebastian is asking for. > > What's "linear"? Radiometric or perceptual? > > Radiometric linear control would make a terrible UX, so if the control > is radiometric, userspace needs to remap it. That might be a good > thing, but it's also complicated, because the relationship between > brightness and luminance is somewhere between a power curve and > exponential curve. You need to make sure that the userspace remapping > works for different backlights with different luminance ranges. That's > not obvious to me. > > > We can of course provide a way for the user to plug in their own > > actually measured data later. But IMO that doesn't even have to > > happen in the initial implementation. Just need to avoid painting > >
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen wrote: > > On Thu, 29 Sep 2022 20:06:50 +0200 > Sebastian Wick wrote: > > > If it is supposed to be a non-linear luminance curve, which one is it? > > It would be much clearer if user space can control linear luminance > > and use whatever definition of perceived brightness it wants. The > > obvious downside of it is that it requires bits to encode changes that > > users can't perceive. What about backlights which only have a few > > predefined luminance levels? How would user space differentiate > > between the continuous and discrete backlight? What about > > self-emitting displays? They usually increase the dynamic range when > > they increase in brightness because the black level doesn't rise. They > > also probably employ some tonemapping to adjust for that. What about > > the range of the backlight? What about the absolute luminance of the > > backlight? I want to know about that all in user space. > > > > I understand that most of the time the kernel doesn't have answers to > > those questions right now but the API should account for all of that. > > Hi, > > if the API accounts for all that, and the kernel doesn't know, then how > can the API not lie? If the API sometimes lies, how could we ever trust > it at all? Make it possible for the API to say "I don't know". I'd much rather have an API tell me explicitly what it does and doesn't know instead of having to guess what data I can actually rely on. For example if the kernel knows the luminance is linear on one display and doesn't know anything about the other display and it exposes them both in the same way I can not possibly write any code which relies on exact control over the luminance for either display. > > Personally I have the feeling that if we can even get to the level of > "each step in the value is a more or less perceivable change", that > would be good enough. Think of UI, e.g. hotkeys to change brightness. > You'd expect almost every press to change it a bit. The nice thing is that you can have that even if you have no further information about the brightness control and it might be good enough for some use cases but it isn't for others. > If an end user wants defined and controlled luminance, I'd suggest they > need to profile (physically measure) the response of the display at > hand. This is no different from color profiling displays, but you need > a measurement device that produces absolute measurements if absolute > control is what they want. If that's the kind of user experience you're after, good for you. I certainly want things to work out of the box which makes this just a big no-go. > > If there ever becomes an industry standard and conformance test > definitions for luminance levels and backlight control, then things > could be different. But until that, I believe trying to make one in the > kernel is futile, because I have got the impression that there is > practically no consistency between different displays in general. I'm aware that this is the current situation but it's one that must change and we should at least try to create an API which still works when we get more and better data. > > Besides, I would expect some backlights to wear over time, grow dimmer > for the same input value. Without a physical active feedback loop > (measurements), it just won't work. > > If this is mostly for laptop displays, would end users even care? > > > Thanks, > pq > > > On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä > > wrote: > > > > > > On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote: > > > > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote: > > > > > On Fri, 09 Sep 2022, Hans de Goede wrote: > > > > > > Hi all, > > > > > > > > > > > > Here is v2 of my "drm/kms: control display brightness through > > > > > > drm_connector properties" RFC: > > ... > > > > > > > Unlike the /sys/class/backlight/foo/brightness this brightness > > > > > > property > > > > > > has a clear definition for the value 0. The kernel must ensure that > > > > > > 0 > > > > > > means minimum brightness (so 0 should _never_ turn the backlight > > > > > > off). > > > > > > If necessary the kernel must enforce a minimum value by adding > > > > > > an offset to the value seen in the property to ensure this behavior. > > > > > > > > > > > > For example if necessary the driver must clamp 0-255 to 10-255, > > > >
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
If it is supposed to be a non-linear luminance curve, which one is it? It would be much clearer if user space can control linear luminance and use whatever definition of perceived brightness it wants. The obvious downside of it is that it requires bits to encode changes that users can't perceive. What about backlights which only have a few predefined luminance levels? How would user space differentiate between the continuous and discrete backlight? What about self-emitting displays? They usually increase the dynamic range when they increase in brightness because the black level doesn't rise. They also probably employ some tonemapping to adjust for that. What about the range of the backlight? What about the absolute luminance of the backlight? I want to know about that all in user space. I understand that most of the time the kernel doesn't have answers to those questions right now but the API should account for all of that. On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä wrote: > > On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote: > > > On Fri, 09 Sep 2022, Hans de Goede wrote: > > > > Hi all, > > > > > > > > Here is v2 of my "drm/kms: control display brightness through > > > > drm_connector properties" RFC: > > > > > > > > Changes from version 1: > > > > - Drop bl_brightness_0_is_min_brightness from list of new connector > > > > properties. > > > > - Clearly define that 0 is always min-brightness when setting the > > > > brightness > > > > through the connector properties. > > > > - Drop bl_brightness_control_method from list of new connector > > > > properties. > > > > - Phase 1 of the plan has been completed > > > > > > > > As discussed already several times in the past: > > > > https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ > > > > > > > > https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ > > > > > > > > The current userspace API for brightness control offered by > > > > /sys/class/backlight devices has various issues: > > > > > > > > 1. There is no way to map the backlight device to a specific > > > >display-output / panel (1) > > > > 2. Controlling the brightness requires root-rights requiring > > > >desktop-environments to use suid-root helpers for this. > > > > 3. The meaning of 0 is not clearly defined, it can be either off, > > > >or minimum brightness at which the display is still readable > > > >(in a low light environment) > > > > 4. It's not possible to change both the gamma and the brightness in the > > > >same KMS atomic commit. You'd want to be able to reduce brightness to > > > >conserve power, and counter the effects of that by changing gamma to > > > >reach a visually similar image. And you'd want to have the changes > > > > take > > > >effect at the same time instead of reducing brightness at some frame > > > > and > > > >change gamma at some other frame. This is pretty much impossible to > > > > do > > > >via the sysfs interface. > > > > > > > > As already discussed on various conference's hallway tracks > > > > and as has been proposed on the dri-devel list once before (2), > > > > it seems that there is consensus that the best way to to solve these > > > > 2 issues is to add support for controlling a video-output's brightness > > > > through properties on the drm_connector. > > > > > > > > This RFC outlines my plan to try and actually implement this, > > > > which has 3 phases: > > > > > > > > > > > > Phase 1: Stop registering multiple /sys/class/backlight devs for a > > > > single display > > > > = > > > > > > > > On x86 there can be multiple firmware + direct-hw-access methods > > > > for controlling the backlight and in some cases the kernel registers > > > > multiple backlight-devices for a single internal laptop LCD panel. > > > > > > > > A plan to fix this was posted here: > > > > https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ > > > > And a pull-req actually implementing this plan has been send out this > > > > week: > > > > https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ > > > > > > > > > > > > Phase 2: Add drm_connector properties mirroring the matching backlight > > > > device > > > > = > > > > > > > > The plan is to add a drm_connector helper function, which optionally > > > > takes > > > > a pointer to the backlight device for the GPU's native backlight device, > > > > which will then mirror the backlight settings from the backlight device > > > > in a set of read/write brightness* properties on the connector. > > > > > > > > This function can then be called by GPU drivers for the drm_connector > > > > for > > > > the internal panel and it will then take care of
Re: surface-suspension wayland protcool development status?
On Wed, Nov 24, 2021 at 05:23:16PM -0800, Joshua Ashton wrote: > What exactly do you agree with? :V is a funny face in response to Sebastian's > typical idiocy. > > The timeout doesn't fix anything and still has problems for games that demand > a semi-consistent framerate (ie. Not hanging for seconds at a time) The protocol has been marketed as a way to save power/memory but at some point it became obvious that there is at least two other problems which people wanted to address with it: 1. indefinite blocking on present 2. fixed framerate presents (or at least something approximating that) The first problem can be solved in the WSI at the expense of power and memory. With this protocol or any other protocol which introduces surface "suspension" we can fix the power and memory issue later. This seems to be the issue Ethan was talking about and what I answered. The other issue is impossible to solve while relying on the frame callback mechanism: it is inherently and by design not a fixed rate event. It might have solved the issue for you on one compositor on a fixed refresh rate display but in general it is not sufficient. The present-timing vulkan ext and the corresponding wayland protocol should in theory give you the tools to implement the behavior you want but the interest in pushing that forward seems fairly low. I would be happy to help out if you decide to put some work in. > The correct solution ideally would be to keep ticking along at a constant > time for fifo and instantly return for mailbox/immediate when suspended in > Mesa. Apps that want to deal with suspension and use VK WSI can just handle > that on their surfaces and not present. > > As I understand it, nobody has had the time to implement or prototype that > yet. > > As for the protocol, I am not sure what is happening with it since I last > touched it. I haven't had the chance to address the nits to wording and such, > I've been busy with other things recently. > > - Josh > > On November 24, 2021 5:11:20 PM PST, "Ethan “flibitijibibo” Lee" > wrote: > >I agree with Joshua - if you think you can get the timeout implemented in > >EGL and Vulkan WSI I’ll go ahead and mark the SDL Wayland PR as ready to go > >for 2.0.20. Hypothetically it’d be nice to have an event for hidden vs. > >exposed but really we just want to avoid deadlocks. As far as independent > >studios care that model would do the job perfectly fine, not sure about the > >big AAA studios but they can e-mail the Proton team if they run into any > >trouble. > > > >-Ethan > > > >> On Nov 24, 2021, at 7:35 PM, Joshua Ashton wrote: > >> > >> :V > >> > >> On Wed, 24 Nov 2021 at 15:31, Sebastian Wick >> <mailto:sebast...@sebastianwick.net>> wrote: > >> On 2021-11-11 16:00, Neal Gompa wrote: > >> > Hey all, > >> > > >> > Is there a reason why the development of the surface-suspension > >> > protocol[1] has completely stalled out? It's been in the 30 day > >> > discussion period for a few months now and it's a pretty critical > >> > protocol for games (it's the main blocker for SDL to switch to Wayland > >> > by default[2]). > >> > >> FWIW I still believe this is fixable in EGL/WSI: timeout after 1s when > >> waiting for the wl_surface frame callback. > >> > >> > From a purely downstream perspective, I'd like to have Fedora Linux > >> > switch to Wayland by default for SDL-based applications (which we are > >> > able to do relatively quickly and easily since all SDL applications > >> > now use SDL2 since Fedora Linux 35[3]). > >> > > >> > Thanks in advance and best regards, > >> > Neal > >> > > >> > [1]: > >> > https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/99 > >> > > >> > <https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/99> > >> > [2]: https://github.com/libsdl-org/SDL/pull/4306#issuecomment-963513540 > >> > <https://github.com/libsdl-org/SDL/pull/4306#issuecomment-963513540> > >> > [3]: https://fedoraproject.org/wiki/Changes/SDL12onSDL2 > >> > <https://fedoraproject.org/wiki/Changes/SDL12onSDL2> > >> > > >> > -- > >> > 真実はいつも一つ！/ Always, there's only one truth! > > > > - Joshie
Re: surface-suspension wayland protcool development status?
On 2021-11-11 16:00, Neal Gompa wrote: Hey all, Is there a reason why the development of the surface-suspension protocol[1] has completely stalled out? It's been in the 30 day discussion period for a few months now and it's a pretty critical protocol for games (it's the main blocker for SDL to switch to Wayland by default[2]). FWIW I still believe this is fixable in EGL/WSI: timeout after 1s when waiting for the wl_surface frame callback. From a purely downstream perspective, I'd like to have Fedora Linux switch to Wayland by default for SDL-based applications (which we are able to do relatively quickly and easily since all SDL applications now use SDL2 since Fedora Linux 35[3]). Thanks in advance and best regards, Neal [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/99 [2]: https://github.com/libsdl-org/SDL/pull/4306#issuecomment-963513540 [3]: https://fedoraproject.org/wiki/Changes/SDL12onSDL2 -- 真実はいつも一つ！/ Always, there's only one truth!
Re: Best practices for client side buffer management
On 2020-06-24 13:14, Pekka Paalanen wrote: On Wed, 24 Jun 2020 19:17:57 +1000 Brad Robinson wrote: Hi All, @Guillermo: yep, that's exactly the same problem I'm thinking about. Hi, I answered to Guillermo as well further down. I had an idea that I'm wondering about the rendering model in my toolkit is essentially the same as Windows/OSX - where the application invalidates part of the window, and then at some later point the OS calls back with a message prompting the app to paint that part of the screen. ie: multiple invalidate calls are coalesced into a single paint call at a later time - typically just before entering the message loop's idle state. Could a Wayland app mimic this behaviour by having a single background buffer that contains the current window content? When the application invalidates the window it does one of two things depending on whether the buffer has been returned from the compositor yet. * If the buffer has been returned and no longer in use by the compositor, it queues a paint message to itself. * If the buffer hasn't been returned and is still in use by the compositor, set a flag on the window. When the buffer is returned from the compositor, it checks that flag and if set then it clears the flag and queues the paint message. The problem is, the compositor might not release the buffer until you have already submitted a new one. A compositor, that textures directly from the buffer you submitted as a client, cannot release the buffer until it gets a new buffer to replace it. A compositor may repaint at any time for any reason, so it must always have window content (some buffer) available. Compare this to X11 expose events: expose events require the client to draw the damaged area and until it does, the display will have garbage there. A Wayland compositor handles expose events completely internally, which means that no such garbage is ever shown on display. You can of course keep a shadow buffer as the main buffer, and copy appropriately to Wayland buffers when you update, but it consumes an extra buffer obviously. It would be preferable if your widget tree / scenegraph was able to draw arbitrary regions from scratch to arbitrary buffers on-demand, and not autonomously at "random" times, so you can draw directly into Wayland buffers exactly the parts that need drawing. Then you would also never throw any drawing away without showing it, so it's also an energy usage optimization. It's not actually that different from the window invalidation you describe, except it will be all completely internal to your toolkit and "the window" is actually "a buffer", and you need to keep track of damage history as well. More on that below. The paint message redraws all the invalidated (damaged) rectangles and re-submits it to the compositor and the whole cycle starts again. Obviously there's more logic around maintaining the dirty region, coalescing multiple invalidate/damage calls, an assumption that there's a mechanism to post a message via the message loop etc... but I hope this explains the idea. Would this work? I think the main requirement would be that: 1. the compositor doesn't change the contents of the buffer and that when it's returned it's still got the old content. With Wayland, wl_surface.attach does not allow the compositor to write into the buffer, it only allows reading. So that is safe, also because the client manages its own buffers (allocate, re-use, destroy). Isn't this a little bit more subtle? In particular if you use OpenGL in the compositor to access the image there might be layout transitions which change the image in place. So while the buffer is owned by the compositor the client must not read or write to it and when the ownership is transferred back to the client the image might be in another layout. Or am I missing something here? 2. that the compositor returns buffers it's finished with in a timely manner This is false as described above. It's not clear to me from what I've read that either of these points are true or safe assumptions. This would eliminate the need to manage multiple buffers, multiple dirty regions, the need to copy previously rendered content between buffers and fits nicely with the rendering model of Windows/OSX giving similar semantics on all platforms. Unfortunately you cannot avoid multi-buffering in a client that aims to be correct. If Wayland dictated that clients must be able to run fine with just one buffer per window, then quite likely the compositor would be forced to maintain a full copy of the window contents, or it would not be able to refresh the screen at all times, or it might end up showing transitional garbage (glitch). The Wayland design principle is "every frame is perfect", which more or less leads to "never show garbage", and "never stall waiting for clients to respond". On Wed, Jun 24, 2020 at 6:11 PM Guillermo Rodriguez Garcia <
[RFC wayland-protocols v4 1/1] unstable: add color management protocol
This protocol allows clients to attach a color space and rendering intent to a wl_surface. It further allows the client to be informed which color spaces a wl_surface was converted to on the last presented. Signed-off-by: Sebastian Wick --- Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 300 ++ 3 files changed, 305 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..80abc1d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,7 @@ unstable_protocols = \ unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \ unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \ unstable/primary-selection/primary-selection-unstable-v1.xml \ + unstable/color-management/color-management-unstable-v1.xml \ $(NULL) stable_protocols = \ diff --git a/unstable/color-management/README b/unstable/color-management/README new file mode 100644 index 000..140f1e9 --- /dev/null +++ b/unstable/color-management/README @@ -0,0 +1,4 @@ +Color management protocol + +Maintainers: +Sebastian Wick diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml new file mode 100644 index 000..cab44fc --- /dev/null +++ b/unstable/color-management/color-management-unstable-v1.xml @@ -0,0 +1,300 @@ + + + + + Copyright © 2019 Sebastian Wick + Copyright © 2019 Erwin Burema + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice (including the next + paragraph) shall be included in all copies or substantial portions of the + Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. + + + + This protocol specifies a way for a client to set the color space and + HDR metadata of a surface and to get information about the color spaces + and HDR capabilities of outputs. + + + + + A global interface used for getting color management surface and color + management output objects as well as creating color spaces from ICC + profiles. + + + + + These fatal protocol errors may be emitted in response to illegal color + management requests. + + + + + + + + Names that describe a complete (primaries, white point and EOTF), + well-known color space. + + + + + + + + Create a color space object from an ICC profile. + + + The description of the color space to create is send in the form of an + ICC profile as a file descriptor in the argument icc. The fd must be + compatible with O_NONBLOCK and the compositor will not modify it. + + The ICC profile version must be 2 or 4, it must be a 3 channel profile + and the class must be 'input', 'output', 'abstract' or 'display'. + + If the conditions are not met a protocol error 'invalid_icc_profile' is + raised by the compositor. + + If the color space described by the ICC profile has its well-known name + listed in color_space_name, that name can be set in the 'name' argument. + Otherwise 'name' must be set to 'none'. + + See the zwp_color_space_v1 interface for more details about the created + object. + + See the ICC specification for more details about ICC profiles. + + + + + + + + + This creates a new color zwp_color_management_output_v1 object for the + given wl_output. If get_color_management_output is called with a + wl_output that a
[RFC wayland-protocols v4 0/1] Color Management Protocol
New version with smaller changes and clarifications and a new feature: * Introduce enum color_space_name to optionally name a color space object when the color space is well-known. Even if the color space object is describing one of the well-known color spaces it may not be tagged with the name and is purely an augmentation. The reasoning here is that figuring out if a profile describes a well-known color space is not trivial. This might be useful for HDR displays. * tighten the requirements on the accepted ICC profiles * disallow creating multiple zwp_color_management_output/surface objects from the same wl_output/wl_surface * zwp_color_management_output/surface become inert when their associated wl_output/wl_surface is destroyed * rename zwp_* to zwp_*_v1 * reorder rendering intents * add relative+bpc intent * allow to reset the color space attached to a surface by passing NULL in set_color_space and comming the state * clearify that the color space attached to a surface is reset when the associated zwp_color_management_surface is destroyed * getting the color space information event now requires sending the get_information request * restrict the client usage of the ICC fd to mmaping with MAP_PRIVATE * clearify that the ICC fd send in color_space.information may not be the same as the one used to create the object There is two FIXMEs left for which I would also appreciate feedback * How exactly should the restriction on the ICC fd look like that gets send from the client to the compositor? In my Weston POC I use read and make sure the fd is O_NONBLOCK and construct a new independent fd representing the color space so the client fd can be released by the compositor. Mmaping would require handling SIGBUS again. Not sure what's better. * Should we actually mandate un-premultiplied data? I have to look at KMS plane blending, the graphics ROPs and custom blend shaders to figure out what they all do. The focus for the next version is probably going to be HDR. So I'd also appreciate if we can come to a concensus on the overall design of HDR support. I'm also still looking for people who want to become co-maintainers of the protocol. Previous version: https://lists.freedesktop.org/archives/wayland-devel/2019-March/040319.html Weston POC implementation: https://gitlab.freedesktop.org/swick/weston/commits/color-management-backend Sebastian Wick (1): unstable: add color management protocol Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 300 ++ 3 files changed, 305 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml -- 2.23.0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v6] unstable/drm-lease: DRM lease protocol support
On 2019-09-14 19:29, Erwin Burema wrote: On Tue, 03 Sep 2019 00:15:56 +0200 Sebastian Wick wrote: > On 2019-07-30 14:53, Drew DeVault wrote: > > From: Marius Vlad > > > > DRM leasing is a feature which allows the DRM master to "lease" a > > subset > > of its DRM resources to another DRM master via drmModeCreateLease, > > which > > returns a file descriptor for the new DRM master. We use this protocol > > to negotiate the terms of the lease and transfer this file descriptor > > to > > clients. > > > > In less DRM-specific terms: this protocol allows Wayland compositors to > > give over their GPU resources (like displays) to a Wayland client to > > exclusively control. > > > > The primary use-case for this is Virtual Reality headsets, which via > > the > > non-desktop DRM property are generally not used as desktop displays by > > Wayland compositors, and for latency reasons (among others) are most > > useful to games et al if they have direct control over the DRM > > resources > > associated with it. Basically, these are peripherals which are of no > > use > > to the compositor and may be of use to a client, but since they are > > tied > > up in DRM we need to use DRM leasing to get them into client's hands. > > Talking about use-cases, I'm evaluating leasing for display calibration > and profiling which requires leasing outputs which are are part of the > desktop and also requires user interaction (keyboard, mice). Are there > any ideas on how something like that would work? > > I think it would be great if it worked similar to fullscreen surfaces > but input handling depends on having a surface but leasing an output is > independent. Hi, Hi, Since Sebastian hasn't replied yet I think I can give some insight here, to be honest in my opinion using drm-lease for this is already a compromise (since Sebastian already shot down protocols that give any form of calibration/gamma curve access) I'd prefer not having to dictate how calibration/profiling has to work but we might not have a choice here and have to design a wayland protocol. To be clear, it's just a preference. sorry, I don't think that would work. Your measurement app must become a KMS app, which is something other color management people seemed to be quite opposed of. Compositors probably would not allow leasing outputs used for the desktop. You would not be able to get any input unless you had another output still controlled by the compositor where to show a window, or you had access to the kernel input devices which you don't. The biggest reason a straight KMS application won't work for our use case is that it would give horrible user experience in that you first need to get a vtty session and then launch the application from the terminal there, since this will be needed to do by people who are not necessarily the most computer savvy people and the calibration/profiling generally needs to be performed somewhat regularly (from once every quarter to once every month depending on required accuracy and screen drift). With DRM leasing we at least don't have to tell people they first need to switch terminal (which in the worst case can mean a need for logging out), for input I am not that concerned since the needed input can be done using a normal wayland application that then launches the KMS based calibrator/profiler (which is mostly an automatic process). Completely agree although some use-cases are not completely automatic they should still be doable with DRM leasing (if we can lease desktop outputs and get at least some input). There are also some other issues with a KMS bases calibrator/profiler is, one is that it is harder to program and there might be exposed HW differences that we would prefer not to deal with (this would still be an issue with DRM leasing), the other main one is that for extreme accuracy it would be nice to get the compositor involved so that during calibration/profiling the same curve is set as in usage. It's been discussed before if we want to measure the display or the compositor+display. Ideally it's the same but if the compositor somehow screws up it's a bug that should be fixed. With intel exposing color pipelines per plane those screw ups will probably be there sometimes and not some other time so measuring the screw up is not helpful either. If you want to write a KMS app for measurements, I'd recommend making it a stand-alone app, not something using DRM leases. That way you can assume there is no other program (usually a display server) already running and owning the input and output devices. As I said above during the profiling/calibration we don't need input (well from the tool used from profiling/calibrating but that is usually not a classical input device) Some use cases do
Re: [PATCH v6] unstable/drm-lease: DRM lease protocol support
On 2019-07-30 14:53, Drew DeVault wrote: From: Marius Vlad DRM leasing is a feature which allows the DRM master to "lease" a subset of its DRM resources to another DRM master via drmModeCreateLease, which returns a file descriptor for the new DRM master. We use this protocol to negotiate the terms of the lease and transfer this file descriptor to clients. In less DRM-specific terms: this protocol allows Wayland compositors to give over their GPU resources (like displays) to a Wayland client to exclusively control. The primary use-case for this is Virtual Reality headsets, which via the non-desktop DRM property are generally not used as desktop displays by Wayland compositors, and for latency reasons (among others) are most useful to games et al if they have direct control over the DRM resources associated with it. Basically, these are peripherals which are of no use to the compositor and may be of use to a client, but since they are tied up in DRM we need to use DRM leasing to get them into client's hands. Talking about use-cases, I'm evaluating leasing for display calibration and profiling which requires leasing outputs which are are part of the desktop and also requires user interaction (keyboard, mice). Are there any ideas on how something like that would work? I think it would be great if it worked similar to fullscreen surfaces but input handling depends on having a surface but leasing an output is independent. Signed-off-by: Marius Vlad Signed-off-by: Drew DeVault --- When implementing this for Xwayland, we realized that we would really like to be able to obtain a lease with zero connectors. The kernel does not support this today, but adding support shouldn't be especially difficult. v6 changes the protocol accordingly to allow for leases with zero connectors, though on today's kernels this will fail vaugely with the finished event. Makefile.am | 1 + unstable/drm-lease/README| 5 + unstable/drm-lease/drm-lease-unstable-v1.xml | 246 +++ 3 files changed, 252 insertions(+) create mode 100644 unstable/drm-lease/README create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..d9fff89 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4,6 +4,7 @@ unstable_protocols = \ unstable/pointer-gestures/pointer-gestures-unstable-v1.xml \ unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml \ unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml \ + unstable/drm-lease/drm-lease-unstable-v1.xml \ unstable/text-input/text-input-unstable-v1.xml \ unstable/text-input/text-input-unstable-v3.xml \ unstable/input-method/input-method-unstable-v1.xml \ diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README new file mode 100644 index 000..16f8551 --- /dev/null +++ b/unstable/drm-lease/README @@ -0,0 +1,5 @@ +Linux DRM lease + +Maintainers: +Drew DeVault +Marius Vlad diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml b/unstable/drm-lease/drm-lease-unstable-v1.xml new file mode 100644 index 000..083d004 --- /dev/null +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml @@ -0,0 +1,246 @@ + + + +Copyright © 2018 NXP +Copyright © 2019 Status Research Development GmbH. + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + + + + This protocol is used by Wayland compositors which act as Direct + Renderering Manager (DRM) masters to lease DRM resources to Wayland + clients. Once leased, the compositor will not use the leased resources + until the lease is
Re: [RFC PATCH v2 0/1] Color manager calibration protocol
On 2019-05-22 05:17, Erwin Burema wrote: On Tue, 21 May 2019 at 21:07, Sebastian Wick wrote: On 2019-05-21 19:32, Erwin Burema wrote: > Hi, > > This is the second version of my color manager calibration protocol, > biggest change is that I now use a surface this surface is set up in a > similar way to the touch screen calibration protocol. Another big > change is more (and hopefully better) documentation and the last big > one is a new way to set the calibration curve (plus some info needed > to use it). There are some other smaller changes many regarding > formatting. > > One thing I am not entirely satisfied with is the way I am setting the > calibration curve but decided to not wait for a brilliant idea to > strike me out of the blue and put it out here so more eyeballs can > have a look. And more brains can think about it. Hi Erwin, the approach still doesn't make sense to me. Why expose this specific part of the color pipeline, the VCGT, to the client? What is the advantage over simply passing e.g. a double to the compositor and the compositor then does the best to display that value at the highest precision it can. In previous discussions there was two arguments: 1. the VCGT might have higher precision than the frame buffer 2. so you can measure the thing you later actually use to show None of them seem to be valid. The compositor should know how to get the best precision out of the whole pipeline. The latter argument ignored that the compositor can use combinations of framebuffers, shaders, plane/crtc gamma, csc and degamma properties all with different precision for different parts of the screen in normal operation. And who knows, maybe we'll get more sophisticated scanout hardware. So what's the rationale behind this? Hi, Good question and to be honest I was at the point to remove the bitdepth information due to the above arguments when Graeme Gill replied with the following (direct quote) """ For accurate measurement any quantization of the color values needs to be known, as well as the effective display depth. The effective display depth may be different to the frame buffer depth if the frame buffer output passes through per channel lookup tables. Ideally the measurement would be at the pixel depth resolution of the output of the per channel tables. Naturally to make use of this, access to the per channel tables contents is necessary, to setup the mapping from the frame buffer value to the value sent to the display. """ So since the quantization needs to be known anyway apparently and we need access to the gamma tables (irregardless if those are implemented as shaders or not) I though this was the "best" solution. Hope this answers your question! I still don't understand how knowing the bit depth of the vcgt gets you anything. There is lots of other things between the vcgt and the display and you don't know their bit depth and what kind of quantization they introduces so it seems impossible to reason about what *exactly* the display will see in the end. And what exactly speaks against "please sent this value to the display in the highest resolution you can" and let the compositor figure out how to get the highest resolution? It knows more about the hardware than the client and you can't possibly expose all of those details to the client. > For those wondering why this is needed see here: > https://www.argyllcms.com/doc/monitorcontrols.html although with the > color manager protocol the first reason becomes moot (although it > wouldn't surprise me if a compositor would only implements this > protocol and not that one for reasons of simplicity or memory/disk > space savings) the other 2 are still valid. > > If this is seen as acceptable I will see if I can implement it in > Weston, although seeing as my day job is something completely > different (doing this as a hobbyist) and I am actually a physicist not > a software engineer I will probably need some help with that. > > > Erwin Burema (1): > Adding color calibration protocol > > .../wp_wayland_calibration.xml| 247 ++ > 1 file changed, 247 insertions(+) > create mode 100644 > unstable/color_calibration/wp_wayland_calibration.xml ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC PATCH v2 0/1] Color manager calibration protocol
On 2019-05-21 19:32, Erwin Burema wrote: Hi, This is the second version of my color manager calibration protocol, biggest change is that I now use a surface this surface is set up in a similar way to the touch screen calibration protocol. Another big change is more (and hopefully better) documentation and the last big one is a new way to set the calibration curve (plus some info needed to use it). There are some other smaller changes many regarding formatting. One thing I am not entirely satisfied with is the way I am setting the calibration curve but decided to not wait for a brilliant idea to strike me out of the blue and put it out here so more eyeballs can have a look. And more brains can think about it. Hi Erwin, the approach still doesn't make sense to me. Why expose this specific part of the color pipeline, the VCGT, to the client? What is the advantage over simply passing e.g. a double to the compositor and the compositor then does the best to display that value at the highest precision it can. In previous discussions there was two arguments: 1. the VCGT might have higher precision than the frame buffer 2. so you can measure the thing you later actually use to show None of them seem to be valid. The compositor should know how to get the best precision out of the whole pipeline. The latter argument ignored that the compositor can use combinations of framebuffers, shaders, plane/crtc gamma, csc and degamma properties all with different precision for different parts of the screen in normal operation. And who knows, maybe we'll get more sophisticated scanout hardware. So what's the rationale behind this? For those wondering why this is needed see here: https://www.argyllcms.com/doc/monitorcontrols.html although with the color manager protocol the first reason becomes moot (although it wouldn't surprise me if a compositor would only implements this protocol and not that one for reasons of simplicity or memory/disk space savings) the other 2 are still valid. If this is seen as acceptable I will see if I can implement it in Weston, although seeing as my day job is something completely different (doing this as a hobbyist) and I am actually a physicist not a software engineer I will probably need some help with that. Erwin Burema (1): Adding color calibration protocol .../wp_wayland_calibration.xml| 247 ++ 1 file changed, 247 insertions(+) create mode 100644 unstable/color_calibration/wp_wayland_calibration.xml ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC 0/1] Color manager calibration protocol v1
On April 17, 2019 12:38:15 PM GMT+02:00, Erwin Burema wrote: >On Wed, 17 Apr 2019 at 11:32, Pekka Paalanen >wrote: >> >> On Tue, 16 Apr 2019 23:42:30 +0200 >> Erwin Burema wrote: >> >> > On Tue, 16 Apr 2019 at 17:03, Pekka Paalanen >wrote: >> > > >> > > On Tue, 16 Apr 2019 13:33:02 + >> > > Erwin Burema wrote: >> > > >> > > > On Tuesday, 16 April 2019, Pekka Paalanen wrote: >> > > > > On Tue, 16 Apr 2019 13:11:06 +0200 >> > > > > Erwin Burema wrote: >> > > > > >> > > > > > On Tue, 16 Apr 2019 at 12:45, Pekka Paalanen > wrote: >> > > > > > > >> > > > > > > On Sun, 14 Apr 2019 12:57:47 +0200 >> > > > > > > Erwin Burema wrote: >> > > > > > > >> > > > > > > > Without a way to calibrate/profile screens an color >management >> > > > > > > > protocol looses a lot of its value. So to add this >missing feature I >> > > > > > > > wrote the following protocol. >> > > > > > > > >> > > > > > > > The idea is that the calibration/profiling SW only sets >the RGB >> > > > > > > > triplet and then the compositor is responsible to draw >a rectanglular >> > > > > > > > region on the selected output screen, since not all >calibration tools >> > > > > > > > will be at the center of the screen a user should be >able to modify >> > > > > > > > the placement of this rectanglular region. Unless >specified the >> > > > > > > > monitor profile (if any) should not be applied but the >GPU curve >> > > > > > > > should, currently to set a new curve the calibration >tool should >> > > > > > > > generate a new ICC profile with the wanted curve in the >VCGT tag (I >> > > > > > > > am not sure if this is the best option but would make >the most >> > > > > > > > universal one). In the end after profiling the last >uploaded ICC >> > > > > > > > could then be saved (although a compositor is not >required to honor >> > > > > > > > the request in that case it should send the not saved >error). If the >> > > > > > > > compositor doesn't save or the connection with this >protocol is >> > > > > > > > broken the compositor should restore previous settings. >> > > > > > > >> > > > > > > Hi, >> > > > > > > >> > > > > > > I only took a very quick glance, but I do like where this >design is >> > > > > > > going. I'll refrain from commenting on wl_surface vs. not >for now >> > > > > > > though. >> > > > > > > >> > > > > > > Forgive me my ignorance, but why is the "GPU curve" >needed to be a >> > > > > > > custom curve provided by the client? >> > > >> > > Hi, >> > > >> > > ok, below we are finally getting to the point. >> > > >> > > > > Ok, you start with an identity curve and iterate. Why only >the "GPU >> > > > > curve" instead of a "full" color correction transformation? >> > > > > >> > > > >> > > > Since we are trying to setup the "GPU curve" in this case a >full >> > > > transform would only get in the way. >> > > >> > > Why? >> > > >> > >> > Good question! >> >> Hi Erwin, >> >> there seems to be two very different cases, and below we discuss one >or >> the other paragraph by paragraph, which might be confusing. >> >> >> Case 1: Calibrating and/or profiling the monitor only >> >> This case is what I see as controlling the pixel values on the wire, >> and the identity LUT instead of "GPU curve" access. I explain why no >> "GPU curve" in my opinion further below. >> >> >> Case 2: Calibrating and/or profiling the full color pipeline >> from application to a specific monitor >> >> This would be using the normal compositor color mapping pipeline and >> adjusting the full output calibration and profile to achieve a >desired >> color reproduction. >> > >Case 2 does not exists you do NOT calibrate a specific application to >monitor. Once you have an output profile you do NOT need to calibrate >every application individual you just use a color management system to >map an input profile to an output profile. Case 2 won't work for the >same reason you can't calibrate/profile HDR screens that don't support >direct access (via something like a Freesync2_Gamma22 display mode), >it is hard to predict what the whole colormanager is going to do so >making profiles that way is a bad idea. I think he means the way you do profiling on X. simply writing values to a surface and measure the output. It measures not the display but the whole pipeline from the surface to the output. This is what Graeme wants. Its not a bad idea if you have a single static pipeline but we want to use all resource we have available and pipelines change depending on what is being shown. If there is an error in one pipeline and it wasn't used when profiling this has gained you nothing. I think the only reasonable thin to do is profile the actual monitor. Applying a lut at the end or not is another topic. >> >> These two are mutually exclusive approaches, so they have independent >> rationales and protocol requirements. I am still not sure which one >you >> want. In some old discussions with Graeme I think he advocated for >Case >> 2, so
Re: [RFC 0/1] Color manager calibration protocol v1
On April 17, 2019 12:38:15 PM GMT+02:00, Erwin Burema wrote: >On Wed, 17 Apr 2019 at 11:32, Pekka Paalanen >wrote: >> >> On Tue, 16 Apr 2019 23:42:30 +0200 >> Erwin Burema wrote: >> >> > On Tue, 16 Apr 2019 at 17:03, Pekka Paalanen >wrote: >> > > >> > > On Tue, 16 Apr 2019 13:33:02 + >> > > Erwin Burema wrote: >> > > >> > > > On Tuesday, 16 April 2019, Pekka Paalanen wrote: >> > > > > On Tue, 16 Apr 2019 13:11:06 +0200 >> > > > > Erwin Burema wrote: >> > > > > >> > > > > > On Tue, 16 Apr 2019 at 12:45, Pekka Paalanen > wrote: >> > > > > > > >> > > > > > > On Sun, 14 Apr 2019 12:57:47 +0200 >> > > > > > > Erwin Burema wrote: >> > > > > > > >> > > > > > > > Without a way to calibrate/profile screens an color >management >> > > > > > > > protocol looses a lot of its value. So to add this >missing feature I >> > > > > > > > wrote the following protocol. >> > > > > > > > >> > > > > > > > The idea is that the calibration/profiling SW only sets >the RGB >> > > > > > > > triplet and then the compositor is responsible to draw >a rectanglular >> > > > > > > > region on the selected output screen, since not all >calibration tools >> > > > > > > > will be at the center of the screen a user should be >able to modify >> > > > > > > > the placement of this rectanglular region. Unless >specified the >> > > > > > > > monitor profile (if any) should not be applied but the >GPU curve >> > > > > > > > should, currently to set a new curve the calibration >tool should >> > > > > > > > generate a new ICC profile with the wanted curve in the >VCGT tag (I >> > > > > > > > am not sure if this is the best option but would make >the most >> > > > > > > > universal one). In the end after profiling the last >uploaded ICC >> > > > > > > > could then be saved (although a compositor is not >required to honor >> > > > > > > > the request in that case it should send the not saved >error). If the >> > > > > > > > compositor doesn't save or the connection with this >protocol is >> > > > > > > > broken the compositor should restore previous settings. >> > > > > > > >> > > > > > > Hi, >> > > > > > > >> > > > > > > I only took a very quick glance, but I do like where this >design is >> > > > > > > going. I'll refrain from commenting on wl_surface vs. not >for now >> > > > > > > though. >> > > > > > > >> > > > > > > Forgive me my ignorance, but why is the "GPU curve" >needed to be a >> > > > > > > custom curve provided by the client? >> > > >> > > Hi, >> > > >> > > ok, below we are finally getting to the point. >> > > >> > > > > Ok, you start with an identity curve and iterate. Why only >the "GPU >> > > > > curve" instead of a "full" color correction transformation? >> > > > > >> > > > >> > > > Since we are trying to setup the "GPU curve" in this case a >full >> > > > transform would only get in the way. >> > > >> > > Why? >> > > >> > >> > Good question! >> >> Hi Erwin, >> >> there seems to be two very different cases, and below we discuss one >or >> the other paragraph by paragraph, which might be confusing. >> >> >> Case 1: Calibrating and/or profiling the monitor only >> >> This case is what I see as controlling the pixel values on the wire, >> and the identity LUT instead of "GPU curve" access. I explain why no >> "GPU curve" in my opinion further below. >> >> >> Case 2: Calibrating and/or profiling the full color pipeline >> from application to a specific monitor >> >> This would be using the normal compositor color mapping pipeline and >> adjusting the full output calibration and profile to achieve a >desired >> color reproduction. >> > >Case 2 does not exists you do NOT calibrate a specific application to >monitor. Once you have an output profile you do NOT need to calibrate >every application individual you just use a color management system to >map an input profile to an output profile. Case 2 won't work for the >same reason you can't calibrate/profile HDR screens that don't support >direct access (via something like a Freesync2_Gamma22 display mode), >it is hard to predict what the whole colormanager is going to do so >making profiles that way is a bad idea. I think he means the way you do profiling on X. simply writing values to a surface and measure the output. It measures not the display but the whole pipeline from the surface to the output. This is what Graeme wants. Its not a bad idea if you have a single static pipeline but we want to use all resource we have available and pipelines change depending on what is being shown. If there is an error in one pipeline and it wasn't used when profiling this has gained you nothing. I think the only reasonable thin to do is profile the actual monitor. Applying a lut at the end or not is another topic. >> >> These two are mutually exclusive approaches, so they have independent >> rationales and protocol requirements. I am still not sure which one >you >> want. In some old discussions with Graeme I think he advocated for >Case >> 2, so
Re: [RFC 0/1] Color manager calibration protocol v1
On 2019-04-16 12:45, Pekka Paalanen wrote: On Sun, 14 Apr 2019 12:57:47 +0200 Erwin Burema wrote: Without a way to calibrate/profile screens an color management protocol looses a lot of its value. So to add this missing feature I wrote the following protocol. The idea is that the calibration/profiling SW only sets the RGB triplet and then the compositor is responsible to draw a rectanglular region on the selected output screen, since not all calibration tools will be at the center of the screen a user should be able to modify the placement of this rectanglular region. Unless specified the monitor profile (if any) should not be applied but the GPU curve should, currently to set a new curve the calibration tool should generate a new ICC profile with the wanted curve in the VCGT tag (I am not sure if this is the best option but would make the most universal one). In the end after profiling the last uploaded ICC could then be saved (although a compositor is not required to honor the request in that case it should send the not saved error). If the compositor doesn't save or the connection with this protocol is broken the compositor should restore previous settings. Hi, I only took a very quick glance, but I do like where this design is going. I'll refrain from commenting on wl_surface vs. not for now though. Forgive me my ignorance, but why is the "GPU curve" needed to be a custom curve provided by the client? I had pretty much the same question. The key point here is that an ICC profile only describes the color space of the output which means that white is whatever the code value 1,1,1 is and black is whatever the code value 0,0,0 is. If you want/need to have a specific white-point the profile on it's own is not enough, you have to calibrate the monitor (or anything else in the chain really, a plane's or connector's gamma/degamma/ctm, a shader) in a way that the code values correspond to the specific color. That's typically done automatically in the calibration/profiling software by modifying the vcgt and measuring the result. The resulting curve is saved in the ICC profile in the vcgt tag which is officially not part of the standard but you basically have to support it. The compositor is supposed to set the vcgt (in KMS terminology the gamma LUT of the connector) to the value in the ICC profile. I think the real question here is if we actually want to modify the vcgt or if we want to ensure it's set to identify and then let the client apply the LUT on its own to the color triplet. Generally I would prefer if the protocol doesn't leak any hardware specifics like the connector's gamma LUT so that the compositor can chose to map the color pipeline to all available hardware (shaders, connector degamma, CTM, gamma, plane degamma, CTM, gamma) as it sees fit. I'm not sure how much it will impact the precision though. My naive thinking would assume that you would like to be able to address the pixel values on the display wire as directly as possible, which means a minimum of 12 or 14 bits per channel framebuffer format and an identity "GPU curve". Is the reason to use the "GPU curve" that you assume there is a 8 bits per channel framebuffer and you need to use the hardware LUT to choose which 8 bits wide range of the possibly 14 bits channel you want to address? (Currently a client cannot know if the framebuffer is 8 bits or less or more.) Your protocol proposal uses the pixel encoding red/green/blue as uint (32-bit) per channel. Would it be possible to have the compositor do the LUT manipulation if it needs to avoid the intermediate rounding caused by a 8 bit per channel framebuffer or color pipeline up to the final LUT? If such "GPU curve" manipulation is necessary, it essentially means nothing else can be shown on the output. Oh, could another reason to have the client control the "GPU curve" be that then the client can still show information on that output, since it can adjust the pixel contents to remain legible even while applying the manipulation. Is that used or desirable? Btw. how would a compositor know the bit depth of a monitor and the transport (wire)? I presume there should be some KMS properties for that in addition to connector types. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC 0/1] Color manager calibration protocol v1
On 2019-04-15 23:38, Erwin Burema wrote: On Mon, 15 Apr 2019 at 20:30, Sebastian Wick wrote: On 2019-04-14 12:57, Erwin Burema wrote: > Without a way to calibrate/profile screens an color management > protocol looses a lot of its value. So to add this missing feature I > wrote the following protocol. > > The idea is that the calibration/profiling SW only sets the RGB > triplet and then the compositor is responsible to draw a rectanglular > region on the selected output screen, since not all calibration tools > will be at the center of the screen a user should be able to modify > the placement of this rectanglular region. Unless specified the > monitor profile (if any) should not be applied but the GPU curve > should, currently to set a new curve the calibration tool should > generate a new ICC profile with the wanted curve in the VCGT tag (I am > not sure if this is the best option but would make the most universal > one). In the end after profiling the last uploaded ICC could then be > saved (although a compositor is not required to honor the request in > that case it should send the not saved error). If the compositor > doesn't save or the connection with this protocol is broken the > compositor should restore previous settings. > > Currently no support to calibrate HDR screens since this will require > direct access (no tonemapping or color correction should happen in the > screen, with the potential exception if it is possible to upload > custom LUTs to the screen), this will require something like the > FREESYN2_GAMMA22 display mode as described here: > https://gpuopen.com/using-amd-freesync-2-hdr-color-spaces/ if this is > not available a HDR screen should be treated as if it is a rec2020+PQ > or HLG since it will do its own compositing+color managing (which > might or might not be correct) > > Erwin Burema (1): > Add color manager calibration protocol v1 > > .../cm_wayland_calibration.xml| 106 ++ > 1 file changed, 106 insertions(+) > create mode 100644 > unstable/color-manager-calibration/cm_wayland_calibration.xml The requirement to let the user position the patch makes me think that using a normal surface in a special mode makes more sense. Maybe something like this would work: I have tought about that as well but discarded that for several reasons 1) Will always be a single solid color, so no need for the full wl_buffer/wl_surface interface 2) Will need to be displayed above anything else, anywhere on the specified screen (for normal surface this might be a problem for tiling window managers) 3) Since after the colorimeter/spectrometer has been places and measuring is begun it shouldn't be touched anymore (so placing is a setup action, afterwards should be imovable) 4) Needs to be confined to a single output 5) Preferably need to be in display color depth (especially for calibration) My preffered way to place it would actually be similar to a screengrabber that allows to grab a section of the screen, where a user could just select a portion of the screen as "in this area the measuring device is/will be place", so deciding I wanted to keep things as simple as possible I decided against using wl_surface. (It will also get rid of the is_valid events since it should always display above anything else since it is not a normal surface) There are some good arguments in there I have not thought about before. I'm still concerned that it might create a lot of problems. What happens with pointer input over the patch? How do you get rid of the patch? Especially if you managed to occlude most of the screen. How would you interact and abort the measuring process? I'm open to that approach but I think those questions need answers. Using a wl_surface on the other hand side-steps all of those questions. My proposal certainly isn't complete and I forgot to specify that if the surface is occluded by something it should also be in the invalid state and movement of the surface might also trigger invalid state. The underlying surface has a state which indicates if it is shown on the output without further manipulation. In particular if the surface is in the state valid with mode standard the compositor guarantees that only the color correction pipeline and no "special" effects are applied to the surface. In the state valid with mode passthrough the color values of the surface are gauranteed to reach the output as is (e.g. no color correction and no vcgt LUT gets applied).
Re: [RFC 0/1] Color manager calibration protocol v1
On 2019-04-15 21:01, Simon Ser wrote: On Monday, April 15, 2019 9:30 PM, Sebastian Wick wrote: On 2019-04-14 12:57, Erwin Burema wrote: > Without a way to calibrate/profile screens an color management > protocol looses a lot of its value. So to add this missing feature I > wrote the following protocol. > > The idea is that the calibration/profiling SW only sets the RGB > triplet and then the compositor is responsible to draw a rectanglular > region on the selected output screen, since not all calibration tools > will be at the center of the screen a user should be able to modify > the placement of this rectanglular region. Unless specified the > monitor profile (if any) should not be applied but the GPU curve > should, currently to set a new curve the calibration tool should > generate a new ICC profile with the wanted curve in the VCGT tag (I am > not sure if this is the best option but would make the most universal > one). In the end after profiling the last uploaded ICC could then be > saved (although a compositor is not required to honor the request in > that case it should send the not saved error). If the compositor > doesn't save or the connection with this protocol is broken the > compositor should restore previous settings. > > Currently no support to calibrate HDR screens since this will require > direct access (no tonemapping or color correction should happen in the > screen, with the potential exception if it is possible to upload > custom LUTs to the screen), this will require something like the > FREESYN2_GAMMA22 display mode as described here: > https://gpuopen.com/using-amd-freesync-2-hdr-color-spaces/ if this is > not available a HDR screen should be treated as if it is a rec2020+PQ > or HLG since it will do its own compositing+color managing (which > might or might not be correct) > > Erwin Burema (1): > Add color manager calibration protocol v1 > > .../cm_wayland_calibration.xml| 106 ++ > 1 file changed, 106 insertions(+) > create mode 100644 > unstable/color-manager-calibration/cm_wayland_calibration.xml The requirement to let the user position the patch makes me think that using a normal surface in a special mode makes more sense. This makes sense to me as well. Is there a guarantee that the surface is always a single solid color? Maybe something like this would work: Typo: not a wl_output. Yes, thanks. The underlying surface has a state which indicates if it is shown on the output without further manipulation. In particular if the surface is in the state valid with mode standard the compositor guarantees that only the color correction pipeline and no "special" effects are applied to the surface. In the state valid with mode passthrough the color values of the surface are gauranteed to reach the output as is (e.g. no color correction and no vcgt LUT gets applied). If the state is invalid there is no guarantee how the color values are transformed before reaching the output. What would this be for? Apparently I really wasn't paying attention when writing this. This shouldn't be there. The wl_display.sync mechanism should be used to ensure that all measurements are done in the valid state. I'd imagine the client to do something like this: 1. create a zwp_color_measure_surface_v1 for the surface and the output to measure in the passthrough mode 2. tell the user to position the surface correctly and wait for the state to be valid (for example in gnome shell the state would be invalid when the surface is shown in expose or not shown on the output at all) 3. calibrate by filling the surface and applying the "vcgt" in software 4. profile 5. if the state changes to invalid in between go to 2 6. create the ICC profile and load it to the compositor via colord 7. destroy the interface, create a new one in the standard mode 8. wait for the state to be valid (as before) 9. measure the colors and check if the profile works as intended How would the client "measure"? With a colorimeter. Write some values that you expect to have a certain color, read the actual colors, calculate the error. I'm not sure if that answers your question. 10. if the state changes to invalid in between go to 8 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC 0/1] Color manager calibration protocol v1
On 2019-04-14 12:57, Erwin Burema wrote: Without a way to calibrate/profile screens an color management protocol looses a lot of its value. So to add this missing feature I wrote the following protocol. The idea is that the calibration/profiling SW only sets the RGB triplet and then the compositor is responsible to draw a rectanglular region on the selected output screen, since not all calibration tools will be at the center of the screen a user should be able to modify the placement of this rectanglular region. Unless specified the monitor profile (if any) should not be applied but the GPU curve should, currently to set a new curve the calibration tool should generate a new ICC profile with the wanted curve in the VCGT tag (I am not sure if this is the best option but would make the most universal one). In the end after profiling the last uploaded ICC could then be saved (although a compositor is not required to honor the request in that case it should send the not saved error). If the compositor doesn't save or the connection with this protocol is broken the compositor should restore previous settings. Currently no support to calibrate HDR screens since this will require direct access (no tonemapping or color correction should happen in the screen, with the potential exception if it is possible to upload custom LUTs to the screen), this will require something like the FREESYN2_GAMMA22 display mode as described here: https://gpuopen.com/using-amd-freesync-2-hdr-color-spaces/ if this is not available a HDR screen should be treated as if it is a rec2020+PQ or HLG since it will do its own compositing+color managing (which might or might not be correct) Erwin Burema (1): Add color manager calibration protocol v1 .../cm_wayland_calibration.xml| 106 ++ 1 file changed, 106 insertions(+) create mode 100644 unstable/color-manager-calibration/cm_wayland_calibration.xml The requirement to let the user position the patch makes me think that using a normal surface in a special mode makes more sense. Maybe something like this would work: interface="zwp_color_measure_surface_v1"/> interface="wl_output"/> The underlying surface has a state which indicates if it is shown on the output without further manipulation. In particular if the surface is in the state valid with mode standard the compositor guarantees that only the color correction pipeline and no "special" effects are applied to the surface. In the state valid with mode passthrough the color values of the surface are gauranteed to reach the output as is (e.g. no color correction and no vcgt LUT gets applied). If the state is invalid there is no guarantee how the color values are transformed before reaching the output. I'd imagine the client to do something like this: 1. create a zwp_color_measure_surface_v1 for the surface and the output to measure in the passthrough mode 2. tell the user to position the surface correctly and wait for the state to be valid (for example in gnome shell the state would be invalid when the surface is shown in expose or not shown on the output at all) 3. calibrate by filling the surface and applying the "vcgt" in software 4. profile 5. if the state changes to invalid in between go to 2 6. create the ICC profile and load it to the compositor via colord 7. destroy the interface, create a new one in the standard mode 8. wait for the state to be valid (as before) 9. measure the colors and check if the profile works as intended 10. if the state changes to invalid in between go to 8 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[RFC wayland-protocols v3 1/1] unstable: add color management protocol
This protocol allows clients to attach a color space, rendering intent and HDR information to surfaces and to query outputs about their color spaces and HDR capabilities. Signed-off-by: Sebastian Wick --- Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 228 ++ 3 files changed, 233 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..80abc1d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,7 @@ unstable_protocols = \ unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \ unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \ unstable/primary-selection/primary-selection-unstable-v1.xml \ + unstable/color-management/color-management-unstable-v1.xml \ $(NULL) stable_protocols = \ diff --git a/unstable/color-management/README b/unstable/color-management/README new file mode 100644 index 000..140f1e9 --- /dev/null +++ b/unstable/color-management/README @@ -0,0 +1,4 @@ +Color management protocol + +Maintainers: +Sebastian Wick diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml new file mode 100644 index 000..7b4d08e --- /dev/null +++ b/unstable/color-management/color-management-unstable-v1.xml @@ -0,0 +1,228 @@ + + + + + Copyright © 2019 Sebastian Wick + Copyright © 2019 Erwin Burema + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice (including the next + paragraph) shall be included in all copies or substantial portions of the + Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. + + + + This protocol specifies a way for a client to set the color space and + HDR metadata of a surface and to get information about the color spaces + and HDR capabilities of outputs. + + + + + A global interface used for getting color management surface and color + management output objects as well as creating color spaces from ICC + profiles. + + + + + These fatal protocol errors may be emitted in response to illegal color + management requests. + + + + + + + Create a color space object from an ICC profile. + + Only three channel display profiles are allowed. The file descriptor + must be mmap-able. If the conditions are not met a protocol error + "invalid_icc_profile" is raised by the compositor. + + See the zwp_color_space interface for more details about the created + object. + + See the ICC specification for more details about ICC profiles. + + + + + + + + This creates a new color zwp_color_management_output object for the + given wl_output. + + See the zwp_color_management_output interface for more details. + + + + + + + + This creates a new color zwp_color_management_surface object for the + given wl_surface. + + See the zwp_color_management_surface interface for more details. + + + + + + + + Destroy the zwp_color_manager object. + + + + + + + A zwp_color_management_output describes the color properties of an + output. + + + + + The color_space_changed event is sent after creating an zwp_color_management_output + (see zwp_color_manager.get_color_management_output) and whenever the color + space of the output changed. + + + + + + This creates a new zwp_colo
[RFC wayland-protocols v3 0/1] Color Management Protocol
New version with big changes. * zwp_color_manager is used only to create new objects * new interface zwp_color_management_output represents an output with color space and HDR capabilities * new interface zwp_color_management_surface represents a surface with color space and HDR properties * per-surface preferred color space/output * wl_surface.enter/leave can be used to get information about color spaces the surface is shown in (application can render to mastering display color space always when the surface is on that display) * require un-premultiplied data in color managed surfaces * clearify fd requirements (mmap-able 3-channel display ICC profile) * removed well-known color spaces * no more server created objects Thanks for all the feedback so far and as always, more feedback is very welcome. Sebastian Wick (1): unstable: add color management protocol Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 228 ++ 3 files changed, 233 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml -- 2.20.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[RFC wayland-protocols v2 1/1] unstable: add color management protocol
This protocol allows clients to attach a color space and rendering intent to a wl_surface. It further allows the client to be informed which color spaces a wl_surface was converted to on the last presented. Signed-off-by: Sebastian Wick --- Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 189 ++ 3 files changed, 194 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..80abc1d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,7 @@ unstable_protocols = \ unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \ unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \ unstable/primary-selection/primary-selection-unstable-v1.xml \ + unstable/color-management/color-management-unstable-v1.xml \ $(NULL) stable_protocols = \ diff --git a/unstable/color-management/README b/unstable/color-management/README new file mode 100644 index 000..140f1e9 --- /dev/null +++ b/unstable/color-management/README @@ -0,0 +1,4 @@ +Color management protocol + +Maintainers: +Sebastian Wick diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml new file mode 100644 index 000..47dd453 --- /dev/null +++ b/unstable/color-management/color-management-unstable-v1.xml @@ -0,0 +1,189 @@ + + + + +Copyright © 2019 Sebastian Wick +Copyright © 2019 Erwin Burema + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + + +This protocol provides the ability to specify the color space +of a wl_surface. If further enumerates the color spaces currently +in the system and allows to query feedback about which color spaces +a wl_surface was converted to on the last present. +The idea behind the feedback system is to allow the client to do color +conversion to a color space which will likely be used to show the surface +which allows the compositor to skip the color conversion step. + + + + + The color manager is a singleton global object that provides access + to outputs' color spaces, let's you create new color spaces and attach + a color space to surfaces. + + + + +render intents + + + + + + + + + +Well-known color spaces + + + + + + + + + + + + + + + + +These fatal protocol errors may be emitted in response to +illegal color management requests. + + + + + + +Set the color space of a surface. The color space is double buffered, +and will be applied at the time wl_surface.commit of the corresponding +wl_surface is called. + +The render intent gives the compositor a hint what to optimize for +in color space conversions. + +If a surface has no color space set, sRGB and an arbitrary render +intent will be assumed. + + + + + + + + +Request color space feedback for the current content submission +on the given surface. This creates a new color_space_feedback +object, which will deliver the feedback information once. If +multiple color_space_feedback objects are created for the same +submission, they will all deliver the same information. + +For details on what informat
[RFC wayland-protocols v2 0/1] Color Management Protocol
Sending in v2 with small fixes only. I'm using this in the hope to focus the previous discussion in the direction of the actual protocol and implementation. It looks like we have come to at least some consensus on a few points. If anyone disagrees here that would be great to know. 1. The color management protocol should contain support for HDR. I've previously argued against it but everyone else seems to agree and I don't have a strong opinion. 2. The whole pipeline should look something like [surface cs] -cs conversion-> [output cs] -tone mapping-> [output cs] -degamma-> [output linear cs] -blending-> [output linear cs] -gamma-> [output cs]. Where some parts can be skipped if e.g. surface cs == output cs or surface and output are SDR. 3. A CMS (like lcms2) can create a 3d lut which the pipeline can use to do the color space conversion from two 3 channel ICC profiles and a rendering intent. 4. A CMS can also create a 1d lut from the same data to linearize a surface from the output color space (degamma, gamma). 5. Device link profiles don't offer enough information to correctly do blending and convert to a random output color space. 6. The client should know which color spaces the surface is likely going to be converted to. 7. The CRTC gamma lut belongs to the compositor and no client should be able to directly change it. Which brings me to open issues: 1. Does it make sense to support device link profiles? I'm against it but would love to hear if anyone has objections given that they are supported in the other proposal. 2. How exactly should the client be informed of the "prefered" color spaces? IMO there are a few requirements: * the client should know when the prefered color space changes * the client should know when multiple color spaces are involved * the priority of those color spaces The single "prefered" color space that can be requested doesn't meet those requirements, neither does the wl_output.enter/leave (missing priority) nor does my protocol (moving the surface to another output will not trigger any new events). Another point here is that wl_output actually doesn't map to a single but multiple color spaces (e.g. cloned display). Better ideas are very welcome! 3. Do the standard color spaces actually improve anything? When the compositor doesn't have to make them available to clients and they can't rely on them being available, what's the point? Especially when displays don't really support standard color spaces most of the time (according to the EDID). 4. How do universal planes handle gamma when blending? This hopefully has a sinmple answer and there probably is some documention which I just can't find. There is some more open issues regarding HDR but I'll leave that to Ankit. Did I miss anything? Sebastian Wick (1): unstable: add color management protocol Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 189 ++ 3 files changed, 194 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml -- 2.20.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC wayland-protocols v2 1/1] Add the color-management protocol
On 2019-03-04 14:45, Pekka Paalanen wrote: Hi Sebastian and Graeme On Mon, 04 Mar 2019 13:37:06 +0100 Sebastian Wick wrote: On 2019-03-04 12:27, Pekka Paalanen wrote: > On Mon, 4 Mar 2019 19:04:11 +1100 > Graeme Gill wrote: > >> Pekka Paalanen wrote: >> >> > My failing is that I haven't read about what ICC v4 definition actually >> > describes, does it characterise content or a device, or is it more >> > about defining a transformation from something to something without >> > saying what something is. >> >> The ICC format encompasses several related forms. The one >> that is pertinent to this discussion is ICC device profiles. >> >> At a minimum an ICC device profile characterizes a devices color >> response by encoding a model of device values (i.e. RGB value >> combinations) >> to device independent color values (i.e. values related to device >> independent CIE XYZ, called Profile Connection Space values in ICC >> terms). A simple model such as color primary values, white point >> and per channel responses is easily invertible to allow transformation >> both directions. >> >> For less additive devices there are more general models (cLut - >> multi-dimensional color Lookup Table), and they are non-trivial >> to invert, so a profile contains both forward tables (device -> PCS >> AKA A2B tables) and reverse tables (PCS -> device AKA B2A tables). >> >> Then there is intents. The most basic is Absolute Colorimetric >> and Relative Colorimetric. The former relates the measured >> values, while the latter one assumes that the observer is adapted >> to the white point of the devices. Typically the difference is assumed >> to be a simple chromatic adaptation transform that can be encoded >> as the absolute white point or a 3x3 matrix. The default intent >> is Relative Colorimetric because this is the transform of least >> surprise. >> >> cLUT based profiles allow for two additional intents, >> Perceptual where out of gamut colors are mapped to be within >> gamut while retaining proportionality, and Saturation where >> colors are expanded if possible to maximize colorfulness. These >> two intents allow the profile creator considerable latitude in >> how they achieve these aims, and they can only be encoded using >> a cLUT model. > > Hi Graeme, > > thank you for taking the time to explain this, much appreciated. > > I'm still wondering, if an application uses an ICC profile for the > content it provides and defines an intent with it, should a compositor > apply that intent when converting from application color space to the > blending color space in the compositor? I think the correct approach would be to first convert from application color space to the output color space using the intent and then to blending color space. That way all colors in the blending color space will fit in the output color space. > Should the same application provided intent be used when converting the > composition result of the window to the output color space? When all blending color sources are in the output color space so is the resulting color. No intent required. Right, this is what I did not realize until Graeme explained it. Very good. >> > What is the use for a device link profile? >> >> Device link profile use was a suggestion to overcome the previously >> stated >> impossibility of a client knowing which output a surface was mapped >> to. >> Since this no longer appears to be impossible (due to >> wl_surface.enter/leave events >> being available), device link profiles should be dropped from the >> extension. >> It is sufficient that a client can do its own color transformation >> to the primary output if it chooses, while leaving the compositor to >> perform >> a fallback color transform for any portion that is mapped to a >> secondary output, >> or for any client that is color management unaware, or does not wish >> to >> implement its own color transforms. >> This greatly reduces the implementation burden on the compositor. > > Btw. wl_surface.enter/leave is not unambigous, because they may > indicate multiple outputs simultaneously. I did talk with you about > adding an event to define the one output the app should be optimizing > for, but so far neither protocol proposal has that. > > Niels, Sebastian, would you consider such event? My proposal has the zwp_color_space_feedback_v1 interface which is trying to solve this issue by listing the color spaces a surface was converted to in order of importance. Oh yes, indeed, sorry. We had a discussion going on about that. Eith
Re: [RFC wayland-protocols v2 1/1] Add the color-management protocol
On 2019-03-04 12:27, Pekka Paalanen wrote: On Mon, 4 Mar 2019 19:04:11 +1100 Graeme Gill wrote: Pekka Paalanen wrote: > My failing is that I haven't read about what ICC v4 definition actually > describes, does it characterise content or a device, or is it more > about defining a transformation from something to something without > saying what something is. The ICC format encompasses several related forms. The one that is pertinent to this discussion is ICC device profiles. At a minimum an ICC device profile characterizes a devices color response by encoding a model of device values (i.e. RGB value combinations) to device independent color values (i.e. values related to device independent CIE XYZ, called Profile Connection Space values in ICC terms). A simple model such as color primary values, white point and per channel responses is easily invertible to allow transformation both directions. For less additive devices there are more general models (cLut - multi-dimensional color Lookup Table), and they are non-trivial to invert, so a profile contains both forward tables (device -> PCS AKA A2B tables) and reverse tables (PCS -> device AKA B2A tables). Then there is intents. The most basic is Absolute Colorimetric and Relative Colorimetric. The former relates the measured values, while the latter one assumes that the observer is adapted to the white point of the devices. Typically the difference is assumed to be a simple chromatic adaptation transform that can be encoded as the absolute white point or a 3x3 matrix. The default intent is Relative Colorimetric because this is the transform of least surprise. cLUT based profiles allow for two additional intents, Perceptual where out of gamut colors are mapped to be within gamut while retaining proportionality, and Saturation where colors are expanded if possible to maximize colorfulness. These two intents allow the profile creator considerable latitude in how they achieve these aims, and they can only be encoded using a cLUT model. Hi Graeme, thank you for taking the time to explain this, much appreciated. I'm still wondering, if an application uses an ICC profile for the content it provides and defines an intent with it, should a compositor apply that intent when converting from application color space to the blending color space in the compositor? I think the correct approach would be to first convert from application color space to the output color space using the intent and then to blending color space. That way all colors in the blending color space will fit in the output color space. Should the same application provided intent be used when converting the composition result of the window to the output color space? When all blending color sources are in the output color space so is the resulting color. No intent required. What would be a reasonable way to do those conversions, using which intents? So in summary an ICC profile provides device characterization, as well as facilitating fast and efficient transformation between different devices, as well as a choice of intent handling that cannot typically be computed on the fly. Naturally to do a device to device space transform you need two device profiles, one for the source space and one for the destination. Do I understand correctly that an ICC profile can provide separate (A2B and B2A) cLUT for each intent? That's my understanding as well. > What is the use for a device link profile? Device link profile use was a suggestion to overcome the previously stated impossibility of a client knowing which output a surface was mapped to. Since this no longer appears to be impossible (due to wl_surface.enter/leave events being available), device link profiles should be dropped from the extension. It is sufficient that a client can do its own color transformation to the primary output if it chooses, while leaving the compositor to perform a fallback color transform for any portion that is mapped to a secondary output, or for any client that is color management unaware, or does not wish to implement its own color transforms. This greatly reduces the implementation burden on the compositor. Btw. wl_surface.enter/leave is not unambigous, because they may indicate multiple outputs simultaneously. I did talk with you about adding an event to define the one output the app should be optimizing for, but so far neither protocol proposal has that. Niels, Sebastian, would you consider such event? My proposal has the zwp_color_space_feedback_v1 interface which is trying to solve this issue by listing the color spaces a surface was converted to in order of importance. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org
Re: [RFC wayland-protocols 1/1] unstable: add color management protocol
On 2019-02-14 08:13, Nautiyal, Ankit K via wayland-devel wrote: Hi Sebastian, I am trying to extend Ole's color management protocol [1] to enable a client to pass HDR meta data. Added the modified protocol as weston protocol for the time being. [2] Thanks. That's useful. You have mention that the proposed protocol, ignores the HDR calibration/profiling, so do you suggest there should be another protocol for that altogether or is there a possibility to extend this for handling HDR metadata? I absolutely think that this can be extended for handling HDR. [1] https://lists.freedesktop.org/archives/wayland-devel/2017-January/032770.html [2] https://gitlab.freedesktop.org/aknautiyal/weston/commit/9d08cc22d6ba2b0c48ac255abc86ba5e217a507c Also, there are a few queries to understand the flow, please find below inline: On 2/14/2019 8:16 AM, Sebastian Wick wrote: This protocol allows clients to attach a color space and rendering intent to a wl_surface. It further allows the client to be informed which color spaces a wl_surface was converted to on the last presented. Signed-off-by: Sebastian Wick --- Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 183 ++ 3 files changed, 188 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..80abc1d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,7 @@ unstable_protocols = \ unstable/xdg-decoration/xdg-decoration-unstable-v1.xml\ unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \ unstable/primary-selection/primary-selection-unstable-v1.xml \ +unstable/color-management/color-management-unstable-v1.xml \ $(NULL) stable_protocols =\ diff --git a/unstable/color-management/README b/unstable/color-management/README new file mode 100644 index 000..140f1e9 --- /dev/null +++ b/unstable/color-management/README @@ -0,0 +1,4 @@ +Color management protocol + +Maintainers: +Sebastian Wick diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml new file mode 100644 index 000..1615fe6 --- /dev/null +++ b/unstable/color-management/color-management-unstable-v1.xml @@ -0,0 +1,183 @@ + + + + +Copyright © 2019 Sebastian Wick +Copyright © 2019 Erwin Burema + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + + +This protocol provides the ability to specify the color space +of a wl_surface. If further enumerates the color spaces currently +in the system and allows to query feedback about which color spaces +a wl_surface was converted to on the last present. +The idea behind the feedback system is to allow the client to do color +conversion to a color space which will likely be used to show the surface +which allows the compositor to skip the color conversion step. + + + + + The color manager is a singleton global object that provides access + to outputs' color spaces, let's you create new color spaces and attach + a color space to surfaces. + + + + +render intents + + + + + + + + + +Well-known color spaces + + + + + + + + + + + + +These fatal protocol errors may be emitted in response to +illegal color management requests. + + + + + + +Set the color space of a surface. The color space is double buffered, +and will be applied at the time wl_surface.commit of the corresponding +wl_surface is called. +
Re: [RFC wayland-protocols 1/1] unstable: add color management protocol
On 2019-02-14 05:55, Sharma, Shashank via wayland-devel wrote: Hello Sebastian, My comments inline. Regards Shashank -Original Message- From: wayland-devel [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of Sebastian Wick Sent: Thursday, February 14, 2019 8:16 AM To: wayland-devel@lists.freedesktop.org Cc: Sebastian Wick Subject: [RFC wayland-protocols 1/1] unstable: add color management protocol This protocol allows clients to attach a color space and rendering intent to a wl_surface. It further allows the client to be informed which color spaces a wl_surface was converted to on the last presented. Signed-off-by: Sebastian Wick --- Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 183 ++ 3 files changed, 188 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..80abc1d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,7 @@ unstable_protocols = \ unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \ unstable/linux-explicit-synchronization/linux-explicit-synchronization- unstable-v1.xml \ unstable/primary-selection/primary-selection-unstable-v1.xml \ + unstable/color-management/color-management-unstable-v1.xml \ $(NULL) stable_protocols = \ diff --git a/unstable/color-management/README b/unstable/color- management/README new file mode 100644 index 000..140f1e9 --- /dev/null +++ b/unstable/color-management/README @@ -0,0 +1,4 @@ +Color management protocol + +Maintainers: +Sebastian Wick diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml new file mode 100644 index 000..1615fe6 --- /dev/null +++ b/unstable/color-management/color-management-unstable-v1.xml @@ -0,0 +1,183 @@ + + + + +Copyright © 2019 Sebastian Wick +Copyright © 2019 Erwin Burema + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + + +This protocol provides the ability to specify the color space +of a wl_surface. If further enumerates the color spaces currently +in the system and allows to query feedback about which color spaces +a wl_surface was converted to on the last present. +The idea behind the feedback system is to allow the client to do color +conversion to a color space which will likely be used to show the surface +which allows the compositor to skip the color conversion step. + + + + + The color manager is a singleton global object that provides access + to outputs' color spaces, let's you create new color spaces and attach + a color space to surfaces. + + + + +render intents + + + + + Would you elaborate a bit more on what do we want to achieve with render-intent ? How can the compositor utilize this filed during color conversion ? It's about how to handle colors which are outside of the destionation gamut https://en.wikipedia.org/wiki/Color_management#Rendering_intent + + + + Its slightly difficult to define what is the definition of a "well-known" color space, as different use cases may have their own favorite. May be we can call it "general-purpose" or "standard" or something in those line. +Well-known color spaces + + + + + I guess we mean DCI-P3 here + + /> + (HDR)" /> Well, this is slightly off the chart for me. If I understood this c
Re: [RFC wayland-protocols 1/1] unstable: add color management protocol
Thanks. I don't really know about all the standards so I'll just trust you on this. On 2019-02-14 05:47, Chris Murphy wrote: On Wed, Feb 13, 2019 at 7:55 PM Sebastian Wick wrote: + I suggest ICC spec language. 'ICC-absolute colorimetric' + 'media-relative colorimetric' + I think this is 'deviceRGB' although it could be 'unknown'. Practically, it could only be unknown if the values aren't being displayed or output anywhere. The instant RGB values are displayed anywhere, the color space is knowable, therefore it's at least deviceRGB. But unknown is also consistent with EXIF. And 'color space' can be assumed. + Summary probably should be 'sRGB IEC61966-2.1' and again 'color space' can be assumed. + Summary probably should be 'Adobe RGB (1998)' + DCI-P3 But there are three, so it needs to be specific which one or define all three. And there's a variant, which is 'Display P3' from Apple, which starts out being defined with DCI-P3 D65 but uses a tone response curve defined by the sRGB curve. + Probably the summary should be 'ITU-R BT.2020 for UHDTV' same for below. + + It's reasonable to add 'ITU-R BT.709 for HDTV' or Rec. 709 since that's used today still. -- Chris Murphy ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[RFC wayland-protocols 0/1] Color Management Protocol
This is yet another color management protocol proposal. It ignores HDR and calibration/profiling on purpose. The general idea here is twofold: 1. clients can create color spaces from ICC profiles and attach them with a render intent to a wl_surface 2. clients can get feedback about which color spaces a given wl_surface was converted to with which priority (implicitly via the order of events) The compositor should be doing its best to convert all surfaces to the correct color space for the next present. Color space conversions don't have a single correct algorithm but are subjective which means for accurate colors clients should be able to do all color conversion. This is not always possible (surface spanning multiple outputs, hotplugging outputs, etc) but with the color space conversion feedback the client can get as close as possible. For example if the surface is shown on a single output the client can assume that the next frame will be shown on the same output and use the correct color space for that output. It it spans multiple outputs the client can choose one color space and let the compositor do the best to convert to the other. In many cases, with a well-behaving client, the compositor can skip color conversion entirely. Feedback and comments appreciated. The protocol summary and description texts could use more work (help here would also be appreciated). Sebastian Wick (1): unstable: add color management protocol Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 183 ++ 3 files changed, 188 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml -- 2.20.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[RFC wayland-protocols 1/1] unstable: add color management protocol
This protocol allows clients to attach a color space and rendering intent to a wl_surface. It further allows the client to be informed which color spaces a wl_surface was converted to on the last presented. Signed-off-by: Sebastian Wick --- Makefile.am | 1 + unstable/color-management/README | 4 + .../color-management-unstable-v1.xml | 183 ++ 3 files changed, 188 insertions(+) create mode 100644 unstable/color-management/README create mode 100644 unstable/color-management/color-management-unstable-v1.xml diff --git a/Makefile.am b/Makefile.am index 345ae6a..80abc1d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,7 @@ unstable_protocols = \ unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \ unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \ unstable/primary-selection/primary-selection-unstable-v1.xml \ + unstable/color-management/color-management-unstable-v1.xml \ $(NULL) stable_protocols = \ diff --git a/unstable/color-management/README b/unstable/color-management/README new file mode 100644 index 000..140f1e9 --- /dev/null +++ b/unstable/color-management/README @@ -0,0 +1,4 @@ +Color management protocol + +Maintainers: +Sebastian Wick diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml new file mode 100644 index 000..1615fe6 --- /dev/null +++ b/unstable/color-management/color-management-unstable-v1.xml @@ -0,0 +1,183 @@ + + + + +Copyright © 2019 Sebastian Wick +Copyright © 2019 Erwin Burema + +Permission is hereby granted, free of charge, to any person obtaining a +copy of this software and associated documentation files (the "Software"), +to deal in the Software without restriction, including without limitation +the rights to use, copy, modify, merge, publish, distribute, sublicense, +and/or sell copies of the Software, and to permit persons to whom the +Software is furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. + + + +This protocol provides the ability to specify the color space +of a wl_surface. If further enumerates the color spaces currently +in the system and allows to query feedback about which color spaces +a wl_surface was converted to on the last present. +The idea behind the feedback system is to allow the client to do color +conversion to a color space which will likely be used to show the surface +which allows the compositor to skip the color conversion step. + + + + + The color manager is a singleton global object that provides access + to outputs' color spaces, let's you create new color spaces and attach + a color space to surfaces. + + + + +render intents + + + + + + + + + +Well-known color spaces + + + + + + + + + + + + +These fatal protocol errors may be emitted in response to +illegal color management requests. + + + + + + +Set the color space of a surface. The color space is double buffered, +and will be applied at the time wl_surface.commit of the corresponding +wl_surface is called. + +The render intent gives the compositor a hint what to optimize for +in color space conversions. + +If a surface has no color space set, sRGB and an arbitrary render +intent will be assumed. + + + + + + + + +Request color space feedback for the current content submission +on the given surface. This creates a new color_space_feedback +object, which will deliver the feedback information once. If +multiple color_space_feedback objects are created for the same +submission, they will all deliver the same information. + +For details on what information is returned, see the +color_spac
Re: Summary of the security discussions around Wayland and privileged clients
Hey Jasper, maybe I didn't understand what you're saying but why can't you use the application authorization mechanism you're talking about in a WSM? Wouldn't it make sense to make it independent of the compositor? Am 2014-02-26 23:05, schrieb Jasper St. Pierre: Hi Martin, My experience with PAM and similar pluggable security modules is that they provide a subpar user experience, are hard to integrate properly into the system, and have large pain points that stem from having such flexibility. My compositor, mutter, will probably never call out to your WSM, and we'll probably defer to another application authorization mechanism, probably the same one that provides application sandboxing, and other such capabilities. I'd also recommend that you go ahead and talk to the people, and perhaps even help build that mechanism, which isn't specific to Wayland, but will also cover DBus requests, system calls, and more. On Wed, Feb 26, 2014 at 4:40 PM, Martin Peres martin.pe...@free.fr wrote: Le 19/02/2014 17:11, Martin Peres a écrit : Wayland Security Modules As seen earlier, granting access to a restricted interface or not depends on the context of the client (how it was launched, previous actions). The expected behaviour should be defined by a security policy. As no consensus on the policy [can apparently be reached](https://www.mail-archive.com/Wayland-devel@lists.freedesktop.org/msg12261.html [1]) (as usual in security), we have all agreed that we needed to separate the policy from the code. This is very much alike [Linux Security Modules (LSM)](http://www.nsa.gov/research/_files/selinux/papers/module/x45.shtml [2]) or [X Access Control Extension (XACE)](http://www.x.org/releases/X11R7.5/doc/security/XACE-Spec.html [3]). From a software engineering point of view, we would work on a security library called Wayland Security Modules (name subject to changes) that Wayland compositors would call when a security decision would need to be made. The library would then load the wanted security policy, defined by a shared-object that I will refer to as the security backend. In the case of allowing a client to bind a restricted interface or not, the corresponding WSM hook should return ``ACCEPT``, ``PROMPT`` or ``DENY``, prompt meaning the compositor would have to ask the user if he wants to accept the risk or not. Let me stress out that prompting should be a last-resort measure as numerous studies have been made proving that unless asked very rarely, users will always allow the operation. Some additional hooks would also be needed in order to track the state of Wayland clients (open, close, etc...) but nothing too major should be needed. The compositors would just have to store this context in a ``void *security;`` attribute in the Wayland client structure. Finally, WSM could be extended to control the access to the clipboard and maybe other interfaces I haven't thought about yet. The design of this library has not started yet. If you are interested in helping out, I would love to have some feedback on what are your use cases for WSM. Hey Guys, I think I'll start working on this lib pretty soon. If you have any objection towards going down this path, please voice them now. Also, do you think we should allow stacking security modules or not? For simplicity reasons, I don't think I'll allow it, but some one of you may have compelling reasons to allow it. Cheers, Martin ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel [4] -- Jasper Links: -- [1] https://www.mail-archive.com/Wayland-devel@lists.freedesktop.org/msg12261.html [2] http://www.nsa.gov/research/_files/selinux/papers/module/x45.shtml [3] http://www.x.org/releases/X11R7.5/doc/security/XACE-Spec.html [4] http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Summary of the security discussions around Wayland and privileged clients
Am 2014-02-20 20:02, schrieb Martin Peres: Le 20/02/2014 13:04, Pekka Paalanen a écrit : snip It can be done, but with a little more effort than implied here. Binding to an interace means wl_registry.bind request, and failing that is always a fatal error, which terminates the client connection. All errors in Wayland are fatal like that. Instead, the interface should be always bindable, but include explicit protocol to indicate failure in using its requests. In this case, we can make something pretty simple, send a signal to the application if rights to use this interface has been granted. If the application tries to use the interface without having the rights to do so, an EPERM signal can be sent (not to be confused with the revokation signal that happens when rights have been revoked). What do you think? I would like to have a request_permission and a revoke_permission method and respectively a permission_granted and a permission_revoked event. An application might not need to permission or only needs it for a small amount of time. Slightly unrelated: We also need a way for a program to ask the compositor to start a client (and we have to ask ourselves how to handle arguments, environment variables etc.). ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Authorized clients
Am 2014-01-08 13:02, schrieb Martin Peres: On 07/01/2014 20:26, Jasper St. Pierre wrote: Would it be ok for you if the compositor asked the user to agree for the program to do the operation? If so, we can guarantee that this is really the user's intent and allow the application. We can also add a security warning with a Do not ask again checkbox. Would it be satisfactory to you? The user opened up a screen recording app. The user's intent is very much to record the screen. We don't need to ask the user again with a prompt. How do you make sure it WAS launched by the user and not run silently by one application? That's the whole problem. If the application starts recording the screen without user interaction I would consider it broken. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Authorized clients
Am 2014-01-07 15:07, schrieb Martin Peres: Those are extremely rare cases. Users wanting to do that should agree they give up confidentiality and should thus be administrators in order to tell the compositor that. Why should those people have worse security then others only because they want a feature you define as non-standard? In this case, we can still restrict access to the interface to a handful of programs to lower the risks, but it will still be possible for these applications to spy on the user without him knowing and this is something that shouldn't be allowed by default. Like I said, we should be able to let polkit decide. You could even distribute a .rules file which white-lists an application if we pass the executable path. You may be right. I meant for screen grabbing (images or videos), no idea what restricted interface could be useful for a wayland compositor. Any idea? The GNOME accessibility team need a few restricted protocols. Don't know about the details, though. Would it be ok for you if the compositor asked the user to agree for the program to do the operation? If so, we can guarantee that this is really the user's intent and allow the application. We can also add a security warning with a Do not ask again checkbox. Would it be satisfactory to you? If an application has the permission to use an restricted protocol it already met all the requirements. You should talk to the polkit dev if you want such an feature, I guess. I don't really like mandating compositors to implement that much code, but that's the only secure way I see to allow the uses cases you want to allow. And that's exactly why I don't want to implement the authorization checking in the compositor! We can safely let polkit decide in non-obvious cases. Less code in the compositor, less duplicated code and less security risks because polkit is designed to do that. By the way, I asked Peter about the security of input and that should be good. We then discussed about visual feedback as a mean to provide some mitigation and show some applications are grabbing the screen in the background. That may be something you would be interested in, in your case. What do you think? I'm personally not interested in it but I guess it's a nice feature for some people and I don't see why it should not work. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Authorized clients
Am 2014-01-08 19:53, schrieb Martin Peres: Le 08/01/2014 17:20, Sebastian Wick a écrit : Am 2014-01-07 15:07, schrieb Martin Peres: Those are extremely rare cases. Users wanting to do that should agree they give up confidentiality and should thus be administrators in order to tell the compositor that. Why should those people have worse security then others only because they want a feature you define as non-standard? That's not what I meant. I meant that if they want what you say you want to allow, they will de-facto loose some security (namely, confidentiality if the screenshot app is misbehaving). I don't understand you. You trust the compositor to do the right things, why shouldn't you trust a screenshot app to do the right things? I mean, if you make a screenshot via a key-binding you also trust weston-screenshooter to not screw up. You have to trust the application if you want to give it access to a privileged protocol. In this case, we can still restrict access to the interface to a handful of programs to lower the risks, but it will still be possible for these applications to spy on the user without him knowing and this is something that shouldn't be allowed by default. Like I said, we should be able to let polkit decide. You could even distribute a .rules file which white-lists an application if we pass the executable path. It is indeed something that is good, but insufficient because the trusted programs can do whatever they want and the compositor won't be able to tell if what they request really is what the user wants. Of course they can. Every client can do whatever they want. Even when taking a screenshot in weston via a key-binding you launch weston-screnshooter and it can do whatever it wants to do. The only way to be sure that it does exactly what you want it to do is by reading the source code. Simple example, you want a CLI application to be able to make screenshots and save them to a file. As an attacker, if I want a screenshot, I'll just run the program and get the screenshot. Is that something you think is OK, I definitely think this is not. It's not okay. You should not give the application permission to use the screenshoter protocol without further restrictions, then. If we would use polkit you could configure it so that the application gets access to the restricted protocol if it is started with root permission or after you entered your password. Oh, and if you make a screenshot, no matter how, and save it to disk, it is readable by any other application you start. It's not our job to prevent that. This is why I said the compositor shouldn't agree on a screenshot request if it can't tell if it was a user-made request or an app-made one. That's wrong. It should not matter who made the request. If you can use a program to exploit the security, the program is broken or has the wrong security settings. If you have a screenshot UI and it gets started (by whoever) the user must do something for the application to actually take the screenshot, otherwise it's broken. If you have a command line app to make screenshots and someone or something wants to start it there must be some kind of user interaction to confirm it. If you press a key-biding and the compositor starts an application to make a screenshot there doesn't need to be a confirmation. We simply start the app and be done with it. So we can have a configuration which only allows access to restricted protocols if the user agrees to it (or something else, this is really up to the one who configures the system). If the request to start the application comes from the compositor itself we can safely assume that everything is fine and start to app. If the request to start the application comes from another client, we check the configuration. If the configuration says it's fine to start the app, we do so (however it will determine if it's okay or not; maybe the app is configured to just start, maybe the app is configured to require the user to press on a yes button, maybe the app is configured to require the user to enter his password or maybe it is simply configured to deny the request). The only solutions we found so far have been: - listen for hot keys from an input device (we have to trust the kernel for not allowing to forge events) - require a confirmation of some sort (popup / systray icon / whatever) Like I said, we can configure certain application to not require a confirmation dialog. The decision making is just what it is, a decision. This is an implementation detail. You propose to use polkit for the decision making, why not? But I think this is overkill since we only need a file in /etc/wayland/restricted_apps.d/ containing the full path of the authorized app and the interfaces that are allowed. Some apps might need some confirmation (however it will look) like command line screenshooters or applications which are not installed and thus
Re: Authorized clients
Am 2014-01-06 16:05, schrieb Martin Peres: As I said before, I think trusting applications is taking the problem the wrong way. What we want is that a screenshot can only happen when the *user* wants it. This is why I think it is the desktop shell's job to launch the screenshot app when required by the user. In this case, even if the user can select the application that can perform a screen shot and even if a malicious application changes the default setting, it won't be able to perform screenshots silently. This is what we want. It's just not flexible enough. What if you want to autostart a screen-reader? By allowing some programs (and not the action), you expose yourself to letting the application record the screen without the user knowing and this is not acceptable. You would only allow certain functionality to trusted programs. If the program does something unexpected or malicious you should not trust the program. You would also know which program uses which functionality so you could inform the user about it. My proposal is that it should require a physical key pressing to be able to get ONE screenshot means we also require a secure input method and can attest the origin of the key stroke but what else can be do? Of course, you should also allow key strokes from the on-screen keyboard, but this application should be launched by the compositor and should be trusted anyway. I should talk to Peter Hutterer about input security/origin. Like I said, I think it's too inflexible. Martin, I would rather not use cgroups for that because it isn't stable yet and I'm sure we can find a solution that doesn't depend on it. I do agree on this one but I'm not even sure how cgroups would be useful. The problem we have is relatively simple. We want a protocol to be restricted to certain clients. Which means: 1. The program of the client must not be manipulated by ptrace, LD_PRELOAD etc. 2. The communication must be separate from other clients to prevent snooping 3. The compositor must decide if a client may use the protocol or not The only way I know to achieve 1. is by starting the program by another trusted application. The compositor is one of them. Making the communication separate works by passing a new socket to the program and the only way I know of is by passing it to the program when starting it. To decide if a client may use a protocol we could outsource the whole decision to polkit and just pass enough information so it can make a good decision. All in all I think the best and most simple way to do it is by starting the program by the compositor, passing a new socket, then let the client ask for permission to use a protocol and pass the request to polkit. The permission would be per-connection so if a program started by the compositor starts another program with a client it would have the same permission as the parent. That also means that we need a protocol to tell the compositor to start a program with a new socket and a protocol to ask the compositor for permission. Polkit should be flexible enough to allow a protocol based on how a program was started. You could configure polkit that if the compositor started a program because of a key-binding it has permission to use a protocol and stuff like that. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Authorized clients
Am 2014-01-06 19:33, schrieb Martin Peres: Le 06/01/2014 19:10, Sebastian Wick a écrit : Am 2014-01-06 16:05, schrieb Martin Peres: As I said before, I think trusting applications is taking the problem the wrong way. What we want is that a screenshot can only happen when the *user* wants it. This is why I think it is the desktop shell's job to launch the screenshot app when required by the user. In this case, even if the user can select the application that can perform a screen shot and even if a malicious application changes the default setting, it won't be able to perform screenshots silently. This is what we want. It's just not flexible enough. What if you want to autostart a screen-reader? Please provide a better example, this one doesn't make sense. Who would want that? And if they do want that, they should press a button on their keyboard to do just that. Taking a screenshot from the command line with delay, recording the desktop as soon as a program starts. Making screenshots at a specific times. There are some valid cases you can't even think of until someone wants it. Let's not repeat X11's mistakes. Being too lax on security isn't good for anyone. By allowing some programs (and not the action), you expose yourself to letting the application record the screen without the user knowing and this is not acceptable. You would only allow certain functionality to trusted programs. If the program does something unexpected or malicious you should not trust the program. You would also know which program uses which functionality so you could inform the user about it. See, I hate the term trusted. Just because a program is trusted doesn't mean it is cannot do anything malicious at some point. Even malicious doesn't mean much since, to me, malicious means doing something without the user expecting it. My proposal is that it should require a physical key pressing to be able to get ONE screenshot means we also require a secure input method and can attest the origin of the key stroke but what else can be do? Of course, you should also allow key strokes from the on-screen keyboard, but this application should be launched by the compositor and should be trusted anyway. I should talk to Peter Hutterer about input security/origin. Like I said, I think it's too inflexible. Please, share a list of valid use cases :) I already gave reasons why I think not doing what I proposed almost means no confidentiality on the application's buffers. Of course, it could be mitigated by making the screen flash or something when a screenshot is taken but people just hate that and some would even fail to notice it. What you want is allowing apps to grab the screen whenever you want. Allowing that should mean you have root/admin access. A global configuration file could disable the screenshooting security, but that should be a conscious choice of the administrator (for whatever weird reason they want to be able to grab the screen). However, I re-iterate, this is something stupid security-wise for very little benefit. Why don't you want to associate a physical event to start the recording process? I just don't agree that a restricted protocol is only useful if the user interacts with it. Martin, I would rather not use cgroups for that because it isn't stable yet and I'm sure we can find a solution that doesn't depend on it. I do agree on this one but I'm not even sure how cgroups would be useful. The problem we have is relatively simple. We want a protocol to be restricted to certain clients. Which means: 1. The program of the client must not be manipulated by ptrace, LD_PRELOAD etc. 2. The communication must be separate from other clients to prevent snooping 3. The compositor must decide if a client may use the protocol or not The only way I know to achieve 1. is by starting the program by another trusted application. The compositor is one of them. Making the communication separate works by passing a new socket to the program and the only way I know of is by passing it to the program when starting it. To decide if a client may use a protocol we could outsource the whole decision to polkit and just pass enough information so it can make a good decision. All in all I think the best and most simple way to do it is by starting the program by the compositor, passing a new socket, then let the client ask for permission to use a protocol and pass the request to polkit. The permission would be per-connection so if a program started by the compositor starts another program with a client it would have the same permission as the parent. Fully agree on this. That also means that we need a protocol to tell the compositor to start a program with a new socket and a protocol to ask the compositor for permission. Why do you want to create so many protocols? The compositor simply has to create a new connection to itself, mark this connection as being
Re: Authorized clients
Am 2014-01-02 23:01, schrieb Maarten Baert: On 31/12/13 05:02, Sebastian Wick wrote: I haven't looked at your code yet, but I suspect this detection mechanism would be seriously flawed, because it doesn't consider the environment of the application (chroot, LD_PRELOAD, LD_LIBRARY_PATH, the Qt and GTK plugin mechanisms are also triggered by environment variables and allow loading arbitrary code). My proof-of-concept Wayland keylogger [1] demonstrates that: it's not limited to logging keys, it can do anything at all, including accessing sensitive Wayland APIs if the original application is allowed to do that. Maybe I should have made it more clear. The client must be started by the compositor and it needs permission from either a config file or polkit. The patches introduce a new protocol which lets a client tell the compositor to start a new program/client. By the way, the executable path alone is not enough, because applications launched with different command-line arguments can behave very differently, even if the environment is completely clean. This can be solved by launching a simple bash script rather than the actual application, and letting that script decide what arguments are allowed (the simplest case would be a bash script that ignores all arguments and just launches the application). That's something application developers should be made aware of. That's not a problem as long as you don't allow bash to use a restricted protocol. If a program can do something you don't want it to do you shouldn't give it permission anyhow. Correct me if I'm wrong, but my understanding was that polkit authorizes _actions_, not clients. It doesn't seem to care what application requested the action, it just asks the user 'do you want to execute this action?' and the user decides. In this case, the action is launching an application with access to sensitive Wayland APIs, regardless of what application requested that action. Again, to me this seems like the only safe way to do this - just because the client is a known program doesn't mean that it can be trusted. Authorization for an action is always bound to a subject; in this case to a process. I don't really understand why you want to use custom configuration files _and_ polkit, which has pretty much the exact same purpose. Why? They have a completely different purpose. The config files allow specific protocols for a specific executable. If a client wants to use more protocols than allowed in the config files or there is no config file at all, the client could not use any of the restricted protocols. In that case the compositor asks polkit if the process is allowed to use the protocol or not. Polkit is a system-wide configuration which means that you can make a restricted interface available for every client in case that you don't care about the security feature or make it ask for the user or admin password. If Wayland is going to use polkit for the authentication API, why are the Wayland-specific authentication rules even needed? Polkit already has an advanced rule system, it would be easy enough to just add the executable path and the list of allowed protocols to the polkit actions file, right? This seems to be what 'pkexec' does (the polkit equivalent of sudo/gksudo/kdesu). Two reasons: 1. applications should not provide polkit rules 2. it still works without polkit, it's optional and just makes some stuff easier Adding two parallel authentication mechanisms with the same purpose doubles the attack surface, and I don't see the benefit. I'm not saying that we should use polkit, but I think there should be only one mechanism, not two. Like I said, polkit makes stuff easier for the user - nothing else. Is it acceptable for the Wayland protocol to have polkit as a dependency for the authentication API? It's a patch for weston, the compositor and it's an optional dependency. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Authorized clients
I'm currently working on a system which allows specific clients to use restricted interfaces [1]. This is needed for applications like screenhooters, desktop recorders outside of the compositor, accessibility tools and others. The current implementation consists of a protocol which can be used to start an application via the compositor to ensure a chain of trust and a mechanism for the compositor to determine if a client is authorized to use the protocol. A client is authorized for a protocol if... a) the client's executable path is found in a config file in the directory /etc/xdg/wayland/auth.d and if the config allows access on the protocol b) polkit authorizes the client The config files in /etc/xdg/wayland/auth.d have the weston ini format and can contain an arbitrary number of sections. A section must contain an executable config which is the path to the executable and an allow config which is a list of allowed protocols separated by a white-space. If the config doesn't allow the client to use the protocol, the compositor queries polkit for authorization. The benefit of having polkit has a fallback is that you can even use authorize clients which don't provide a config file and can be configured easily. The problem is that checking for authorization is now asynchronous which means that the current approach, to immediately post an error and delete the resource [2], doesn't work anymore and I don't know how to fix it. I would appreciate if you can help me with the problem and I'd also appreciate comments regarding the design of the system and other criticism. [1] https://github.com/swick/weston/compare/authorizedclient [2] https://github.com/swick/weston/blob/master/src/screenshooter.c#L231-L235 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] introduces a setting to give permission to any client to do screenshots
Am 2013-12-16 00:52, schrieb Timothée Ravier: polkit could also be used for the whole process of deciding whether a screenshot action is allowed or not: * The compositor would first request authorization for the screenshot action to polkit; * polkit would check if the action is legitimate using the authorization rules defined on the system; * If no rules matches, a dialog requesting confirmation by the user is displayed; * If the polkit answer is positive, the compositor would launch the screenshot application. Adding rules is a little bit more difficult in this case, but this has the advantage of reusing an already developed architecture. Cheers Sounds like a good plan but I still don't understand why the application must be started by the compositor. What exactly is the benefit? ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] introduces a setting to give permission to any client to do screenshots
Am 2013-12-13 16:12, schrieb Martin Peres: What prevents other applications from modifying this setting to true if they want to spy on applications? Nothing. But then again if you can write to the ini file you can make the compositor load any code with the shell setting. I don't even think my patch is the right way to handle it anymore. There must be a way to trust a client even when it's not started by the compositor. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] introduces a setting to give permission to any client to do screenshots
Am 2013-12-10 00:20, schrieb Bryce W. Harrington:
On Wed, Dec 04, 2013 at 05:38:23PM +0100, Sebastian Wick wrote:
This patch adds a screenshooter section with the restrict-access
setting which is on by default and is the current behavior of weston.
When turning it off, all clients can use the screenshooter protocol.
This makes screen capturing for clients easier because they don't
have to be started by weston.
---
man/weston.ini.man | 6 ++
src/screenshooter.c | 8 +++-
weston.ini.in | 3 +++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index 6be90bf..b94ac3d 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
@@ -408,6 +408,12 @@ The terminal shell (string). Sets the $TERM
variable.
sets the path to the xserver to run (string).
.RE
.RE
+.SH SCREENSHOOTER SECTION
+.TP 7
+.BI restrict-access= true
+only allow authenticated clients to take screenshots (boolean).
+.RE
+.RE
.SH SEE ALSO
.BR weston (1),
.BR weston-launch (1),
diff --git a/src/screenshooter.c b/src/screenshooter.c
index 0c657bc..65b6c09 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -224,11 +224,17 @@ bind_shooter(struct wl_client *client,
{
struct screenshooter *shooter = data;
struct wl_resource *resource;
+ struct weston_config_section *section;
+ int restrict_access;
resource = wl_resource_create(client,
screenshooter_interface, 1, id);
- if (client != shooter-client) {
+ section = weston_config_get_section(shooter-ec-config,
screenshooter, NULL, NULL);
+ weston_config_section_get_bool(section,
+ restrict-access, restrict_access, 1);
Could also check the return value of weston_config_section_get_bool;
it'll set errno and return -1 if the config value was typo'd or
omitted.
It will save the default value, true in this case, if it's missing so it
should be fine.
But does this have security implications? I assume it is restricted by
default in order to prevent clients from snooping. Could you add a bit
more detail about the specific problem(s) being solved with this?
Maybe
there's a way to solve the problem without fully dropping the
restriction?
I wrote a GStreamer wayland source element which needs to receive the
data
somehow and it uses the screenshooter protocol to do so.
The screenshooter protocol however is restricted to clients which got
started by weston itself (only weston-screenshooter so far) to make sure
the client has not been manipulated. You would have to start every
application which might use the GStreamer wayland source element by
weston.
It would make it drastically harder to use it and on a average linux PC
the current mechanism doesn't give you more security so it's pretty save
to turn the restriction off.
If you have a system where every client is sandboxed and you don't want
any
client to see what the others have rendered, you should not turn the
restriction off.
+
+ if (restrict_access client != shooter-client) {
wl_resource_post_error(resource,
WL_DISPLAY_ERROR_INVALID_OBJECT,
screenshooter failed: permission
denied);
wl_resource_destroy(resource);
diff --git a/weston.ini.in b/weston.ini.in
index 5181a9e..bc32567 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -65,3 +65,6 @@ path=@libexecdir@/weston-keyboard
#constant_accel_factor = 50
#min_accel_factor = 0.16
#max_accel_factor = 1.0
+
+#[screenshooter]
+#restrict-access=false
Bryce
Sebastian
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston] introduces a setting to give permission to any client to do screenshots
This patch adds a screenshooter section with the restrict-access
setting which is on by default and is the current behavior of weston.
When turning it off, all clients can use the screenshooter protocol.
This makes screen capturing for clients easier because they don't
have to be started by weston.
---
man/weston.ini.man | 6 ++
src/screenshooter.c | 8 +++-
weston.ini.in | 3 +++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index 6be90bf..b94ac3d 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
@@ -408,6 +408,12 @@ The terminal shell (string). Sets the $TERM variable.
sets the path to the xserver to run (string).
.RE
.RE
+.SH SCREENSHOOTER SECTION
+.TP 7
+.BI restrict-access= true
+only allow authenticated clients to take screenshots (boolean).
+.RE
+.RE
.SH SEE ALSO
.BR weston (1),
.BR weston-launch (1),
diff --git a/src/screenshooter.c b/src/screenshooter.c
index 0c657bc..65b6c09 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -224,11 +224,17 @@ bind_shooter(struct wl_client *client,
{
struct screenshooter *shooter = data;
struct wl_resource *resource;
+ struct weston_config_section *section;
+ int restrict_access;
resource = wl_resource_create(client,
screenshooter_interface, 1, id);
- if (client != shooter-client) {
+ section = weston_config_get_section(shooter-ec-config,
screenshooter, NULL, NULL);
+ weston_config_section_get_bool(section,
+ restrict-access, restrict_access, 1);
+
+ if (restrict_access client != shooter-client) {
wl_resource_post_error(resource,
WL_DISPLAY_ERROR_INVALID_OBJECT,
screenshooter failed: permission
denied);
wl_resource_destroy(resource);
diff --git a/weston.ini.in b/weston.ini.in
index 5181a9e..bc32567 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -65,3 +65,6 @@ path=@libexecdir@/weston-keyboard
#constant_accel_factor = 50
#min_accel_factor = 0.16
#max_accel_factor = 1.0
+
+#[screenshooter]
+#restrict-access=false
--
1.8.4.3
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[RFC weston] make client isolation optional
This patch allows wayland clients to use protocols which give away information
about other clients without being started by the compositor. The reason to
denie access on those protocols is to make sure no information about the
clients is leaked to other clients (=security). I think that we don't need to
enforce this on the most systems because without complete isolation of all
processes it's possible to get the information even without using the
compositor. In all other cases you can simply turn it on again.
---
man/weston.ini.man | 3 +++
src/screenshooter.c | 8 +++-
weston.ini.in | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index cc88ca8..6d41653 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
@@ -128,6 +128,9 @@ sets the GBM format used for the framebuffer for the GBM
backend. Can be
.B xrgb2101010,
.B rgb565.
By default, xrgb is used.
+.TP 7
+.BI client-isolation=false
+completely isolate clients (boolean).
.RS
.PP
diff --git a/src/screenshooter.c b/src/screenshooter.c
index 0c657bc..fa3dbb8 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -224,11 +224,17 @@ bind_shooter(struct wl_client *client,
{
struct screenshooter *shooter = data;
struct wl_resource *resource;
+ struct weston_config_section *section;
+ int client_isolation;
resource = wl_resource_create(client,
screenshooter_interface, 1, id);
- if (client != shooter-client) {
+ section = weston_config_get_section(shooter-ec-config, core, NULL,
NULL);
+ weston_config_section_get_bool(section,
+ client-isolation, client_isolation, 0);
+
+ if (client_isolation client != shooter-client) {
wl_resource_post_error(resource,
WL_DISPLAY_ERROR_INVALID_OBJECT,
screenshooter failed: permission
denied);
wl_resource_destroy(resource);
diff --git a/weston.ini.in b/weston.ini.in
index 5181a9e..1261788 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -2,6 +2,7 @@
#modules=xwayland.so,cms-colord.so
#shell=desktop-shell.so
#gbm-format=xrgb2101010
+#client-isolation=true
[shell]
background-image=/usr/share/backgrounds/gnome/Aqua.jpg
--
1.8.4.2
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
