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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 07, 2022 at 07:34:43PM -0800, Rob Clark wrote:
> On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >
> >
> >
> > On 11/7/2022 2:09 PM, Rob Clark wrote:
> > > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> > >>
> > >>
> > >>
> > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> > >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> > >>>> properties. When the color fill value is set, and the framebuffer is set
> > >>>> to NULL, memory fetch will be disabled.
> > >>>
> > >>> Thinking a bit more universally I wonder if there should be
> > >>> some kind of enum property:
> > >>>
> > >>> enum plane_pixel_source {
> > >>>        FB,
> > >>>        COLOR,
> > >>>        LIVE_FOO,
> > >>>        LIVE_BAR,
> > >>>        ...
> > >>> }
> > >>
> > >> Hi Ville,
> > >>
> > >> Makes sense -- this way, we'd also lay some groundwork for cases where
> > >> drivers want to use other non-FB sources.
> > >>
> > >>>
> > >>>> In addition, loosen the NULL FB checks within the atomic commit callstack
> > >>>> to allow a NULL FB when color_fill is nonzero 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.color_fill and
> > >>>> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a
> > >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
> > >>>> framebuffer to NULL.
> > >>>
> > >>> Is there some real reason for the format property? Ie. why not
> > >>> just do what was the plan for the crttc background color and
> > >>> specify the color in full 16bpc format and just pick as many
> > >>> msbs from that as the hw can use?
> > >>
> > >> The format property was added because we can't assume that all hardware
> > >> will support/use the same color format for solid fill planes. Even for
> > >> just MSM devices, the hardware supports different variations of RGB
> > >> formats [1].
> > >
> > > Sure, but the driver can convert the format into whatever the hw
> > > wants.  A 1x1 color conversion is not going to be problematic ;-)
> >
> > Hi Rob,
> >
> > Hm... that's also a fair point. Just wondering, is there any advantage
> > of having the driver convert the format, other than not having to
> > implement an extra format property?
> >
> > (In case we end up wrapping everything into a prop blob or something)
> >
> 
> It keeps the uabi simpler.. for obvious reasons you don't want the
> driver to do cpu color conversion for an arbitrary size plane, which
> is why we go to all the complexity to expose formats and modifiers for
> "real" planes, but we are dealing with a single pixel value here,
> let's not make the uabi more complex than we need to.  I'd propose
> making it float32[4] if float weren't a pita for kernel/uabi, but
> u16[4] or u32[4] should be fine, and drivers can translate that easily
> into whatever weird formats their hw wants for solid-fill.

u16[4] fits into a single u64 property value.

That was the plan for the background prop as well:
https://lore.kernel.org/all/20190703125442.GW5942@intel.com/T/

-- 
Ville Syrjälä
Intel