[PATCH v3] drm/ast: Add Atomic gamma lut support for aspeed
Jocelyn Falempe
jfalempe at redhat.com
Fri Sep 30 08:56:53 UTC 2022
On 30/09/2022 09:55, Thomas Zimmermann wrote:
> 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.
No problem, without proper hardware to test, and good documentation,
it's hard to guess right what the hardware will do at first attempt.
>
> 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.
done
>
>> + 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.
done
>
>> + 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.
done
>
>> + 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'.
done
>
>> + 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.
ok, I've done this in v4.
I thought the color_mgmt_changed would cover this, but now in v4 it does.
Thanks a lot for the review and tests, I will send the v4 soon.
--
Jocelyn
>
> 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;
>
More information about the dri-devel
mailing list