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

[Jessica Zhang]

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@xxxxxxxxxxx>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> 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