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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 06, 2023 at 04:33:04PM -0800, Abhinav Kumar wrote:
> Hi Daniel
> 
> Thanks for looking into this series.
> 
> On 1/6/2023 1:49 PM, Dmitry Baryshkov wrote:
> > On Fri, 6 Jan 2023 at 20:41, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > 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.
> > 
> > I checked your idea and notes from Jessica. So while we can pass blobs
> > to property objects, the prop_fb_id is created as an object property
> > with the type DRM_MODE_OBJECT_FB. Passing DRM_MODE_OBJECT_BLOB would
> > fail a check in drm_property_change_valid_get() ->
> > __drm_mode_object_find(). And I don't think that we should break the
> > existing validation code for this special case.
> > 
> 
> Like Jessica wrote, re-using the FB_ID property to pass solid fill
> information will need modification of existing checks shown in [1] OR the
> property creation itself would fail.

Yeah modifications there are needed. Or well, we need to extend that code
to allow bitmask for objects iff the driver allows that. What I meant is
that from an uapi pov it's doable, which is the part that matters because
that is the part that we wont ever be able to change once it's baked in.

> We just went with this approach, as it was less intrusive and would not
> affect the existing FB_ID path.
> 
> Since both approaches need modifications of validation checks, adding a new
> property is less intrusive and safer than the already convoluted checks in
> drm_property_flags_valid().

It's easier, but it's also the part that we cannot ever change (because
it's uapi). If the NULL fb on an active plane makes some compositor die on
vt switch, we then have a pretty serious problem.

I'm honestly not sure which is the approach that blows up less, just that
justifying uapi with the implementation work on the kernel side is not
really the right approach :-)

> Let us know if its a strong preference on your side to re-use FB_ID and if
> so why.

I just want to make sure we're picking the right uapi option, and that
we're not just excluding them because it looks like we can't implement
them in the kernel. Uapi is forever, the implemenation is not.
-Daniel

> Thanks
> 
> Abhinav
> 
> > If you insist on using FB_ID for passing solid_fill information, I'd
> > ask you to reconsider using a 1x1 framebuffer. It would be fully
> > compatible with the existing userspace, which can then treat it
> > seamlessly.
> > 
> > > -Daniel
> > > 
> > > > 
> > > > > 
> > > > > [1]
> > > > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71
> > 

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