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 >> >> 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 >>> --- >>>   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. > -- 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