Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-04-12 06:48 PM, Leo Li wrote: > > A side question: How does xrandr display lengthy properties, like LUTs? Presumably it would be displayed the same way as the EDID property, e.g.: EDID: 000022f06f3201010101 181b0103803420782a4ca5a7554da226 105054210800b30095008100d1c0a9c0 81c0a9408180283c80a070b023403020 36000644211a00fd00323c1e 5011000a20202020202000fc0048 5020453234320a202020202000ff 00434e43373234305a37470a20200111 020318b148101f04130302121167030c 001022e2002b023a801871382d40 582c45000644211e023a80d07238 2d40102c45800644211e011d0072 51d01e206e2855000644211e011d 00bc52d01e20b82855400644211e 8c0ad08a20e02d10103e960006442100 0018002f > Can users set it via --set still? Not sure, maybe not. If not, maybe you can write a simple proof-of-concept client, that should also be useful for review and testing of these patches. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-04-12 06:30 AM, Michel Dänzer wrote: On 2018-04-11 11:26 PM, Leo Li wrote: On 2018-04-11 04:39 AM, Michel Dänzer wrote: Hmm. So either legacy or non-legacy clients won't work at all, or they'll step on each other's toes, clobbering the HW gamma LUT from each other. I'm afraid neither of those alternatives sound like a good user experience to me. Consider on the one hand something like Night Light / redshift, using legacy APIs to adjust colour temperature to the time of day. On the other hand, another client using the non-legacy API for say fine-tuning of a display's advanced gamma capabilities. Ideally, in this case the gamma LUTs from the legacy and non-legacy APIs should be combined, such that the hardware LUT reflects both the colour temperature set by Night Light / refshift and the fine-tuning set by the non-legacy client. Is that feasible? If not, can you explain a little why? Hmm, combining LUTs could be feasible, but I don't think that's the right approach. LUTs can be combined through composition h(x) = f(g(x)), with some interpolation involved. Once combined, it can be set using the non-legacy API, since that'll likely have a larger LUT size. But I think what you've mentioned raises a bigger issue of color management conflicts in general. It doesn't have to be redshift vs monitor correction, what if there more than 2 applications contending to set gamma? xrandr's brightness already conflicts with redshift, and so does some apps running on WINE. What you're describing is an X11 design flaw, which we cannot do anything about in the DDX driver. What I want to avoid is repeating a similar situation as we had before xserver 1.19, where one could have either working per-window colormaps and global gamma, or per-CRTC gamma via RandR, not all at the same time. (Before xserver 1.7, they would clobber the HW LUT from each other) I fixed this in https://cgit.freedesktop.org/xorg/xserver/commit/?id=b4e46c0444bb09f4af59d9d13acc939a0fbbc6d6 by composing the LUTs. For the small example at hand, the ideal solution is to have redshift use the color transformation matrix (CTM), which will be exposed as part of the non-legacy color API. It'll need modification of redshift, but at least it won't conflict with anything gamma related. From past experience, it can take a long time (up to forever) for some clients to be updated like this. E.g. it's unlikely at this point that libraries such as SDL1 will ever be updated for the new API, so I'm afraid users would run into things like https://bugs.freedesktop.org/show_bug.cgi?id=27222 again. (Besides, it would conflict with anything else using the same API, as you described above, so it's not even obviously the ideal solution) Fair enough, it's better to have the frequent redshift+monitor adjust use-case working now instead of pushing an entirely new API. Jumping back on some patch 1 topics: Are there ways to get arbitrarily (no more than 4096 elements) sized arrays from a client, to the DDX driver? Seems like the RRChangeOutputProperty request could work. I agree, it would definitely be nicer for clients to not worry about DRM blobs at all. Indeed. E.g. as a bonus, it would allow this to work even with a remote display connection. I'm holding off on the more detailed discussion about the other patches until there is a plan for this fundamental issue. I'll take another look at RRChangeOutputProperty. Seems I missed the 'length' argument when I first went through it. Once the blobs are gone, the other issues should be much simpler to solve. I'll see if I can come up with some v2's. A side question: How does xrandr display lengthy properties, like LUTs? Can users set it via --set still? Leo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-04-11 11:26 PM, Leo Li wrote: > On 2018-04-11 04:39 AM, Michel Dänzer wrote: >> >> Hmm. So either legacy or non-legacy clients won't work at all, or >> they'll step on each other's toes, clobbering the HW gamma LUT from >> each other. >> >> I'm afraid neither of those alternatives sound like a good user >> experience to me. >> >> Consider on the one hand something like Night Light / redshift, >> using legacy APIs to adjust colour temperature to the time of day. >> On the other hand, another client using the non-legacy API for say >> fine-tuning of a display's advanced gamma capabilities. >> >> Ideally, in this case the gamma LUTs from the legacy and non-legacy >> APIs should be combined, such that the hardware LUT reflects both >> the colour temperature set by Night Light / refshift and the >> fine-tuning set by the non-legacy client. Is that feasible? If not, >> can you explain a little why? > > Hmm, combining LUTs could be feasible, but I don't think that's the > right approach. > > LUTs can be combined through composition h(x) = f(g(x)), with some > interpolation involved. Once combined, it can be set using the > non-legacy API, since that'll likely have a larger LUT size. > > But I think what you've mentioned raises a bigger issue of color > management conflicts in general. It doesn't have to be redshift vs > monitor correction, what if there more than 2 applications contending > to set gamma? xrandr's brightness already conflicts with redshift, > and so does some apps running on WINE. What you're describing is an X11 design flaw, which we cannot do anything about in the DDX driver. What I want to avoid is repeating a similar situation as we had before xserver 1.19, where one could have either working per-window colormaps and global gamma, or per-CRTC gamma via RandR, not all at the same time. (Before xserver 1.7, they would clobber the HW LUT from each other) I fixed this in https://cgit.freedesktop.org/xorg/xserver/commit/?id=b4e46c0444bb09f4af59d9d13acc939a0fbbc6d6 by composing the LUTs. > For the small example at hand, the ideal solution is to have > redshift use the color transformation matrix (CTM), which will be > exposed as part of the non-legacy color API. It'll need modification > of redshift, but at least it won't conflict with anything gamma > related. From past experience, it can take a long time (up to forever) for some clients to be updated like this. E.g. it's unlikely at this point that libraries such as SDL1 will ever be updated for the new API, so I'm afraid users would run into things like https://bugs.freedesktop.org/show_bug.cgi?id=27222 again. (Besides, it would conflict with anything else using the same API, as you described above, so it's not even obviously the ideal solution) > Jumping back on some patch 1 topics: > > Are there ways to get arbitrarily (no more than 4096 elements) sized > arrays from a client, to the DDX driver? Seems like the RRChangeOutputProperty request could work. > I agree, it would definitely be nicer for clients to not worry about > DRM blobs at all. Indeed. E.g. as a bonus, it would allow this to work even with a remote display connection. I'm holding off on the more detailed discussion about the other patches until there is a plan for this fundamental issue. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-04-11 04:39 AM, Michel Dänzer wrote: On 2018-04-10 08:02 PM, Leo Li wrote: On 2018-04-09 11:03 AM, Michel Dänzer wrote: On 2018-03-26 10:00 PM, sunpeng...@amd.com wrote: From: "Leo (Sunpeng) Li" In cases where CRTC properties are updated without going through RRChangeOutputProperty, we don't update the properties in user land. Consider setting legacy gamma. It doesn't go through RRChangeOutputProperty, but modifies the CRTC's color management properties. Unless they are updated, the user properties will remain stale. Can you describe a bit more how the legacy gamma and the new properties interact? Sure thing, I'll include this in the message for v2: In kernel, the legacy set gamma interface is essentially an adapter to the non-legacy set properties interface. In the end, they both set the same property to a DRM property blob, which contains the gamma lookup table. The key difference between them is how this blob is created. For legacy gamma, the kernel takes 3 arrays from user-land, and creates the blob using them. Note that a blob is identified by it's blob_id. For non-legacy gamma, the kernel takes a blob_id from user-land that references the blob. This means user-land is responsible for creating the blob. From the perspective of RandR, this presents some problems. Since both paths modify the same property, RandR must keep the reported property value up-to-date with which ever path is used: 1. Legacy gamma via xrandr --output --gamma x:x:x 2. Non-legacy color properties via xrandr --output --set GAMMA_LUT Keeping the value up-to-date isn't a problem for 2, since RandR updates it for us as part of changing output properties. But if 1 is used, the property blob is created within kernel, and RandR is unaware of the new blob_id. To update it, we need to ask kernel about it. --- continue with rest of message --- Therefore, add a function to update user CRTC properties by querying DRM, and call it whenever legacy gamma is changed. Note that drmmode_crtc_gamma_do_set is called from drmmode_set_mode_major, i.e. on every modeset or under some circumstances when a DRI3 client stops page flipping. The property will have to be updated each time the legacy set gamma ioctl is called, since a new blob (with a new blob_id) is created each time. Not sure if this is a good idea, but perhaps we can have a flag that explicitly enable one or the other, depending on user preference? A user-only property with something like: 0: Use legacy gamma, calls to change non-legacy properties are ignored. 1: Use non-legacy, calls to legacy gamma will be ignored. On 0, we can remove/disable all non-legacy properties from the property list, and avoid having to update them. On 1, we'll enable the properties, and won't have to update them either since legacy gamma is "disabled". It has the added benefit of avoiding unexpected legacy gamma sets when using non-legacy, and vice versa. Hmm. So either legacy or non-legacy clients won't work at all, or they'll step on each other's toes, clobbering the HW gamma LUT from each other. I'm afraid neither of those alternatives sound like a good user experience to me. Consider on the one hand something like Night Light / redshift, using legacy APIs to adjust colour temperature to the time of day. On the other hand, another client using the non-legacy API for say fine-tuning of a display's advanced gamma capabilities. Ideally, in this case the gamma LUTs from the legacy and non-legacy APIs should be combined, such that the hardware LUT reflects both the colour temperature set by Night Light / refshift and the fine-tuning set by the non-legacy client. Is that feasible? If not, can you explain a little why? Hmm, combining LUTs could be feasible, but I don't think that's the right approach. LUTs can be combined through composition h(x) = f(g(x)), with some interpolation involved. Once combined, it can be set using the non-legacy API, since that'll likely have a larger LUT size. But I think what you've mentioned raises a bigger issue of color management conflicts in general. It doesn't have to be redshift vs monitor correction, what if there more than 2 applications contending to set gamma? xrandr's brightness already conflicts with redshift, and so does some apps running on WINE. Ultimately, any (legacy or not) gamma set requests are unified into one CRTC property in DRM, and they will all conflict if not managed. I don't think combining two LUTs will help here. For the small example at hand, the ideal solution is to have redshift use the color transformation matrix (CTM), which will be exposed as part of the non-legacy color API. It'll need modification of redshift, but at least it won't conflict with anything gamma related. Jumping back on some patch 1 topics: Are there ways to get arbitrarily (no more than 4096 elements) sized arrays from a client, to the DDX driver? Otherwise, I'm not sure if there's another way to expose these properties,
Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-04-10 08:02 PM, Leo Li wrote: > On 2018-04-09 11:03 AM, Michel Dänzer wrote: >> On 2018-03-26 10:00 PM, sunpeng...@amd.com wrote: >>> From: "Leo (Sunpeng) Li" >>> >>> In cases where CRTC properties are updated without going through >>> RRChangeOutputProperty, we don't update the properties in user land. >>> >>> Consider setting legacy gamma. It doesn't go through >>> RRChangeOutputProperty, but modifies the CRTC's color management >>> properties. Unless they are updated, the user properties will remain >>> stale. >> >> Can you describe a bit more how the legacy gamma and the new properties >> interact? >> > > Sure thing, I'll include this in the message for v2: > > In kernel, the legacy set gamma interface is essentially an adapter to > the non-legacy set properties interface. In the end, they both set the > same property to a DRM property blob, which contains the gamma lookup > table. The key difference between them is how this blob is created. > > For legacy gamma, the kernel takes 3 arrays from user-land, and creates > the blob using them. Note that a blob is identified by it's blob_id. > > For non-legacy gamma, the kernel takes a blob_id from user-land that > references the blob. This means user-land is responsible for creating > the blob. > > From the perspective of RandR, this presents some problems. Since both > paths modify the same property, RandR must keep the reported property > value up-to-date with which ever path is used: > > 1. Legacy gamma via > xrandr --output --gamma x:x:x > 2. Non-legacy color properties via > xrandr --output --set GAMMA_LUT > > Keeping the value up-to-date isn't a problem for 2, since RandR updates > it for us as part of changing output properties. > > But if 1 is used, the property blob is created within kernel, and RandR > is unaware of the new blob_id. To update it, we need to ask kernel about > it. > > --- continue with rest of message --- >> >>> Therefore, add a function to update user CRTC properties by querying >>> DRM, >>> and call it whenever legacy gamma is changed. >> >> Note that drmmode_crtc_gamma_do_set is called from >> drmmode_set_mode_major, i.e. on every modeset or under some >> circumstances when a DRI3 client stops page flipping. >> > > The property will have to be updated each time the legacy set gamma > ioctl is called, since a new blob (with a new blob_id) is created each > time. > > Not sure if this is a good idea, but perhaps we can have a flag that > explicitly enable one or the other, depending on user preference? A > user-only property with something like: > > 0: Use legacy gamma, calls to change non-legacy properties are ignored. > 1: Use non-legacy, calls to legacy gamma will be ignored. > > On 0, we can remove/disable all non-legacy properties from the property > list, and avoid having to update them. On 1, we'll enable the > properties, and won't have to update them either since legacy gamma is > "disabled". It has the added benefit of avoiding unexpected legacy gamma > sets when using non-legacy, and vice versa. Hmm. So either legacy or non-legacy clients won't work at all, or they'll step on each other's toes, clobbering the HW gamma LUT from each other. I'm afraid neither of those alternatives sound like a good user experience to me. Consider on the one hand something like Night Light / redshift, using legacy APIs to adjust colour temperature to the time of day. On the other hand, another client using the non-legacy API for say fine-tuning of a display's advanced gamma capabilities. Ideally, in this case the gamma LUTs from the legacy and non-legacy APIs should be combined, such that the hardware LUT reflects both the colour temperature set by Night Light / refshift and the fine-tuning set by the non-legacy client. Is that feasible? If not, can you explain a little why? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-04-09 11:03 AM, Michel Dänzer wrote:
On 2018-03-26 10:00 PM, sunpeng...@amd.com wrote:
From: "Leo (Sunpeng) Li"
In cases where CRTC properties are updated without going through
RRChangeOutputProperty, we don't update the properties in user land.
Consider setting legacy gamma. It doesn't go through
RRChangeOutputProperty, but modifies the CRTC's color management
properties. Unless they are updated, the user properties will remain
stale.
Can you describe a bit more how the legacy gamma and the new properties
interact?
Sure thing, I'll include this in the message for v2:
In kernel, the legacy set gamma interface is essentially an adapter to
the non-legacy set properties interface. In the end, they both set the
same property to a DRM property blob, which contains the gamma lookup
table. The key difference between them is how this blob is created.
For legacy gamma, the kernel takes 3 arrays from user-land, and creates
the blob using them. Note that a blob is identified by it's blob_id.
For non-legacy gamma, the kernel takes a blob_id from user-land that
references the blob. This means user-land is responsible for creating
the blob.
From the perspective of RandR, this presents some problems. Since both
paths modify the same property, RandR must keep the reported property
value up-to-date with which ever path is used:
1. Legacy gamma via
xrandr --output --gamma x:x:x
2. Non-legacy color properties via
xrandr --output --set GAMMA_LUT
Keeping the value up-to-date isn't a problem for 2, since RandR updates
it for us as part of changing output properties.
But if 1 is used, the property blob is created within kernel, and RandR
is unaware of the new blob_id. To update it, we need to ask kernel about it.
--- continue with rest of message ---
Therefore, add a function to update user CRTC properties by querying DRM,
and call it whenever legacy gamma is changed.
Note that drmmode_crtc_gamma_do_set is called from
drmmode_set_mode_major, i.e. on every modeset or under some
circumstances when a DRI3 client stops page flipping.
The property will have to be updated each time the legacy set gamma
ioctl is called, since a new blob (with a new blob_id) is created each time.
Not sure if this is a good idea, but perhaps we can have a flag that
explicitly enable one or the other, depending on user preference? A
user-only property with something like:
0: Use legacy gamma, calls to change non-legacy properties are ignored.
1: Use non-legacy, calls to legacy gamma will be ignored.
On 0, we can remove/disable all non-legacy properties from the property
list, and avoid having to update them. On 1, we'll enable the
properties, and won't have to update them either since legacy gamma is
"disabled". It has the added benefit of avoiding unexpected legacy gamma
sets when using non-legacy, and vice versa.
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 1966fd2..45457c4 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -61,8 +61,13 @@
#define DEFAULT_NOMINAL_FRAME_RATE 60
+/* Forward declarations */
+
static Bool drmmode_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height);
+static void drmmode_crtc_update_resources(xf86CrtcPtr crtc);
Can you move the drmmode_crtc_update_resources such that the forward
declaration isn't necessary?
Seems possible. It uses the rr_configure_and_change helper, so I'll pull
both of them up.
static Bool
AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, char *output_name)
{
@@ -768,6 +773,7 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red,
uint16_t *green,
drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
size, red, green, blue);
+ drmmode_crtc_update_resources(crtc);
}
Bool
@@ -1653,10 +1659,15 @@ static Bool drmmode_property_ignore(drmModePropertyPtr
prop)
* Configure and change the given output property through randr. Currently
* ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
*
+* @output: The output to configure and change the property on.
+* @pmode_prop: The driver-private property object.
These two should have been added in patch 1.
Yep, will move.
Leo
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent
On 2018-03-26 10:00 PM, sunpeng...@amd.com wrote:
> From: "Leo (Sunpeng) Li"
>
> In cases where CRTC properties are updated without going through
> RRChangeOutputProperty, we don't update the properties in user land.
>
> Consider setting legacy gamma. It doesn't go through
> RRChangeOutputProperty, but modifies the CRTC's color management
> properties. Unless they are updated, the user properties will remain
> stale.
Can you describe a bit more how the legacy gamma and the new properties
interact?
> Therefore, add a function to update user CRTC properties by querying DRM,
> and call it whenever legacy gamma is changed.
Note that drmmode_crtc_gamma_do_set is called from
drmmode_set_mode_major, i.e. on every modeset or under some
circumstances when a DRI3 client stops page flipping.
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 1966fd2..45457c4 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -61,8 +61,13 @@
>
> #define DEFAULT_NOMINAL_FRAME_RATE 60
>
> +/* Forward declarations */
> +
> static Bool drmmode_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height);
>
> +static void drmmode_crtc_update_resources(xf86CrtcPtr crtc);
Can you move the drmmode_crtc_update_resources such that the forward
declaration isn't necessary?
> static Bool
> AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, char
> *output_name)
> {
> @@ -768,6 +773,7 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t
> *red, uint16_t *green,
>
> drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
> size, red, green, blue);
> + drmmode_crtc_update_resources(crtc);
> }
>
> Bool
> @@ -1653,10 +1659,15 @@ static Bool
> drmmode_property_ignore(drmModePropertyPtr prop)
> * Configure and change the given output property through randr. Currently
> * ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
> *
> +* @output: The output to configure and change the property on.
> +* @pmode_prop: The driver-private property object.
These two should have been added in patch 1.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
