Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote:
> On Fri, 6 Jan 2023 at 02:38, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >
> >
> >
> > On 1/5/2023 3:33 AM, Daniel Vetter wrote:
> > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote:
> > >> Introduce and add support for a solid_fill property. When the solid_fill
> > >> property is set, and the framebuffer is set to NULL, memory fetch will be
> > >> disabled.
> > >>
> > >> In addition, loosen the NULL FB checks within the atomic commit callstack
> > >> to allow a NULL FB when the solid_fill property is set and add FB checks
> > >> in methods where the FB was previously assumed to be non-NULL.
> > >>
> > >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead of
> > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit
> > >> callstack to account for a NULL FB in cases where solid_fill is set.
> > >>
> > >> Some drivers support hardware that have optimizations for solid fill
> > >> planes. This series aims to expose these capabilities to userspace as
> > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > >> hardware composer HAL) that can be set by apps like the Android Gears
> > >> app.
> > >>
> > >> Userspace can set the solid_fill property to a blob containing the
> > >> appropriate version number and solid fill color (in RGB323232 format) and
> > >> setting the framebuffer to NULL.
> > >>
> > >> Note: Currently, there's only one version of the solid_fill blob property.
> > >> However if other drivers want to support a similar feature, but require
> > >> more than just the solid fill color, they can extend this feature by
> > >> creating additional versions of the drm_solid_fill struct.
> > >>
> > >> Changes in V2:
> > >> - Dropped SOLID_FILL_FORMAT property (Simon)
> > >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry)
> > >> - Changed to checks for if solid_fill_blob is set (Dmitry)
> > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method
> > >>    (Dmitry)
> > >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> > >> - Fixed whitespace and indentation issues (Dmitry)
> > >
> > > Now that this is a blob, I do wonder again whether it's not cleaner to set
> > > the blob as the FB pointer. Or create some kind other kind of special data
> > > source objects (because solid fill is by far not the only such thing).
> > >
> > > We'd still end up in special cases like when userspace that doesn't
> > > understand solid fill tries to read out such a framebuffer, but these
> > > cases already exist anyway for lack of priviledges.
> > >
> > > So I still think that feels like the more consistent way to integrate this
> > > feature. Which doesn't mean it has to happen like that, but the
> > > patches/cover letter should at least explain why we don't do it like this.
> >
> > Hi Daniel,
> >
> > IIRC we were facing some issues with this check [1] when trying to set
> > FB to a PROP_BLOB instead. Which is why we went with making it a
> > separate property instead. Will mention this in the cover letter.
> 
> What kind of issues? Could you please describe them?

We switched from bitmask to enum style for prop types, which means it's
not possible to express with the current uapi a property which accepts
both an object or a blob.

Which yeah sucks a bit ...

But!

blob properties are kms objects (like framebuffers), so it should be
possible to stuff a blob into an object property as-is. Of course you need
to update the validation code to make sure we accept either an fb or a
blob for the internal representation. But that kind of split internally is
required no matter what I think.
-Daniel

> 
> >
> > [1]
> > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> > > -Daniel
> > >
> > >>
> > >> Changes in V3:
> > >> - Fixed some logic errors in atomic checks (Dmitry)
> > >> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper
> > >>    methods (Dmitry)
> > >>
> > >> Jessica Zhang (3):
> > >>    drm: Introduce solid fill property for drm plane
> > >>    drm: Adjust atomic checks for solid fill color
> > >>    drm/msm/dpu: Use color_fill property for DPU planes
> > >>
> > >>   drivers/gpu/drm/drm_atomic.c              | 136 +++++++++++++---------
> > >>   drivers/gpu/drm/drm_atomic_helper.c       |  34 +++---
> > >>   drivers/gpu/drm/drm_atomic_state_helper.c |   9 ++
> > >>   drivers/gpu/drm/drm_atomic_uapi.c         |  59 ++++++++++
> > >>   drivers/gpu/drm/drm_blend.c               |  17 +++
> > >>   drivers/gpu/drm/drm_plane.c               |   8 +-
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   9 +-
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |  65 +++++++----
> > >>   include/drm/drm_atomic_helper.h           |   5 +-
> > >>   include/drm/drm_blend.h                   |   1 +
> > >>   include/drm/drm_plane.h                   |  62 ++++++++++
> > >>   11 files changed, 302 insertions(+), 103 deletions(-)
> > >>
> > >> --
> > >> 2.38.1
> > >>
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch