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@xxxxxxxxxx>
Tested-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
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-Portif (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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
