On 09/05/2022 16:22, Thomas Zimmermann wrote:
Hi,
first of all
Tested-by: Thomas Zimemrmann <tzimmermann@xxxxxxx>
on G200EH. I clicked a bit in Gnome settings and the display changed
colors. It's pretty cool.
yeah, I also played a bit with https://github.com/zb3/gnome-gamma-tool
Am 09.05.22 um 11:49 schrieb Jocelyn Falempe:
Add support for atomic update of gamma lut.
With this patch the "Night light" feature of gnome3
is working properly on mgag200.
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---
drivers/gpu/drm/mgag200/mgag200_mode.c | 46 ++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 6e18d3bbd720..9fc688e15db8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -86,6 +86,46 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc)
mga_crtc_load_lut is legacy code and needs to go away.
}
}
+static void mga_crtc_update_lut(struct mga_device *mdev,
+ struct drm_crtc_state *state,
+ u8 depth)
Rather name this function mgag200_crtc_set_gamma().
The driver was once ported from X11 userspace, where it was called mga.
Thus the occational mga_ prefix. It it should now be mgag200.
ok
+{
+ struct drm_color_lut * lut;
+ int i;
+
+ if (!state->color_mgmt_changed || !state->gamma_lut)
+ return
Semicolon is missing here.
oops ;)
The test itself should go into the caller. The update function here
should be independent from the crtc state. Pass in the plane state's
framebuffer and the crtc state's gamma_lut property.
ok, it makes sense.
+
+ lut = (struct drm_color_lut *) state->gamma_lut->data;
+ WREG8(DAC_INDEX + MGA1064_INDEX, 0);
+
+ if (depth == 15) {
format->depth is deprecated. Better write these if-else branches as
switch of the format's 4cc code:
switch (fb->format->format) {
case DRM_FORMAT_XRGB1555:
...
break;
case DRM_FORMAT_RGB565:
...
break;
case DRM_FORMAT_RGB888:
case DRM_FORMAT_XRGB:
...
break;
}
As the driver doesn't advertise XRGB1555, maybe I can drop it ?
I kept it because the mga_crtc_load_lut() supports it.
+ /* 16 bits r5g5b5a1 */
With 4cc codes, you can remove these comments.
+ for (i = 0; i < MGAG200_LUT_SIZE; i += 8) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
+ }
+ } else if (depth == 16) {
+ /* 16 bits r5g6b5, as green has one more bit,
+ * add padding with 0 for red and blue. */
+ for (i = 0; i < MGAG200_LUT_SIZE; i += 4) {
+ u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0;
+ u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8
: 0;
'[].blue' here.
oops again ;)
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, red);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, blue);
+ }
+ } else {
+ /* 24 bits r8g8b8 */
+ for (i = 0; i < MGAG200_LUT_SIZE; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
+ }
+ }
+}
+
These loops seem hard to understand because the index i doesn't
obviously correspond to the source or destination; except for the final
one.
I'd write out the offset into the HW palette as constant value in the
for loop and walk over the given lut table via pointer arithmetic.
It might also make sense to adjust the starting value of the lut table
such that its final entry is used for the final entry in the HW palette.
For typical gamma ramps ~2, the curve is fairly flat for small values
and goes up steeply at high values. (Please correct me if I'm
misinterpreting the gamma ramps.)
I didn't realize that taking 1 out of 8 values will have this side
effect. sure this can be fixed.
For 15-bit case I'd do thing like this.
lut += 7;
for (i < 0; i < 32; ++i, lut += 8) {
// write lut
}
16-bit is complicated and may better be done in 2 loops
lutr += 7;
lutg += 3;
lutb += 7;
for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) {
// write r/g/b lut
}
for (; i < 64; ++i, lutg += 3) {
// write 0/g/0 lut
}
ok, will try to do something like this. It took me a while to understand
the loops of mga_crtc_load_lut(), so I tried to simplify.
static inline void mga_wait_vsync(struct mga_device *mdev)
{
unsigned long timeout = jiffies + HZ/10;
@@ -953,6 +993,7 @@ mgag200_simple_display_pipe_update(struct
drm_simple_display_pipe *pipe,
struct drm_plane_state *old_state)
{
struct drm_plane *plane = &pipe->plane;
+ struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *dev = plane->dev;
struct mga_device *mdev = to_mga_device(dev);
struct drm_plane_state *state = plane->state;
@@ -963,7 +1004,10 @@ mgag200_simple_display_pipe_update(struct
drm_simple_display_pipe *pipe,
if (!fb)
return;
+ mga_crtc_update_lut(mdev, crtc->state, fb->format->depth);
+
We should also call this function in pipe_enable.
And there's the question what happens if gamma_lut is not set. So far,
we get away with it because mga_crtc_load_lut(). A better approach is
to add another function mgag200_crtc_set_gamma_linear() that clears the
palette to a linear curve (i.e., same as mga_crtc_load_lut() does now).
It would be called if no crtc->state->gamma_lut is NULL.
Yes, if I remove mga_crtc_load_lut() I will need to set the default.
should be simple to do.
if (drm_atomic_helper_damage_merged(old_state, state, &damage))
+
That empty line is fallout from rebasing from the other patchset?
yes ;)
mgag200_handle_damage(mdev, fb, &damage,
&shadow_plane_state->data[0]);
}
@@ -1110,6 +1154,8 @@ int mgag200_modeset_init(struct mga_device *mdev)
/* FIXME: legacy gamma tables; convert to CRTC state */
drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
Here's another legacy call that should get removed.
Yes, I can remove that one too.
+ drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE);
AFAICT the DRM core does not enforce the LUT size. It's just information
for userspace, which could give any amount of palette entries. So the
driver's pipe_check function has to enforce the limit. If the gamma_lut
property is set, it should always contain 256 entries.
Best regards
Thomas
+
drm_mode_config_reset(dev);
return 0;
Thanks a lot for your review, I will rework this and send a v2.
--
Jocelyn
