Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f2a50fb0-f9d3-d884-8f67-28591413e935@quicinc.com>
Date:   Tue, 11 Jul 2023 17:01:42 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH RFC v4 7/7] drm/msm/dpu: Use DRM solid_fill property
Content-Language: en-US
To:     Pekka Paalanen <ppaalanen@gmail.com>
CC:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Clark <robdclark@gmail.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        "Sean Paul" <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        <quic_abhinavk@quicinc.com>, <contact@emersion.fr>,
        <laurent.pinchart@ideasonboard.com>, <sebastian.wick@redhat.com>,
        <ville.syrjala@linux.intel.com>, <dri-devel@lists.freedesktop.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <freedreno@lists.freedesktop.org>,
        <wayland-devel@lists.freedesktop.org>
References: <20230404-solid-fill-v4-0-f4ec0caa742d@quicinc.com>
 <20230404-solid-fill-v4-7-f4ec0caa742d@quicinc.com>
 <20230630112649.263331b4@eldfell> <20230703104246.7226953a@eldfell>
From:   Jessica Zhang <quic_jesszhan@quicinc.com>
In-Reply-To: <20230703104246.7226953a@eldfell>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/3/2023 12:42 AM, Pekka Paalanen wrote:
> 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.

Hi Pekka,

The solid fill blob will accept an RGB323232 value -- will document this 
in the drm_solid_fill_info struct

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

Acked.

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

Acked.

Thanks,

Jessica Zhang

> 
> 
> 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 */
>>>    
>>
>