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@redhat.com --- 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) } }
+static void mga_crtc_update_lut(struct mga_device *mdev, + struct drm_crtc_state *state, + u8 depth) +{ + struct drm_color_lut * lut; + int i; + + if (!state->color_mgmt_changed || !state->gamma_lut) + return + + lut = (struct drm_color_lut *) state->gamma_lut->data; + WREG8(DAC_INDEX + MGA1064_INDEX, 0); + + if (depth == 15) { + /* 16 bits r5g5b5a1 */ + 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; + 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); + } + } +} + 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); + if (drm_atomic_helper_damage_merged(old_state, state, &damage)) + 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);
+ drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE); + drm_mode_config_reset(dev);
return 0;
Hi,
first of all
Tested-by: Thomas Zimemrmann tzimmermann@suse.de
on G200EH. I clicked a bit in Gnome settings and the display changed colors. It's pretty cool.
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@redhat.com
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.
+{
- struct drm_color_lut * lut;
- int i;
- if (!state->color_mgmt_changed || !state->gamma_lut)
return
Semicolon is missing here.
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.
- 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; }
/* 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.
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.)
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 }
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.
if (drm_atomic_helper_damage_merged(old_state, state, &damage))
That empty line is fallout from rebasing from the other patchset?
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.
- 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;
On 09/05/2022 16:22, Thomas Zimmermann wrote:
Hi,
first of all
Tested-by: Thomas Zimemrmann tzimmermann@suse.de
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@redhat.com
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.
Hi
Am 09.05.22 um 17:43 schrieb Jocelyn Falempe:
On 09/05/2022 16:22, Thomas Zimmermann wrote:
Hi,
first of all
Tested-by: Thomas Zimemrmann tzimmermann@suse.de
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@redhat.com
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.
If you want to remove it, go ahead. If we'd want to support 15-bit modes, it would be trivial to add.
+ /* 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.
I'm looking at gamma curves as given in [1]. The typical display gamma of ~2 gives a exponential-looking graph, which starts flat but grows quickly.
Michel is right that we ideally what full-dark and full-bright as-is, and do some interpolation in between. In your reply to Michel, I think you already gave a simple and correct-enough implementation. I'd just go with that.
Best regards Thomas
[1] https://www.cambridgeincolour.com/tutorials/gamma-correction.htm
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.
On 2022-05-09 16:22, Thomas Zimmermann wrote:
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 don't think that's accurate. The most common ramp should be a straight line from 0 to the maximum value, and others may be curved toward the top or bottom.
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 }
That'll just drop the first 3-7 entries of the LUT instead of the last ones, i.e. generally the full black entries instead of the full white ones.
Ideally, the loop should start at 0 and then count as evenly as possible up to 255/63/31. I realize that's tricky though, and I don't have any specific suggestions for how to achieve this offhand.
On 09/05/2022 18:04, Michel Dänzer wrote:
On 2022-05-09 16:22, Thomas Zimmermann wrote:
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 don't think that's accurate. The most common ramp should be a straight line from 0 to the maximum value, and others may be curved toward the top or bottom.
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 }
That'll just drop the first 3-7 entries of the LUT instead of the last ones, i.e. generally the full black entries instead of the full white ones.
Ideally, the loop should start at 0 and then count as evenly as possible up to 255/63/31. I realize that's tricky though, and I don't have any specific suggestions for how to achieve this offhand.
If you want 32 values from the 256 table, something like this should work:
for (i=0; i<32; i++) { lut_index = i * 8 + i / 4; }
lut_index will have this value:
0, 8, 16, 24, 33, 41, 49, 57, 66, 74, 82, 90, 99, 107, 115, 123, 132, 140, 148, 156, 165, 173, 181, 189, 198, 206, 214, 222, 231, 239, 247, 255
Hi
Am 09.05.22 um 23:00 schrieb Jocelyn Falempe:
On 09/05/2022 18:04, Michel Dänzer wrote:
On 2022-05-09 16:22, Thomas Zimmermann wrote:
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 don't think that's accurate. The most common ramp should be a straight line from 0 to the maximum value, and others may be curved toward the top or bottom.
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 }
That'll just drop the first 3-7 entries of the LUT instead of the last ones, i.e. generally the full black entries instead of the full white ones.
Ideally, the loop should start at 0 and then count as evenly as possible up to 255/63/31. I realize that's tricky though, and I don't have any specific suggestions for how to achieve this offhand.
If you want 32 values from the 256 table, something like this should work:
for (i=0; i<32; i++) { lut_index = i * 8 + i / 4; }
lut_index will have this value:
0, 8, 16, 24, 33, 41, 49, 57, 66, 74, 82, 90, 99, 107, 115, 123, 132, 140, 148, 156, 165, 173, 181, 189, 198, 206, 214, 222, 231, 239, 247, 255
Great. That's even better.
Best regards Thomas
Hi Jocelyn,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tegra-drm/drm/tegra/for-next] [also build test ERROR on v5.18-rc6] [cannot apply to drm/drm-next drm-tip/drm-tip airlied/drm-next next-20220509] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/mgag200-Enabl... base: git://anongit.freedesktop.org/tegra/linux.git drm/tegra/for-next config: mips-randconfig-r015-20220509 (https://download.01.org/0day-ci/archive/20220510/202205100516.IJQ7MRHW-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a385645b470e2d3a1534aae618ea56b31177639f) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install mips cross compiling tool for clang build # apt-get install binutils-mips-linux-gnu # https://github.com/intel-lab-lkp/linux/commit/c0e0a39a5acd3aa9d0cd6f25679bd1... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220509-175430 git checkout c0e0a39a5acd3aa9d0cd6f25679bd16930233491 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpu/drm/mgag200/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/mgag200/mgag200_mode.c:97:3: error: void function 'mga_crtc_update_lut' should not return a value [-Wreturn-type]
return ^ 1 error generated.
vim +/mga_crtc_update_lut +97 drivers/gpu/drm/mgag200/mgag200_mode.c
88 89 static void mga_crtc_update_lut(struct mga_device *mdev, 90 struct drm_crtc_state *state, 91 u8 depth) 92 { 93 struct drm_color_lut * lut; 94 int i; 95 96 if (!state->color_mgmt_changed || !state->gamma_lut)
97 return
98 99 lut = (struct drm_color_lut *) state->gamma_lut->data; 100 WREG8(DAC_INDEX + MGA1064_INDEX, 0); 101 102 if (depth == 15) { 103 /* 16 bits r5g5b5a1 */ 104 for (i = 0; i < MGAG200_LUT_SIZE; i += 8) { 105 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8); 106 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); 107 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8); 108 } 109 } else if (depth == 16) { 110 /* 16 bits r5g6b5, as green has one more bit, 111 * add padding with 0 for red and blue. */ 112 for (i = 0; i < MGAG200_LUT_SIZE; i += 4) { 113 u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; 114 u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; 115 WREG8(DAC_INDEX + MGA1064_COL_PAL, red); 116 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); 117 WREG8(DAC_INDEX + MGA1064_COL_PAL, blue); 118 } 119 } else { 120 /* 24 bits r8g8b8 */ 121 for (i = 0; i < MGAG200_LUT_SIZE; i++) { 122 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8); 123 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); 124 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8); 125 } 126 } 127 } 128
dri-devel@lists.freedesktop.org
-
Jocelyn Falempe -
kernel test robot -
Michel Dänzer -
Thomas Zimmermann