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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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

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