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

[Dmitry Baryshkov]

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@xxxxxxxxxx>
> > > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> > > ---
> > >   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

--
With best wishes
Dmitry