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

[Jessica Zhang]

On 7/3/2023 12:42 AM, Pekka Paalanen wrote:
> On Fri, 30 Jun 2023 11:26:49 +0300
> Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
>
> > On Thu, 29 Jun 2023 17:25:06 -0700
> > Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > ---
> > >   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 */
> > >