Hi Am 11.05.22 um 17:28 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.v2: - Add a default linear gamma function - renamed functions with mgag200 prefix - use format's 4cc code instead of bit depth - use better interpolation for 16bits gamma - remove legacy function mga_crtc_load_lut() - can't remove the call to drm_mode_crtc_set_gamma_size() because it doesn't work with userspace. - other small refactors Signed-off-by: Jocelyn Falempe <jfale...@redhat.com>
I already gave a Tested-by on the first iteration. It's good practice to add these tags in follow-up patches unless the patch has changed entirely.
A few more comments are below. With those fixed: Reviewed-by: Thomas Zimmermann <tzimemrm...@suse.de>I suggest to post another version of the patch and merge it if no further comments arrive within 2 days.
--- drivers/gpu/drm/mgag200/mgag200_mode.c | 125 ++++++++++++++++--------- 1 file changed, 81 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 6e18d3bbd720..b748bc5b0e93 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -32,57 +32,76 @@ * This file contains setup code for the CRTC. */-static void mga_crtc_load_lut(struct drm_crtc *crtc)+static void mgag200_crtc_set_gamma_linear(struct mga_device *mdev, + uint32_t format) { - struct drm_device *dev = crtc->dev; - struct mga_device *mdev = to_mga_device(dev); - struct drm_framebuffer *fb; - u16 *r_ptr, *g_ptr, *b_ptr; int i;- if (!crtc->enabled)- return; - - if (!mdev->display_pipe.plane.state) - return; + WREG8(DAC_INDEX + MGA1064_INDEX, 0);- fb = mdev->display_pipe.plane.state->fb;+ switch (format) { + case DRM_FORMAT_RGB565: + /* Use better interpolation, to take 32 values from 0 to 255 */ + for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) { + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4); + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16); + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4); + } + /* Green has one more bit, so add padding with 0 for red and blue. */ + for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) { + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0); + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16); + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0); + } + break; + case DRM_FORMAT_RGB888: + case DRM_FORMAT_XRGB8888: + for (i = 0; i < MGAG200_LUT_SIZE; i++) { + WREG8(DAC_INDEX + MGA1064_COL_PAL, i); + WREG8(DAC_INDEX + MGA1064_COL_PAL, i); + WREG8(DAC_INDEX + MGA1064_COL_PAL, i); + }
These loops look much nicer to me.
+ break; + default: + drm_warn_once(&mdev->base, "Unsupported format for gamma %d\n", format);
There's a print format modifier for 4cc formats. It's %p4cc and expects a pointer to the format's 4cc code. See 'git grep p4cc' for examples.
The comment itself is not easy to understand. Maybe "Unsupported format %p4cc for gamma correction.\n" ?
+ break; + } +}- r_ptr = crtc->gamma_store;- g_ptr = r_ptr + crtc->gamma_size; - b_ptr = g_ptr + crtc->gamma_size; +static void mgag200_crtc_set_gamma(struct mga_device *mdev, + struct drm_color_lut *lut, + uint32_t format) +{ + int i;WREG8(DAC_INDEX + MGA1064_INDEX, 0); - if (fb && fb->format->cpp[0] * 8 == 16) {- int inc = (fb->format->depth == 15) ? 8 : 4; - u8 r, b; - for (i = 0; i < MGAG200_LUT_SIZE; i += inc) { - if (fb->format->depth == 16) { - if (i > (MGAG200_LUT_SIZE >> 1)) { - r = b = 0; - } else { - r = *r_ptr++ >> 8; - b = *b_ptr++ >> 8; - r_ptr++; - b_ptr++; - } - } else { - r = *r_ptr++ >> 8; - b = *b_ptr++ >> 8; - } - /* VGA registers */ - WREG8(DAC_INDEX + MGA1064_COL_PAL, r); - WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8); - WREG8(DAC_INDEX + MGA1064_COL_PAL, b); + switch (format) { + case DRM_FORMAT_RGB565: + /* Use better interpolation, to take 32 values from lut[0] to lut[255] */ + for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) { + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].red >> 8); + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i / 16].green >> 8); + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].blue >> 8); } - return; - } - for (i = 0; i < MGAG200_LUT_SIZE; i++) { - /* VGA registers */ - WREG8(DAC_INDEX + MGA1064_COL_PAL, *r_ptr++ >> 8); - WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8); - WREG8(DAC_INDEX + MGA1064_COL_PAL, *b_ptr++ >> 8); + /* Green has one more bit, so add padding with 0 for red and blue. */ + for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) { + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0); + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i / 16].green >> 8); + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0); + } + break; + case DRM_FORMAT_RGB888: + case DRM_FORMAT_XRGB8888: + 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); + } + break; + default: + drm_warn_once(&mdev->base, "Unsupported format for gamma %d\n", format);
Same as above.
+ break; } }@@ -900,7 +919,11 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,if (mdev->type == G200_WB || mdev->type == G200_EW3) mgag200_g200wb_release_bmc(mdev);- mga_crtc_load_lut(crtc);+ if (crtc_state->gamma_lut) + mgag200_crtc_set_gamma(mdev, crtc_state->gamma_lut->data, fb->format->format);
Nitpicking: I'd give the format before the LUT data. It's more logical and aligns with '_set_gamma_linear'. I'd also pass fb->format instead of fb->format->format.
+ else + mgag200_crtc_set_gamma_linear(mdev, fb->format->format); + mgag200_enable_display(mdev);mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);@@ -945,6 +968,14 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe, return ret; }+ if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {+ if (crtc_state->gamma_lut->length != + MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) { + drm_err(dev, "Wrong size for gamma_lut %ld\n",
The kernel bot complained about '%ld'. I think %zu is the one for size_t.
+ crtc_state->gamma_lut->length); + return -EINVAL; + } + } return 0; }@@ -953,6 +984,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,6 +995,9 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, if (!fb) return;+ if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)+ mgag200_crtc_set_gamma(mdev, crtc->state->gamma_lut->data, fb->format->format); +
This also needs a call to _set_gamma_linear? Best regards Thomas
if (drm_atomic_helper_damage_merged(old_state, state, &damage)) mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]); } @@ -1107,9 +1142,11 @@ int mgag200_modeset_init(struct mga_device *mdev) return ret; }- /* FIXME: legacy gamma tables; convert to CRTC state */+ /* FIXME: legacy gamma tables, but atomic gamma doesn't work without */ 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;
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
