From: Jocelyn Falempe <jfalempe@redhat.com>
To: Thomas Zimmermann <tzimmermann@suse.de>,
javierm@redhat.com, airlied@redhat.com, simona@ffwll.ch,
airlied@gmail.com, maarten.lankhorst@linux.intel.com,
mripard@kernel.org
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/5] drm/mgag200: Use helpers for programming gamma ramps
Date: Tue, 13 May 2025 14:27:12 +0200 [thread overview]
Message-ID: <66c4553e-17ec-428e-8a3b-73a51af65cc1@redhat.com> (raw)
In-Reply-To: <20250509083911.39018-4-tzimmermann@suse.de>
On 09/05/2025 10:23, Thomas Zimmermann wrote:
> Replace mgag200's code for programming the hardware gamma LUT with
> DRM helpers. Either load a provided gamma ramp or program a default.
> Set the individual entries with a callback.
>
> Each gamma value is given as 3 individual 16-bit values for red,
> green and blue. The driver reduces them to 8 bit to make them fit
> into hardware registers.
Thanks, it looks good to me.
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/mgag200/mgag200_drv.h | 4 +-
> drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 +-
> drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 +-
> drivers/gpu/drm/mgag200/mgag200_g200se.c | 4 +-
> drivers/gpu/drm/mgag200/mgag200_mode.c | 78 ++++++++++--------------
> 5 files changed, 40 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 819a7e9381e3..7d481e11f9d6 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -382,8 +382,8 @@ int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
> .destroy = drm_plane_cleanup, \
> DRM_GEM_SHADOW_PLANE_FUNCS
>
> -void mgag200_crtc_set_gamma_linear(struct mga_device *mdev, const struct drm_format_info *format);
> -void mgag200_crtc_set_gamma(struct mga_device *mdev,
> +void mgag200_crtc_fill_gamma(struct mga_device *mdev, const struct drm_format_info *format);
> +void mgag200_crtc_load_gamma(struct mga_device *mdev,
> const struct drm_format_info *format,
> struct drm_color_lut *lut);
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index c20ed0ab50ec..23debc70dc54 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -200,9 +200,9 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> mgag200_g200er_reset_tagfifo(mdev);
>
> if (crtc_state->gamma_lut)
> - mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
> + mgag200_crtc_load_gamma(mdev, format, crtc_state->gamma_lut->data);
> else
> - mgag200_crtc_set_gamma_linear(mdev, format);
> + mgag200_crtc_fill_gamma(mdev, format);
>
> mgag200_enable_display(mdev);
> }
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index 78be964eb97c..f8796e2b7a0f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -201,9 +201,9 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> mgag200_g200ev_set_hiprilvl(mdev);
>
> if (crtc_state->gamma_lut)
> - mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
> + mgag200_crtc_load_gamma(mdev, format, crtc_state->gamma_lut->data);
> else
> - mgag200_crtc_set_gamma_linear(mdev, format);
> + mgag200_crtc_fill_gamma(mdev, format);
>
> mgag200_enable_display(mdev);
> }
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index 7a32d3b1d226..e80da12ba1fe 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -332,9 +332,9 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> mgag200_g200se_set_hiprilvl(mdev, adjusted_mode, format);
>
> if (crtc_state->gamma_lut)
> - mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
> + mgag200_crtc_load_gamma(mdev, format, crtc_state->gamma_lut->data);
> else
> - mgag200_crtc_set_gamma_linear(mdev, format);
> + mgag200_crtc_fill_gamma(mdev, format);
>
> mgag200_enable_display(mdev);
> }
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 6067d08aeee3..39bfb9f4b205 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -13,6 +13,7 @@
>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_color_mgmt.h>
> #include <drm/drm_damage_helper.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_format_helper.h>
> @@ -30,35 +31,37 @@
> * This file contains setup code for the CRTC.
> */
>
> -void mgag200_crtc_set_gamma_linear(struct mga_device *mdev,
> - const struct drm_format_info *format)
> +static void mgag200_set_gamma_lut(struct drm_crtc *crtc, unsigned int index,
> + u16 red, u16 green, u16 blue)
> {
> - int i;
> + struct drm_device *dev = crtc->dev;
> + struct mga_device *mdev = to_mga_device(dev);
> + u8 i8 = index & 0xff;
> + u8 r8 = red >> 8;
> + u8 g8 = green >> 8;
> + u8 b8 = blue >> 8;
> +
> + if (drm_WARN_ON_ONCE(dev, index != i8))
> + return; /* driver bug */
> +
> + WREG8(DAC_INDEX + MGA1064_INDEX, i8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, r8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, g8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, b8);
> +}
>
> - WREG8(DAC_INDEX + MGA1064_INDEX, 0);
> +void mgag200_crtc_fill_gamma(struct mga_device *mdev,
> + const struct drm_format_info *format)
> +{
> + struct drm_crtc *crtc = &mdev->crtc;
>
> switch (format->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);
> - }
> + drm_crtc_fill_gamma_565(crtc, mgag200_set_gamma_lut);
> 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);
> - }
> + drm_crtc_fill_gamma_888(crtc, mgag200_set_gamma_lut);
> break;
> default:
> drm_warn_once(&mdev->base, "Unsupported format %p4cc for gamma correction\n",
> @@ -67,36 +70,19 @@ void mgag200_crtc_set_gamma_linear(struct mga_device *mdev,
> }
> }
>
> -void mgag200_crtc_set_gamma(struct mga_device *mdev,
> +void mgag200_crtc_load_gamma(struct mga_device *mdev,
> const struct drm_format_info *format,
> struct drm_color_lut *lut)
> {
> - int i;
> -
> - WREG8(DAC_INDEX + MGA1064_INDEX, 0);
> + struct drm_crtc *crtc = &mdev->crtc;
>
> switch (format->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);
> - }
> - /* 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);
> - }
> + drm_crtc_load_gamma_565_from_888(crtc, lut, mgag200_set_gamma_lut);
> 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);
> - }
> + drm_crtc_load_gamma_888(crtc, lut, mgag200_set_gamma_lut);
> break;
> default:
> drm_warn_once(&mdev->base, "Unsupported format %p4cc for gamma correction\n",
> @@ -642,9 +628,9 @@ void mgag200_crtc_helper_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_s
> const struct drm_format_info *format = mgag200_crtc_state->format;
>
> if (crtc_state->gamma_lut)
> - mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
> + mgag200_crtc_load_gamma(mdev, format, crtc_state->gamma_lut->data);
> else
> - mgag200_crtc_set_gamma_linear(mdev, format);
> + mgag200_crtc_fill_gamma(mdev, format);
> }
> }
>
> @@ -665,9 +651,9 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
> funcs->pixpllc_atomic_update(crtc, old_state);
>
> if (crtc_state->gamma_lut)
> - mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
> + mgag200_crtc_load_gamma(mdev, format, crtc_state->gamma_lut->data);
> else
> - mgag200_crtc_set_gamma_linear(mdev, format);
> + mgag200_crtc_fill_gamma(mdev, format);
>
> mgag200_enable_display(mdev);
> }
next prev parent reply other threads:[~2025-05-13 12:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-09 8:23 [PATCH 0/5] drm: Provide helpers for programming gamma ramps and palettes Thomas Zimmermann
2025-05-09 8:23 ` [PATCH 1/5] drm: Add helpers for programming hardware gamma LUTs Thomas Zimmermann
2025-05-12 13:31 ` Jocelyn Falempe
2025-05-12 13:55 ` Thomas Zimmermann
2025-05-13 12:26 ` Jocelyn Falempe
2025-05-19 13:06 ` Javier Martinez Canillas
2025-05-09 8:23 ` [PATCH 2/5] drm/ast: Use helpers for programming gamma ramps and palettes Thomas Zimmermann
2025-05-13 12:26 ` Jocelyn Falempe
2025-05-09 8:23 ` [PATCH 3/5] drm/mgag200: Use helpers for programming gamma ramps Thomas Zimmermann
2025-05-13 12:27 ` Jocelyn Falempe [this message]
2025-05-09 8:23 ` [PATCH 4/5] drm/ofdrm: " Thomas Zimmermann
2025-05-19 13:16 ` Javier Martinez Canillas
2025-05-09 8:23 ` [PATCH 5/5] drm/vesadrm: " Thomas Zimmermann
2025-05-19 13:17 ` Javier Martinez Canillas
2025-05-12 11:48 ` [PATCH 0/5] drm: Provide helpers for programming gamma ramps and palettes Thomas Zimmermann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66c4553e-17ec-428e-8a3b-73a51af65cc1@redhat.com \
--to=jfalempe@redhat.com \
--cc=airlied@gmail.com \
--cc=airlied@redhat.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=javierm@redhat.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=simona@ffwll.ch \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).