On Tue, 27 Jun 2023 at 03:45, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:
>
>
>
> On 6/26/2023 5:06 PM, Dmitry Baryshkov wrote:
> > On 27/06/2023 02:02, Jessica Zhang 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,
> >>> ...
> >>> }
> >>
> >> Reviving this thread as this was the initial comment suggesting to
> >> implement pixel_source as an enum.
> >>
> >> I think the issue with having pixel_source as an enum is how to decide
> >> what counts as a NULL commit.
> >>
> >> Currently, setting the FB to NULL will disable the plane. So I'm
> >> guessing we will extend that logic to "if there's no pixel_source set
> >> for the plane, then it will be a NULL commit and disable the plane".
> >>
> >> In that case, the question then becomes when to set the pixel_source
> >> to NONE. Because if we do that when setting a NULL FB (or NULL
> >> solid_fill blob), it then forces userspace to set one property before
> >> the other.
> >
> > Why? The userspace should use atomic commits and as such it should all
> > properties at the same time.
>
> Correct, userspace will set all the properties within the same atomic
> commit. The issue happens when the driver iterates through each property
> within the MODE_ATOMIC ioctl [1].
>
> For reference, I'm thinking of an implementation where we're setting the
> pixel_source within drm_atomic_plane_set_property().
>
> So something like:
>
> drm_atomic_plane_set_property( ... )
> {
> if (property == config->prop_fb_id) {
> if (fb)
> state->pixel_source = FB;
> else
> state->pixel_source = NONE;
> } else if (property == config->prop_solid_fill) {
> if (solid_fill_blob)
> state->pixel_source = SOLID_FILL;
> }
>
> // ...
> }
I think this is somewhat overcomplicated. Allow userspace to set these
properties as it sees fit and then in
drm_atomic_helper_check_plane_state() consider all of them to set
plane_state->visible.
We still have to remain compatible with older userspace (esp. with a
non-atomic one). It expects that a plane is enabled after setting both
CRTC and FB. So maybe you are right and we should force pixel_source
to FB if FB is set.
>
> If userspace sets solid_fill to a valid blob and FB to NULL, it's
> possible for driver to first set the solid_fill property then set the
> fb_id property later. This would cause pixel_source to be set to NONE
> after all properties have been set.
>
> I've also considered an implementation without the `pixel_source = NONE`
> line in the prop_fb_id case, but we would still need to find somewhere
> to set the pixel_source to NONE in order to allow userspace to disable a
> plane.
Good point. I don't think we would need NONE (just setting CRTC to
none or FB to none and pixel_source to FB would disable the plane),
but I might be missing something here.
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_uapi.c#L1385
>
> >
> >> Because of that, I'm thinking of having pixel_source be represented by
> >> a bitmask instead. That way, we will simply unset the corresponding
> >> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
> >> order to detect whether a commit is NULL or has a valid pixel source,
> >> we can just check if pixel_source == 0.
> >>
> >> Would be interested in any feedback on this.
> >
> > This is an interesting idea. Frankly speaking, I'd consider it
> > counter-intuitive at the first glance.
> >
> > Consider it to act as a curtain. Setup the curtain (by writing the fill
> > colour property). Then one can close the curtain (by switching source to
> > colour), or open it (by switching to any other source). Bitmask wouldn't
> > complicate this.
>
> So if I'm understanding your analogy correctly, pixel_source won't
> necessarily be set whenever the FB or solid_fill properties are set. So
> that way we can have both FB *and* solid_fill set at the same time, but
> only the source that pixel_source is set to would be displayed.
Yes. And if the source is not configured, the plane will be marked as
not visible.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >>
> >> Thanks,
> >>
> >> Jessica Zhang
> >>
> >>>
> >>>> 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?
> >>>
> >>>>
> >>>> Jessica Zhang (3):
> >>>> drm: Introduce color fill properties 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 | 68
> >>>> ++++++++++++-----------
> >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
> >>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> >>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
> >>>> drivers/gpu/drm/drm_plane.c | 8 +--
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
> >>>> include/drm/drm_atomic_helper.h | 5 +-
> >>>> include/drm/drm_blend.h | 2 +
> >>>> include/drm/drm_plane.h | 28 ++++++++++
> >>>> 10 files changed, 188 insertions(+), 76 deletions(-)
--
With best wishes
Dmitry
