Re: [PATCH] mgag200: Enable atomic gamma lut update

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux