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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 6405 bytes --]

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;

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]