linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/msm/dpu: add support for alpha blending properties
@ 2021-06-28 19:19 Dmitry Baryshkov
  2021-08-17 17:48 ` [Freedreno] " abhinavk
  0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Baryshkov @ 2021-06-28 19:19 UTC (permalink / raw)
  To: Bjorn Andersson, Rob Clark, Sean Paul, Abhinav Kumar
  Cc: Jonathan Marek, Stephen Boyd, David Airlie, Daniel Vetter,
	linux-arm-msm, dri-devel, freedreno

Add support for alpha blending properties. Setup the plane blend state
according to those properties.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 43 ++++++++++++++++-------
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 ++++--
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 9a5c70c87cc8..768012243b44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -30,12 +30,6 @@
 #include "dpu_core_perf.h"
 #include "dpu_trace.h"
 
-#define DPU_DRM_BLEND_OP_NOT_DEFINED    0
-#define DPU_DRM_BLEND_OP_OPAQUE         1
-#define DPU_DRM_BLEND_OP_PREMULTIPLIED  2
-#define DPU_DRM_BLEND_OP_COVERAGE       3
-#define DPU_DRM_BLEND_OP_MAX            4
-
 /* layer mixer index on dpu_crtc */
 #define LEFT_MIXER 0
 #define RIGHT_MIXER 1
@@ -146,20 +140,43 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
 {
 	struct dpu_hw_mixer *lm = mixer->hw_lm;
 	uint32_t blend_op;
+	uint32_t fg_alpha, bg_alpha;
 
-	/* default to opaque blending */
-	blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
-		DPU_BLEND_BG_ALPHA_BG_CONST;
+	fg_alpha = pstate->base.alpha >> 8;
+	bg_alpha = 0xff - fg_alpha;
 
-	if (format->alpha_enable) {
+	/* default to opaque blending */
+	if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
+	    !format->alpha_enable) {
+		blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
+			DPU_BLEND_BG_ALPHA_BG_CONST;
+	} else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) {
+		blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
+			DPU_BLEND_BG_ALPHA_FG_PIXEL;
+		if (fg_alpha != 0xff) {
+			bg_alpha = fg_alpha;
+			blend_op |= DPU_BLEND_BG_MOD_ALPHA |
+				    DPU_BLEND_BG_INV_MOD_ALPHA;
+		} else {
+			blend_op |= DPU_BLEND_BG_INV_ALPHA;
+		}
+	} else {
 		/* coverage blending */
 		blend_op = DPU_BLEND_FG_ALPHA_FG_PIXEL |
-			DPU_BLEND_BG_ALPHA_FG_PIXEL |
-			DPU_BLEND_BG_INV_ALPHA;
+			DPU_BLEND_BG_ALPHA_FG_PIXEL;
+		if (fg_alpha != 0xff) {
+			bg_alpha = fg_alpha;
+			blend_op |= DPU_BLEND_FG_MOD_ALPHA |
+				    DPU_BLEND_FG_INV_MOD_ALPHA |
+				    DPU_BLEND_BG_MOD_ALPHA |
+				    DPU_BLEND_BG_INV_MOD_ALPHA;
+		} else {
+			blend_op |= DPU_BLEND_BG_INV_ALPHA;
+		}
 	}
 
 	lm->ops.setup_blend_config(lm, pstate->stage,
-				0xFF, 0, blend_op);
+				fg_alpha, bg_alpha, blend_op);
 
 	DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
 		  &format->base.pixel_format, format->alpha_enable, blend_op);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ec4a6f04394a..c989621209aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1339,9 +1339,7 @@ static void dpu_plane_reset(struct drm_plane *plane)
 		return;
 	}
 
-	pstate->base.plane = plane;
-
-	plane->state = &pstate->base;
+	__drm_atomic_helper_plane_reset(plane, &pstate->base);
 }
 
 #ifdef CONFIG_DEBUG_FS
@@ -1647,6 +1645,12 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
 	if (ret)
 		DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
 
+	drm_plane_create_alpha_property(plane);
+	drm_plane_create_blend_mode_property(plane,
+			BIT(DRM_MODE_BLEND_PIXEL_NONE) |
+			BIT(DRM_MODE_BLEND_PREMULTI) |
+			BIT(DRM_MODE_BLEND_COVERAGE));
+
 	drm_plane_create_rotation_property(plane,
 			DRM_MODE_ROTATE_0,
 			DRM_MODE_ROTATE_0 |
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dpu: add support for alpha blending properties
  2021-06-28 19:19 [PATCH] drm/msm/dpu: add support for alpha blending properties Dmitry Baryshkov
@ 2021-08-17 17:48 ` abhinavk
  2021-08-18 19:34   ` Dmitry Baryshkov
  0 siblings, 1 reply; 3+ messages in thread
From: abhinavk @ 2021-08-17 17:48 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Bjorn Andersson, Rob Clark, Sean Paul, Jonathan Marek,
	Stephen Boyd, linux-arm-msm, dri-devel, David Airlie,
	Daniel Vetter, freedreno

On 2021-06-28 12:19, Dmitry Baryshkov wrote:
> Add support for alpha blending properties. Setup the plane blend state
> according to those properties.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I think this has already been picked up by Rob but just had a couple of 
comments
below.

Also, how has this been validated? On RB boards i dont think all the 
paths get
executed.

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 43 ++++++++++++++++-------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 ++++--
>  2 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 9a5c70c87cc8..768012243b44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -30,12 +30,6 @@
>  #include "dpu_core_perf.h"
>  #include "dpu_trace.h"
> 
> -#define DPU_DRM_BLEND_OP_NOT_DEFINED    0
> -#define DPU_DRM_BLEND_OP_OPAQUE         1
> -#define DPU_DRM_BLEND_OP_PREMULTIPLIED  2
> -#define DPU_DRM_BLEND_OP_COVERAGE       3
> -#define DPU_DRM_BLEND_OP_MAX            4
> -
>  /* layer mixer index on dpu_crtc */
>  #define LEFT_MIXER 0
>  #define RIGHT_MIXER 1
> @@ -146,20 +140,43 @@ static void _dpu_crtc_setup_blend_cfg(struct
> dpu_crtc_mixer *mixer,
>  {
>  	struct dpu_hw_mixer *lm = mixer->hw_lm;
>  	uint32_t blend_op;
> +	uint32_t fg_alpha, bg_alpha;
> 
> -	/* default to opaque blending */
> -	blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
> -		DPU_BLEND_BG_ALPHA_BG_CONST;
> +	fg_alpha = pstate->base.alpha >> 8;
> +	bg_alpha = 0xff - fg_alpha;
> 
> -	if (format->alpha_enable) {
> +	/* default to opaque blending */
> +	if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
> +	    !format->alpha_enable) {
> +		blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
> +			DPU_BLEND_BG_ALPHA_BG_CONST;
> +	} else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) 
> {
> +		blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
> +			DPU_BLEND_BG_ALPHA_FG_PIXEL;
> +		if (fg_alpha != 0xff) {
> +			bg_alpha = fg_alpha;
> +			blend_op |= DPU_BLEND_BG_MOD_ALPHA |
> +				    DPU_BLEND_BG_INV_MOD_ALPHA;
> +		} else {
> +			blend_op |= DPU_BLEND_BG_INV_ALPHA;
> +		}
> +	} else {
>  		/* coverage blending */
>  		blend_op = DPU_BLEND_FG_ALPHA_FG_PIXEL |
> -			DPU_BLEND_BG_ALPHA_FG_PIXEL |
> -			DPU_BLEND_BG_INV_ALPHA;
> +			DPU_BLEND_BG_ALPHA_FG_PIXEL;
> +		if (fg_alpha != 0xff) {
> +			bg_alpha = fg_alpha;
> +			blend_op |= DPU_BLEND_FG_MOD_ALPHA |
> +				    DPU_BLEND_FG_INV_MOD_ALPHA |
comparing this with the blend rule downstream, is this inversion 
necessary?
I only see below rule downstream:

628 			if (fg_alpha != 0xff) {
629 				bg_alpha = fg_alpha;
630 				blend_op |= SDE_BLEND_FG_MOD_ALPHA |
631 					SDE_BLEND_BG_MOD_ALPHA |
632 					SDE_BLEND_BG_INV_MOD_ALPHA;

> +				    DPU_BLEND_BG_MOD_ALPHA |
> +				    DPU_BLEND_BG_INV_MOD_ALPHA;
> +		} else {
> +			blend_op |= DPU_BLEND_BG_INV_ALPHA;
> +		}
>  	}
> 
>  	lm->ops.setup_blend_config(lm, pstate->stage,
> -				0xFF, 0, blend_op);
> +				fg_alpha, bg_alpha, blend_op);
> 
>  	DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
>  		  &format->base.pixel_format, format->alpha_enable, blend_op);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index ec4a6f04394a..c989621209aa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1339,9 +1339,7 @@ static void dpu_plane_reset(struct drm_plane 
> *plane)
>  		return;
>  	}
> 
> -	pstate->base.plane = plane;
> -
> -	plane->state = &pstate->base;
> +	__drm_atomic_helper_plane_reset(plane, &pstate->base);
>  }
> 
>  #ifdef CONFIG_DEBUG_FS
> @@ -1647,6 +1645,12 @@ struct drm_plane *dpu_plane_init(struct 
> drm_device *dev,
>  	if (ret)
>  		DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
> 
> +	drm_plane_create_alpha_property(plane);
> +	drm_plane_create_blend_mode_property(plane,
> +			BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> +			BIT(DRM_MODE_BLEND_PREMULTI) |
> +			BIT(DRM_MODE_BLEND_COVERAGE));
> +
>  	drm_plane_create_rotation_property(plane,
>  			DRM_MODE_ROTATE_0,
>  			DRM_MODE_ROTATE_0 |

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dpu: add support for alpha blending properties
  2021-08-17 17:48 ` [Freedreno] " abhinavk
@ 2021-08-18 19:34   ` Dmitry Baryshkov
  0 siblings, 0 replies; 3+ messages in thread
From: Dmitry Baryshkov @ 2021-08-18 19:34 UTC (permalink / raw)
  To: abhinavk
  Cc: Bjorn Andersson, Rob Clark, Sean Paul, Jonathan Marek,
	Stephen Boyd, linux-arm-msm, dri-devel, David Airlie,
	Daniel Vetter, freedreno

On 17/08/2021 20:48, abhinavk@codeaurora.org wrote:
> On 2021-06-28 12:19, Dmitry Baryshkov wrote:
>> Add support for alpha blending properties. Setup the plane blend state
>> according to those properties.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I think this has already been picked up by Rob but just had a couple of 
> comments
> below.
> 
> Also, how has this been validated? On RB boards i dont think all the 
> paths get
> executed.

I've used modetest to set pixel blending properties. The results looked 
logical from my point of view.

> 
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 43 ++++++++++++++++-------
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 ++++--
>>  2 files changed, 37 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 9a5c70c87cc8..768012243b44 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -30,12 +30,6 @@
>>  #include "dpu_core_perf.h"
>>  #include "dpu_trace.h"
>>
>> -#define DPU_DRM_BLEND_OP_NOT_DEFINED    0
>> -#define DPU_DRM_BLEND_OP_OPAQUE         1
>> -#define DPU_DRM_BLEND_OP_PREMULTIPLIED  2
>> -#define DPU_DRM_BLEND_OP_COVERAGE       3
>> -#define DPU_DRM_BLEND_OP_MAX            4
>> -
>>  /* layer mixer index on dpu_crtc */
>>  #define LEFT_MIXER 0
>>  #define RIGHT_MIXER 1
>> @@ -146,20 +140,43 @@ static void _dpu_crtc_setup_blend_cfg(struct
>> dpu_crtc_mixer *mixer,
>>  {
>>      struct dpu_hw_mixer *lm = mixer->hw_lm;
>>      uint32_t blend_op;
>> +    uint32_t fg_alpha, bg_alpha;
>>
>> -    /* default to opaque blending */
>> -    blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
>> -        DPU_BLEND_BG_ALPHA_BG_CONST;
>> +    fg_alpha = pstate->base.alpha >> 8;
>> +    bg_alpha = 0xff - fg_alpha;
>>
>> -    if (format->alpha_enable) {
>> +    /* default to opaque blending */
>> +    if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
>> +        !format->alpha_enable) {
>> +        blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
>> +            DPU_BLEND_BG_ALPHA_BG_CONST;
>> +    } else if (pstate->base.pixel_blend_mode == 
>> DRM_MODE_BLEND_PREMULTI) {
>> +        blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
>> +            DPU_BLEND_BG_ALPHA_FG_PIXEL;
>> +        if (fg_alpha != 0xff) {
>> +            bg_alpha = fg_alpha;
>> +            blend_op |= DPU_BLEND_BG_MOD_ALPHA |
>> +                    DPU_BLEND_BG_INV_MOD_ALPHA;
>> +        } else {
>> +            blend_op |= DPU_BLEND_BG_INV_ALPHA;
>> +        }
>> +    } else {
>>          /* coverage blending */
>>          blend_op = DPU_BLEND_FG_ALPHA_FG_PIXEL |
>> -            DPU_BLEND_BG_ALPHA_FG_PIXEL |
>> -            DPU_BLEND_BG_INV_ALPHA;
>> +            DPU_BLEND_BG_ALPHA_FG_PIXEL;
>> +        if (fg_alpha != 0xff) {
>> +            bg_alpha = fg_alpha;
>> +            blend_op |= DPU_BLEND_FG_MOD_ALPHA |
>> +                    DPU_BLEND_FG_INV_MOD_ALPHA |
> comparing this with the blend rule downstream, is this inversion necessary?
> I only see below rule downstream:
> 
> 628             if (fg_alpha != 0xff) {
> 629                 bg_alpha = fg_alpha;
> 630                 blend_op |= SDE_BLEND_FG_MOD_ALPHA |
> 631                     SDE_BLEND_BG_MOD_ALPHA |
> 632                     SDE_BLEND_BG_INV_MOD_ALPHA;

I've also stumbled upon this for quite some time. If you check old 
kernel trees, you'll see that up to 4.9 there was an inversion. But 
during the import to 4.14 this line was silently removed. I suspect that 
it got lost because of some mistake during the import.

The same code (with the inversion) was present in the mdp5 driver.

Could you please check against the manual, how these bits should work?
See 
https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties 
for the expected formulas.

> 
>> +                    DPU_BLEND_BG_MOD_ALPHA |
>> +                    DPU_BLEND_BG_INV_MOD_ALPHA;
>> +        } else {
>> +            blend_op |= DPU_BLEND_BG_INV_ALPHA;
>> +        }
>>      }
>>
>>      lm->ops.setup_blend_config(lm, pstate->stage,
>> -                0xFF, 0, blend_op);
>> +                fg_alpha, bg_alpha, blend_op);
>>
>>      DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
>>            &format->base.pixel_format, format->alpha_enable, blend_op);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index ec4a6f04394a..c989621209aa 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -1339,9 +1339,7 @@ static void dpu_plane_reset(struct drm_plane 
>> *plane)
>>          return;
>>      }
>>
>> -    pstate->base.plane = plane;
>> -
>> -    plane->state = &pstate->base;
>> +    __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>  }
>>
>>  #ifdef CONFIG_DEBUG_FS
>> @@ -1647,6 +1645,12 @@ struct drm_plane *dpu_plane_init(struct 
>> drm_device *dev,
>>      if (ret)
>>          DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>>
>> +    drm_plane_create_alpha_property(plane);
>> +    drm_plane_create_blend_mode_property(plane,
>> +            BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +            BIT(DRM_MODE_BLEND_PREMULTI) |
>> +            BIT(DRM_MODE_BLEND_COVERAGE));
>> +
>>      drm_plane_create_rotation_property(plane,
>>              DRM_MODE_ROTATE_0,
>>              DRM_MODE_ROTATE_0 |


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-18 19:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28 19:19 [PATCH] drm/msm/dpu: add support for alpha blending properties Dmitry Baryshkov
2021-08-17 17:48 ` [Freedreno] " abhinavk
2021-08-18 19:34   ` Dmitry Baryshkov

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).