diff mbox series

[RFC,v5,09/10] drm/msm/dpu: Use DRM solid_fill property

Message ID 20230728-solid-fill-v5-9-053dbefa909c@quicinc.com
State New
Headers show
Series Support for Solid Fill Planes | expand

Commit Message

Jessica Zhang July 28, 2023, 5:02 p.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.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Dmitry Baryshkov July 31, 2023, 4:15 a.m. UTC | #1 
On 28/07/2023 20:02, 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.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 ++++++++++++++++++------
>   1 file changed, 18 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 114c803ff99b..95fc0394d13e 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,20 @@ 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_bgr_fill_color(struct drm_solid_fill solid_fill)

As I commented for v4 (please excuse me for not responding to your email 
at thattime), we can return abgr here, taking plane->state->alpha into 
account.

> +{
> +	uint32_t ret = 0;
> +	uint8_t b = solid_fill.b >> 24;
> +	uint8_t g = solid_fill.g >> 24;
> +	uint8_t r = solid_fill.r >> 24;
> +
> +	ret |= b << 16;
> +	ret |= g << 8;
> +	ret |= r;
> +
> +	return ret;
> +}
> +
>   /**
>    * _dpu_plane_color_fill - enables color fill on plane
>    * @pdpu:   Pointer to DPU plane object
> @@ -977,9 +989,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_bgr_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 +1036,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 Aug. 1, 2023, 12:39 a.m. UTC | #2 
On 7/30/2023 9:15 PM, Dmitry Baryshkov wrote:
> On 28/07/2023 20:02, 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.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 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 114c803ff99b..95fc0394d13e 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,20 @@ 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_bgr_fill_color(struct drm_solid_fill 
>> solid_fill)
> 
> As I commented for v4 (please excuse me for not responding to your email 
> at thattime), we can return abgr here, taking plane->state->alpha into 
> account.

Hi Dmitry,

Since it seems that this comment wasn't resolved, I can drop your R-B 
tag in the next revision.

 From my previous response, I pointed out that the color parameter 
expects an RGB value [1].

So is the intention here to refactor _dpu_plane_color_fill() to accept 
an ABGR8888 color?

Thanks,

Jessica Zhang

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

> 
>> +{
>> +    uint32_t ret = 0;
>> +    uint8_t b = solid_fill.b >> 24;
>> +    uint8_t g = solid_fill.g >> 24;
>> +    uint8_t r = solid_fill.r >> 24;
>> +
>> +    ret |= b << 16;
>> +    ret |= g << 8;
>> +    ret |= r;
>> +
>> +    return ret;
>> +}
>> +
>>   /**
>>    * _dpu_plane_color_fill - enables color fill on plane
>>    * @pdpu:   Pointer to DPU plane object
>> @@ -977,9 +989,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_bgr_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 +1036,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
>
Dmitry Baryshkov Aug. 1, 2023, 12:52 a.m. UTC | #3 
On 01/08/2023 03:39, Jessica Zhang wrote:
> 
> 
> On 7/30/2023 9:15 PM, Dmitry Baryshkov wrote:
>> On 28/07/2023 20:02, 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.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 
>>> ++++++++++++++++++------
>>>   1 file changed, 18 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 114c803ff99b..95fc0394d13e 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,20 @@ 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_bgr_fill_color(struct drm_solid_fill 
>>> solid_fill)
>>
>> As I commented for v4 (please excuse me for not responding to your 
>> email at thattime), we can return abgr here, taking 
>> plane->state->alpha into account.
> 
> Hi Dmitry,
> 
> Since it seems that this comment wasn't resolved, I can drop your R-B 
> tag in the next revision.

It's a minor issue, so no need to drop the tag.

> 
>  From my previous response, I pointed out that the color parameter 
> expects an RGB value [1].
> 
> So is the intention here to refactor _dpu_plane_color_fill() to accept 
> an ABGR8888 color?

That's what I'm suggesting.

> 
> Thanks,
> 
> Jessica Zhang
> 
> [1] 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L676
> 
>>
>>> +{
>>> +    uint32_t ret = 0;
>>> +    uint8_t b = solid_fill.b >> 24;
>>> +    uint8_t g = solid_fill.g >> 24;
>>> +    uint8_t r = solid_fill.r >> 24;
>>> +
>>> +    ret |= b << 16;
>>> +    ret |= g << 8;
>>> +    ret |= r;
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   /**
>>>    * _dpu_plane_color_fill - enables color fill on plane
>>>    * @pdpu:   Pointer to DPU plane object
>>> @@ -977,9 +989,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_bgr_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 +1036,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
>>
Jessica Zhang Aug. 7, 2023, 4:59 p.m. UTC | #4 
On 7/31/2023 5:52 PM, Dmitry Baryshkov wrote:
> On 01/08/2023 03:39, Jessica Zhang wrote:
>>
>>
>> On 7/30/2023 9:15 PM, Dmitry Baryshkov wrote:
>>> On 28/07/2023 20:02, 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.
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 
>>>> ++++++++++++++++++------
>>>>   1 file changed, 18 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 114c803ff99b..95fc0394d13e 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,20 @@ 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_bgr_fill_color(struct drm_solid_fill 
>>>> solid_fill)
>>>
>>> As I commented for v4 (please excuse me for not responding to your 
>>> email at thattime), we can return abgr here, taking 
>>> plane->state->alpha into account.
>>
>> Hi Dmitry,
>>
>> Since it seems that this comment wasn't resolved, I can drop your R-B 
>> tag in the next revision.
> 
> It's a minor issue, so no need to drop the tag.
> 
>>
>>  From my previous response, I pointed out that the color parameter 
>> expects an RGB value [1].
>>
>> So is the intention here to refactor _dpu_plane_color_fill() to accept 
>> an ABGR8888 color?
> 
> That's what I'm suggesting.

Hi Dmitry,

Got it, sounds good to me.

Thanks,

Jessica Zhang

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>> [1] 
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L676
>>
>>>
>>>> +{
>>>> +    uint32_t ret = 0;
>>>> +    uint8_t b = solid_fill.b >> 24;
>>>> +    uint8_t g = solid_fill.g >> 24;
>>>> +    uint8_t r = solid_fill.r >> 24;
>>>> +
>>>> +    ret |= b << 16;
>>>> +    ret |= g << 8;
>>>> +    ret |= r;
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>   /**
>>>>    * _dpu_plane_color_fill - enables color fill on plane
>>>>    * @pdpu:   Pointer to DPU plane object
>>>> @@ -977,9 +989,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_bgr_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 +1036,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
>>>
> 
> -- 
> With best wishes
> Dmitry
>
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 114c803ff99b..95fc0394d13e 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,20 @@  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_bgr_fill_color(struct drm_solid_fill solid_fill)
+{
+	uint32_t ret = 0;
+	uint8_t b = solid_fill.b >> 24;
+	uint8_t g = solid_fill.g >> 24;
+	uint8_t r = solid_fill.r >> 24;
+
+	ret |= b << 16;
+	ret |= g << 8;
+	ret |= r;
+
+	return ret;
+}
+
 /**
  * _dpu_plane_color_fill - enables color fill on plane
  * @pdpu:   Pointer to DPU plane object
@@ -977,9 +989,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_bgr_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 +1036,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 */