[PATCH v3] drm/ast: Add Atomic gamma lut support for aspeed
Thomas Zimmermann
tzimmermann at suse.de
Fri Sep 30 07:55:32 UTC 2022
Hi,
I did more testing and you were correct in the original patch. Even in
16-bit mode, the gamma LUT needs 256 entries per color. Apparently the
ast HW expands the RGB565 components into RGB888 internally before doing
the gamma lookup.
I added another review below. I'm sorry for the incorrect reply earlier.
Am 30.09.22 um 08:54 schrieb Jocelyn Falempe:
> The current ast driver only supports legacy gamma interface.
> This also fixes a Gnome3/Wayland error which incorrectly adds
> gamma to atomic commit:
> "Page flip discarded: CRTC property (GAMMA_LUT) not found"
>
> I only tested remotely, so I wasn't able to check that it had
> an effect on the VGA output. But when activating "Night Light"
> in Gnome, ast_crtc_load_lut() is called.
>
> v2: use the same functions as mgag200.
> handle 16bits color mode.
>
> v3: Check gamma_lut size in atomic check.
>
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> Tested-by: Thomas Zimmermann <tzimmermann at suse.de>
> Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
Please keep my tags.
> ---
> drivers/gpu/drm/ast/ast_mode.c | 103 +++++++++++++++++++++++++++------
> 1 file changed, 86 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 214b10178454..874b356ce37a 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -49,6 +49,8 @@
> #include "ast_drv.h"
> #include "ast_tables.h"
>
> +#define AST_LUT_SIZE 256
> +
> static inline void ast_load_palette_index(struct ast_private *ast,
> u8 index, u8 red, u8 green,
> u8 blue)
> @@ -63,20 +65,71 @@ static inline void ast_load_palette_index(struct ast_private *ast,
> ast_io_read8(ast, AST_IO_SEQ_PORT);
> }
>
> -static void ast_crtc_load_lut(struct ast_private *ast, struct drm_crtc *crtc)
> +static void ast_crtc_set_gamma_linear(struct ast_private *ast,
> + const struct drm_format_info *format)
> {
> - u16 *r, *g, *b;
> int i;
>
> - if (!crtc->enabled)
> - return;
> + switch (format->format) {
> + case DRM_FORMAT_RGB565:
> + /* Use better interpolation, to take 32 values from 0 to 255 */
> + for (i = 0; i < AST_LUT_SIZE / 8; i++)
> + ast_load_palette_index(ast,
> + i,
> + i * 8 + i / 4,
> + i * 4 + i / 16,
> + i * 8 + i / 4);
> + /* Green has one more bit, so add padding with 0 for red and blue. */
> + for (i = AST_LUT_SIZE / 8; i < AST_LUT_SIZE / 4; i++)
> + ast_load_palette_index(ast, i, 0, i * 4 + i / 16, 0);
> + break;
This is the code that does not work with ast. I'd like to keep the
switch statement, so simply remove the code and let it fall through as
for the other cases.
> + case DRM_FORMAT_C8: /* In this case, gamma table is used as color palette */
> + case DRM_FORMAT_RGB888:
RGBG888 can be removed. We never support this anywhere.
> + case DRM_FORMAT_XRGB8888:
> + for (i = 0; i < AST_LUT_SIZE; i++)
> + ast_load_palette_index(ast, i, i, i, i);
> + break;
> + default:
> + drm_warn_once(&ast->base, "Unsupported format %p4cc for gamma correction\n",
> + &format->format);
> + break;
> + }
> +}
>
> - r = crtc->gamma_store;
> - g = r + crtc->gamma_size;
> - b = g + crtc->gamma_size;
> +static void ast_crtc_set_gamma(struct ast_private *ast,
> + const struct drm_format_info *format,
> + struct drm_color_lut *lut)
> +{
> + int i;
>
> - for (i = 0; i < 256; i++)
> - ast_load_palette_index(ast, i, *r++ >> 8, *g++ >> 8, *b++ >> 8);
> + switch (format->format) {
> + case DRM_FORMAT_RGB565:
> + /* Use better interpolation, to take 32 values from lut[0] to lut[255] */
> + for (i = 0; i < AST_LUT_SIZE / 8; i++)
> + ast_load_palette_index(ast,
> + i,
> + lut[i * 8 + i / 4].red >> 8,
> + lut[i * 4 + i / 16].green >> 8,
> + lut[i * 8 + i / 4].blue >> 8);
> + /* Green has one more bit, so add padding with 0 for red and blue. */
> + for (i = AST_LUT_SIZE / 8; i < AST_LUT_SIZE / 4; i++)
> + ast_load_palette_index(ast, i, 0, lut[i * 4 + i / 16].green >> 8, 0);
> + break;
Same as above.
> + case DRM_FORMAT_C8: /* In this case, gamma table is used as color palette */
> + case DRM_FORMAT_RGB888:
Same as above.
> + case DRM_FORMAT_XRGB8888:
> + for (i = 0; i < AST_LUT_SIZE; i++)
> + ast_load_palette_index(ast,
> + i,
I think 'i' can go on the previous line after 'ast'.
> + lut[i].red >> 8,
> + lut[i].green >> 8,
> + lut[i].blue >> 8);
> + break;
> + default:
> + drm_warn_once(&ast->base, "Unsupported format %p4cc for gamma correction\n",
> + &format->format);
> + break;
> + }
> }
>
> static bool ast_get_vbios_mode_info(const struct drm_format_info *format,
> @@ -1018,9 +1071,11 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
>
> ast_set_color_reg(ast, format);
> ast_set_vbios_color_reg(ast, format, vbios_mode_info);
> + if (crtc->state->gamma_lut)
> + ast_crtc_set_gamma(ast, format, crtc->state->gamma_lut->data);
> + else
> + ast_crtc_set_gamma_linear(ast, format);
> }
> -
> - ast_crtc_load_lut(ast, crtc);
> break;
> case DRM_MODE_DPMS_STANDBY:
> case DRM_MODE_DPMS_SUSPEND:
> @@ -1128,6 +1183,15 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
> if (drm_WARN_ON_ONCE(dev, !format))
> return -EINVAL; /* BUG: We didn't set format in primary check(). */
>
There should be a format test right here before testing
color_mgmt_changed. See my next comment below.
> + if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
> + if (crtc_state->gamma_lut->length !=
> + AST_LUT_SIZE * sizeof(struct drm_color_lut)) {
> + drm_err(dev, "Wrong size for gamma_lut %zu\n",
> + crtc_state->gamma_lut->length);
> + return -EINVAL;
> + }
> + }
> +
> succ = ast_get_vbios_mode_info(format, &crtc_state->mode,
> &crtc_state->adjusted_mode,
> &ast_state->vbios_mode_info);
> @@ -1158,20 +1222,23 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> {
> struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> crtc);
> - struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
> - crtc);
> struct drm_device *dev = crtc->dev;
> struct ast_private *ast = to_ast_private(dev);
> struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
> - struct ast_crtc_state *old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
> struct ast_vbios_mode_info *vbios_mode_info = &ast_crtc_state->vbios_mode_info;
>
> /*
> * The gamma LUT has to be reloaded after changing the primary
> * plane's color format.
> */
> - if (old_ast_crtc_state->format != ast_crtc_state->format)
> - ast_crtc_load_lut(ast, crtc);
After reading this chunk again, I'm not so sure that this test can be
removed easily. We once had a bug where colors got randomized after
setting display modes. It turned out that changing the plane's color
format invalidated the gamma LUT. Hence, we test this here and reload
the LUT if the format changed. Commit 8e3784dfef8a ("drm/ast: Reload
gamma LUT after changing primary plane's color format") if you're curious.
It's a bit odd to do this test in atomic_flush, but with the new color
MGMT we can improve this as well. The test and comment should be moved
into ast_crtc_helper_atomic_check() at the location I marked above. If
the format changes, simply set the color_mgmt_changed flag to trigger
reloading the LUT. Like this:
/*
* The gamma LUT has to be reloaded after changing the primary
* plane's color format.
*/
if (old_ast_crtc_state->format != ast_crtc_state->format)
crtc_state->color_mgmt_changed = true;
Your code in atomic_flush will then do the right thing.
Best regards
Thomas
> + if (crtc_state->enable && crtc_state->color_mgmt_changed) {
> + if (crtc_state->gamma_lut)
> + ast_crtc_set_gamma(ast,
> + ast_crtc_state->format,
> + crtc_state->gamma_lut->data);
> + else
> + ast_crtc_set_gamma_linear(ast, ast_crtc_state->format);
> + }
>
> //Set Aspeed Display-Port
> if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
> @@ -1309,7 +1376,9 @@ static int ast_crtc_init(struct drm_device *dev)
> if (ret)
> return ret;
>
> - drm_mode_crtc_set_gamma_size(crtc, 256);
> + drm_mode_crtc_set_gamma_size(crtc, AST_LUT_SIZE);
> + drm_crtc_enable_color_mgmt(crtc, 0, false, AST_LUT_SIZE);
> +
> drm_crtc_helper_add(crtc, &ast_crtc_helper_funcs);
>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220930/137de8e2/attachment.sig>
More information about the dri-devel
mailing list