drm: Check whether the gamma lut has changed before updating

[6by9]

drm_crtc_legacy_gamma_set updates the gamma_lut blob unconditionally,
which leads to unnecessary reprogramming of hardware.

Check whether the blob contents has actually changed before
signalling that it has been updated.

Signed-off-by: Dave Stevenson [email protected]

See #4435.

[6by9]

@mripard Thoughts? Is it better to skip the drm_atomic_commit as well?

[JamesH65]

Shame you have to do so much work before not doing the actual change. Can the change be rejected earlier in any way?

[6by9]

Can the change be rejected earlier in any way?

Not within the kernel AFAIK.
This is called in response to X calling the gamma ioctl (callstack drm_ioctl / drm_ioctl_kernel / drm_mode_gamma_set_ioctl), so this is the first point where the info is available.

If you could patch X to not try set the gamma parameter seemingly every time you open a new window, then that would be nicer, but I'm not going hacking in X.

[mripard]

That looks good to me

[6by9]

Your view on killing the drm_atomic_commit as well? Worth it or not?

The easiest abort is actually to flip it around to

if (crtc_state->gamma_lut && crtc_state->gamma_lut->data &&
	    !memcmp(crtc_state->gamma_lut->data, blob_data, blob->length))
		goto fail;

which does the puts for us. It'd be worth renaming fail though as it isn't always a failure then.

[JamesH65]

Can the change be rejected earlier in any way?

Not within the kernel AFAIK. This is called in response to X calling the gamma ioctl (callstack drm_ioctl / drm_ioctl_kernel / drm_mode_gamma_set_ioctl), so this is the first point where the info is available.

If you could patch X to not try set the gamma parameter seemingly every time you open a new window, then that would be nicer, but I'm not going hacking in X.

I was thinking earlier in the function itself, but I suspect you need to build the blob in order to test it quickly. Not looked hard enough to see if its possible.

[6by9]

I was thinking earlier in the function itself, but I suspect you need to build the blob in order to test it quickly. Not looked hard enough to see if its possible.

No.
The source is 3 arrays, red, green, and blue. The blob is one array of

struct drm_color_lut {
	__u16 red;
	__u16 green;
	__u16 blue;
	__u16 reserved;
};

Without running through the for loop you don't have the data in the same format to memcmp.

[6by9]

Side discussion with Maxime. drm_atomic_commit has other side effects, so safest to execute it even though nothing has changed.

That means that means that this PR is complete as is.