[PATCH v2] mgag200: Enable atomic gamma lut update
Jocelyn Falempe
jfalempe at redhat.com
Thu May 12 10:08:33 UTC 2022
On 12/05/2022 10:52, Thomas Zimmermann wrote:
> 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 <jfalempe at 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.
Sorry, I though I changed too much code in v2 to add it.
>
> A few more comments are below. With those fixed:
>
> Reviewed-by: Thomas Zimmermann <tzimemrmann at 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.
ok, that's a cool feature.
>
> The comment itself is not easy to understand. Maybe "Unsupported format
> %p4cc for gamma correction.\n" ?
Sure, having good error message is always helpful.
>
>> + 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.
ok, I will do that too.
>
>> + 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.
I had no warnings when building on x86_64, but printf format is a bit
tricky to get right.
>
>> + 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?
No, I think the gamma table should be properly initialized in
mgag200_simple_display_pipe_enable(), so only set it if userspace gives
a new table.
>
> 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;
>
Thanks for your review, I will send a v3 soon,
--
Jocelyn
More information about the dri-devel
mailing list