[PATCH] drm/ast: Reload gamma LUT after changing primary plane's color format

[PATCH] drm/ast: Reload gamma LUT after changing primary plane's color format

[Thomas Zimmermann]

The gamma LUT has to be reloaded after changing the primary plane's
color format. This used to be done implicitly by the CRTC atomic_enable()
helper after updating the primary plane. With the recent reordering of
the steps, the primary plane's setup was moved last and invalidated
the gamma LUT. Fix this by setting the LUT from within atomic_flush().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 2f0ddd89fe32 ("drm/ast: Enable CRTC before planes")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 834a156e3a75..ba3bf76e104d 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -742,7 +742,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
 	case DRM_MODE_DPMS_SUSPEND:
 		if (ast->tx_chip_type == AST_TX_DP501)
 			ast_set_dp501_video_output(crtc->dev, 1);
-		ast_crtc_load_lut(ast, crtc);
 		break;
 	case DRM_MODE_DPMS_OFF:
 		if (ast->tx_chip_type == AST_TX_DP501)
@@ -777,6 +776,17 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	return 0;
 }
 
+static void
+ast_crtc_helper_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state)
+{
+	struct ast_private *ast = to_ast_private(crtc->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);
+
+	if (old_ast_crtc_state->format != ast_crtc_state->format)
+		ast_crtc_load_lut(ast, crtc);
+}
+
 static void
 ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 			      struct drm_crtc_state *old_crtc_state)
@@ -830,6 +840,7 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
 
 static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
 	.atomic_check = ast_crtc_helper_atomic_check,
+	.atomic_flush = ast_crtc_helper_atomic_flush,
 	.atomic_enable = ast_crtc_helper_atomic_enable,
 	.atomic_disable = ast_crtc_helper_atomic_disable,
 };
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Reload gamma LUT after changing primary plane's color format

[Daniel Vetter]

On Tue, Sep 22, 2020 at 04:46:55PM +0200, Thomas Zimmermann wrote:
> The gamma LUT has to be reloaded after changing the primary plane's
> color format. This used to be done implicitly by the CRTC atomic_enable()
> helper after updating the primary plane. With the recent reordering of
> the steps, the primary plane's setup was moved last and invalidated
> the gamma LUT. Fix this by setting the LUT from within atomic_flush().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 2f0ddd89fe32 ("drm/ast: Enable CRTC before planes")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org

Does what it says in the commit message, and makes sense.

Maybe add a comment to the load_lut function or where it's called stating
that this must be done after every plane color change, seems like an
important piece of information to carry around in the code itself and not
just in the commit message.

Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 834a156e3a75..ba3bf76e104d 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -742,7 +742,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
>  	case DRM_MODE_DPMS_SUSPEND:
>  		if (ast->tx_chip_type == AST_TX_DP501)
>  			ast_set_dp501_video_output(crtc->dev, 1);
> -		ast_crtc_load_lut(ast, crtc);
>  		break;
>  	case DRM_MODE_DPMS_OFF:
>  		if (ast->tx_chip_type == AST_TX_DP501)
> @@ -777,6 +776,17 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> +static void
> +ast_crtc_helper_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state)
> +{
> +	struct ast_private *ast = to_ast_private(crtc->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);
> +
> +	if (old_ast_crtc_state->format != ast_crtc_state->format)
> +		ast_crtc_load_lut(ast, crtc);
> +}
> +
>  static void
>  ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>  			      struct drm_crtc_state *old_crtc_state)
> @@ -830,6 +840,7 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>  
>  static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
>  	.atomic_check = ast_crtc_helper_atomic_check,
> +	.atomic_flush = ast_crtc_helper_atomic_flush,
>  	.atomic_enable = ast_crtc_helper_atomic_enable,
>  	.atomic_disable = ast_crtc_helper_atomic_disable,
>  };
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Reload gamma LUT after changing primary plane's color format

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3259 bytes --]

Hi

Am 23.09.20 um 17:30 schrieb Daniel Vetter:
> On Tue, Sep 22, 2020 at 04:46:55PM +0200, Thomas Zimmermann wrote:
>> The gamma LUT has to be reloaded after changing the primary plane's
>> color format. This used to be done implicitly by the CRTC atomic_enable()
>> helper after updating the primary plane. With the recent reordering of
>> the steps, the primary plane's setup was moved last and invalidated
>> the gamma LUT. Fix this by setting the LUT from within atomic_flush().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 2f0ddd89fe32 ("drm/ast: Enable CRTC before planes")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
> 
> Does what it says in the commit message, and makes sense.
> 
> Maybe add a comment to the load_lut function or where it's called stating
> that this must be done after every plane color change, seems like an
> important piece of information to carry around in the code itself and not
> just in the commit message.

I'll do that before committing the patch.

> 
> Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks

Best regards
Thomas

> 
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 834a156e3a75..ba3bf76e104d 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -742,7 +742,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
>>  	case DRM_MODE_DPMS_SUSPEND:
>>  		if (ast->tx_chip_type == AST_TX_DP501)
>>  			ast_set_dp501_video_output(crtc->dev, 1);
>> -		ast_crtc_load_lut(ast, crtc);
>>  		break;
>>  	case DRM_MODE_DPMS_OFF:
>>  		if (ast->tx_chip_type == AST_TX_DP501)
>> @@ -777,6 +776,17 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
>>  	return 0;
>>  }
>>  
>> +static void
>> +ast_crtc_helper_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state)
>> +{
>> +	struct ast_private *ast = to_ast_private(crtc->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);
>> +
>> +	if (old_ast_crtc_state->format != ast_crtc_state->format)
>> +		ast_crtc_load_lut(ast, crtc);
>> +}
>> +
>>  static void
>>  ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>>  			      struct drm_crtc_state *old_crtc_state)
>> @@ -830,6 +840,7 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>>  
>>  static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
>>  	.atomic_check = ast_crtc_helper_atomic_check,
>> +	.atomic_flush = ast_crtc_helper_atomic_flush,
>>  	.atomic_enable = ast_crtc_helper_atomic_enable,
>>  	.atomic_disable = ast_crtc_helper_atomic_disable,
>>  };
>> -- 
>> 2.28.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: Felix Imendörffer


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel