diff mbox series

[RFC,v4,7/7] drm/msm/dpu: Use DRM solid_fill property

Message ID 20230404-solid-fill-v4-7-f4ec0caa742d@quicinc.com
State Superseded
Headers show
Series Support for Solid Fill Planes | expand

Commit Message

Jessica Zhang June 30, 2023, 12:25 a.m. UTC
Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
determine if the plane is solid fill. In addition drop the DPU plane
color_fill field as we can now use drm_plane_state.solid_fill instead,
and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
allow userspace to configure the alpha value for the solid fill color.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Dmitry Baryshkov June 30, 2023, 12:59 a.m. UTC | #1
On 30/06/2023 03:25, Jessica Zhang wrote:
> Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
> determine if the plane is solid fill. In addition drop the DPU plane
> color_fill field as we can now use drm_plane_state.solid_fill instead,
> and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
> allow userspace to configure the alpha value for the solid fill color.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Minor suggestion below.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 4476722f03bb..11d4fb771a1f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -42,7 +42,6 @@
>   #define SHARP_SMOOTH_THR_DEFAULT	8
>   #define SHARP_NOISE_THR_DEFAULT	2
>   
> -#define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
>   #define DPU_ZPOS_MAX 255
>   
>   /*
> @@ -82,7 +81,6 @@ struct dpu_plane {
>   
>   	enum dpu_sspp pipe;
>   
> -	uint32_t color_fill;
>   	bool is_error;
>   	bool is_rt_pipe;
>   	const struct dpu_mdss_cfg *catalog;
> @@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
>   	_dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation);
>   }
>   
> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)

Please consider accepting drm_plane_state instead and handling alpha 
here. Then _dpu_color_fill can accept rgba colour instead of separate 
RGB and alpha values.

> +{
> +	uint32_t ret = 0;
> +
> +	ret |= ((uint8_t) solid_fill.b) << 16;
> +	ret |= ((uint8_t) solid_fill.g) << 8;
> +	ret |= ((uint8_t) solid_fill.r);
> +
> +	return ret;
> +}
> +
>   /**
>    * _dpu_plane_color_fill - enables color fill on plane
>    * @pdpu:   Pointer to DPU plane object
> @@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane)
>   	if (pdpu->is_error)
>   		/* force white frame with 100% alpha pipe output on error */
>   		_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
> -	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
> -		/* force 100% alpha */
> -		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> +	else if (drm_plane_solid_fill_enabled(plane->state))
> +		_dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),
> +				plane->state->alpha);
>   	else {
>   		dpu_plane_flush_csc(pdpu, &pstate->pipe);
>   		dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
> @@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
>   	}
>   
>   	/* override for color fill */
> -	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
> +	if (drm_plane_solid_fill_enabled(plane->state)) {
>   		_dpu_plane_set_qos_ctrl(plane, pipe, false);
>   
>   		/* skip remaining processing on color fill */
>
Pekka Paalanen June 30, 2023, 8:26 a.m. UTC | #2
On Thu, 29 Jun 2023 17:25:06 -0700
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
> determine if the plane is solid fill. In addition drop the DPU plane
> color_fill field as we can now use drm_plane_state.solid_fill instead,
> and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
> allow userspace to configure the alpha value for the solid fill color.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 4476722f03bb..11d4fb771a1f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -42,7 +42,6 @@
>  #define SHARP_SMOOTH_THR_DEFAULT	8
>  #define SHARP_NOISE_THR_DEFAULT	2
>  
> -#define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
>  #define DPU_ZPOS_MAX 255
>  
>  /*
> @@ -82,7 +81,6 @@ struct dpu_plane {
>  
>  	enum dpu_sspp pipe;
>  
> -	uint32_t color_fill;
>  	bool is_error;
>  	bool is_rt_pipe;
>  	const struct dpu_mdss_cfg *catalog;
> @@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
>  	_dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation);
>  }
>  
> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)
> +{
> +	uint32_t ret = 0;
> +
> +	ret |= ((uint8_t) solid_fill.b) << 16;
> +	ret |= ((uint8_t) solid_fill.g) << 8;
> +	ret |= ((uint8_t) solid_fill.r);

solid_fill.r, g and b are uint32_t, yes?

What's the value encoding in the UAPI? That doc was missing.

I wouldn't expect the UAPI to use 32-bit variables if it was
essentially 8-bit, so this conversion looks wrong.

Nominal color value 1.0 in u8 is 0xff. The same in u32 is probably
0xffffffff? So a simple cast to u8 won't work. You'd want to take the
upper 8 bits instead.


Thanks,
pq

> +
> +	return ret;
> +}
> +
>  /**
>   * _dpu_plane_color_fill - enables color fill on plane
>   * @pdpu:   Pointer to DPU plane object
> @@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane)
>  	if (pdpu->is_error)
>  		/* force white frame with 100% alpha pipe output on error */
>  		_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
> -	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
> -		/* force 100% alpha */
> -		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> +	else if (drm_plane_solid_fill_enabled(plane->state))
> +		_dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),
> +				plane->state->alpha);
>  	else {
>  		dpu_plane_flush_csc(pdpu, &pstate->pipe);
>  		dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
> @@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
>  	}
>  
>  	/* override for color fill */
> -	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
> +	if (drm_plane_solid_fill_enabled(plane->state)) {
>  		_dpu_plane_set_qos_ctrl(plane, pipe, false);
>  
>  		/* skip remaining processing on color fill */
>
Jessica Zhang July 1, 2023, 1:26 a.m. UTC | #3
On 6/29/2023 5:59 PM, Dmitry Baryshkov wrote:
> On 30/06/2023 03:25, Jessica Zhang wrote:
>> Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
>> determine if the plane is solid fill. In addition drop the DPU plane
>> color_fill field as we can now use drm_plane_state.solid_fill instead,
>> and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
>> allow userspace to configure the alpha value for the solid fill color.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Minor suggestion below.
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index 4476722f03bb..11d4fb771a1f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -42,7 +42,6 @@
>>   #define SHARP_SMOOTH_THR_DEFAULT    8
>>   #define SHARP_NOISE_THR_DEFAULT    2
>> -#define DPU_PLANE_COLOR_FILL_FLAG    BIT(31)
>>   #define DPU_ZPOS_MAX 255
>>   /*
>> @@ -82,7 +81,6 @@ struct dpu_plane {
>>       enum dpu_sspp pipe;
>> -    uint32_t color_fill;
>>       bool is_error;
>>       bool is_rt_pipe;
>>       const struct dpu_mdss_cfg *catalog;
>> @@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct 
>> dpu_plane_state *pstate,
>>       _dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, 
>> pstate->rotation);
>>   }
>> +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill 
>> solid_fill)
> 
> Please consider accepting drm_plane_state instead and handling alpha 
> here. Then _dpu_color_fill can accept rgba colour instead of separate 
> RGB and alpha values.

Hi Dmitry,

Do you mean adding a patch to refactor _dpu_plane_color_fill() to accept 
an RGBA8888 color?

Since, currently, the `color` parameter gets truncated to a BGR888 value 
and OR'd with the `alpha` parameter [1].

Thanks,

Jessica Zhang

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L682

> 
>> +{
>> +    uint32_t ret = 0;
>> +
>> +    ret |= ((uint8_t) solid_fill.b) << 16;
>> +    ret |= ((uint8_t) solid_fill.g) << 8;
>> +    ret |= ((uint8_t) solid_fill.r);
>> +
>> +    return ret;
>> +}
>> +
>>   /**
>>    * _dpu_plane_color_fill - enables color fill on plane
>>    * @pdpu:   Pointer to DPU plane object
>> @@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane)
>>       if (pdpu->is_error)
>>           /* force white frame with 100% alpha pipe output on error */
>>           _dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
>> -    else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>> -        /* force 100% alpha */
>> -        _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>> +    else if (drm_plane_solid_fill_enabled(plane->state))
>> +        _dpu_plane_color_fill(pdpu, 
>> _dpu_plane_get_fill_color(plane->state->solid_fill),
>> +                plane->state->alpha);
>>       else {
>>           dpu_plane_flush_csc(pdpu, &pstate->pipe);
>>           dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
>> @@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct 
>> drm_plane *plane,
>>       }
>>       /* override for color fill */
>> -    if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
>> +    if (drm_plane_solid_fill_enabled(plane->state)) {
>>           _dpu_plane_set_qos_ctrl(plane, pipe, false);
>>           /* skip remaining processing on color fill */
>>
> 
> -- 
> With best wishes
> Dmitry
>
Pekka Paalanen July 3, 2023, 7:42 a.m. UTC | #4
On Fri, 30 Jun 2023 11:26:49 +0300
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Thu, 29 Jun 2023 17:25:06 -0700
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> 
> > Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
> > determine if the plane is solid fill. In addition drop the DPU plane
> > color_fill field as we can now use drm_plane_state.solid_fill instead,
> > and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
> > allow userspace to configure the alpha value for the solid fill color.
> > 
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 4476722f03bb..11d4fb771a1f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -42,7 +42,6 @@
> >  #define SHARP_SMOOTH_THR_DEFAULT	8
> >  #define SHARP_NOISE_THR_DEFAULT	2
> >  
> > -#define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
> >  #define DPU_ZPOS_MAX 255
> >  
> >  /*
> > @@ -82,7 +81,6 @@ struct dpu_plane {
> >  
> >  	enum dpu_sspp pipe;
> >  
> > -	uint32_t color_fill;
> >  	bool is_error;
> >  	bool is_rt_pipe;
> >  	const struct dpu_mdss_cfg *catalog;
> > @@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
> >  	_dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation);
> >  }
> >  
> > +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)
> > +{
> > +	uint32_t ret = 0;
> > +
> > +	ret |= ((uint8_t) solid_fill.b) << 16;
> > +	ret |= ((uint8_t) solid_fill.g) << 8;
> > +	ret |= ((uint8_t) solid_fill.r);  
> 
> solid_fill.r, g and b are uint32_t, yes?
> 
> What's the value encoding in the UAPI? That doc was missing.
> 
> I wouldn't expect the UAPI to use 32-bit variables if it was
> essentially 8-bit, so this conversion looks wrong.
> 
> Nominal color value 1.0 in u8 is 0xff. The same in u32 is probably
> 0xffffffff? So a simple cast to u8 won't work. You'd want to take the
> upper 8 bits instead.
> 
> 
> Thanks,
> pq
> 
> > +
> > +	return ret;

Btw. if your driver format is ABGR, then this function leaves alpha as
zero. That's confusing.

It would be nice to mention the exact pixel format in the function name
so the consistency is easier to check in both here and in callers.


Thanks,
pq

> > +}
> > +
> >  /**
> >   * _dpu_plane_color_fill - enables color fill on plane
> >   * @pdpu:   Pointer to DPU plane object
> > @@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane)
> >  	if (pdpu->is_error)
> >  		/* force white frame with 100% alpha pipe output on error */
> >  		_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
> > -	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
> > -		/* force 100% alpha */
> > -		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> > +	else if (drm_plane_solid_fill_enabled(plane->state))
> > +		_dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),
> > +				plane->state->alpha);
> >  	else {
> >  		dpu_plane_flush_csc(pdpu, &pstate->pipe);
> >  		dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
> > @@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
> >  	}
> >  
> >  	/* override for color fill */
> > -	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
> > +	if (drm_plane_solid_fill_enabled(plane->state)) {
> >  		_dpu_plane_set_qos_ctrl(plane, pipe, false);
> >  
> >  		/* skip remaining processing on color fill */
> >   
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 4476722f03bb..11d4fb771a1f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -42,7 +42,6 @@ 
 #define SHARP_SMOOTH_THR_DEFAULT	8
 #define SHARP_NOISE_THR_DEFAULT	2
 
-#define DPU_PLANE_COLOR_FILL_FLAG	BIT(31)
 #define DPU_ZPOS_MAX 255
 
 /*
@@ -82,7 +81,6 @@  struct dpu_plane {
 
 	enum dpu_sspp pipe;
 
-	uint32_t color_fill;
 	bool is_error;
 	bool is_rt_pipe;
 	const struct dpu_mdss_cfg *catalog;
@@ -606,6 +604,17 @@  static void _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
 	_dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation);
 }
 
+static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)
+{
+	uint32_t ret = 0;
+
+	ret |= ((uint8_t) solid_fill.b) << 16;
+	ret |= ((uint8_t) solid_fill.g) << 8;
+	ret |= ((uint8_t) solid_fill.r);
+
+	return ret;
+}
+
 /**
  * _dpu_plane_color_fill - enables color fill on plane
  * @pdpu:   Pointer to DPU plane object
@@ -977,9 +986,9 @@  void dpu_plane_flush(struct drm_plane *plane)
 	if (pdpu->is_error)
 		/* force white frame with 100% alpha pipe output on error */
 		_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
-	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
-		/* force 100% alpha */
-		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+	else if (drm_plane_solid_fill_enabled(plane->state))
+		_dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),
+				plane->state->alpha);
 	else {
 		dpu_plane_flush_csc(pdpu, &pstate->pipe);
 		dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
@@ -1024,7 +1033,7 @@  static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
 	}
 
 	/* override for color fill */
-	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
+	if (drm_plane_solid_fill_enabled(plane->state)) {
 		_dpu_plane_set_qos_ctrl(plane, pipe, false);
 
 		/* skip remaining processing on color fill */