Re: [RFC PATCH 0/3] Support for Solid Fill Planes
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.
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.
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(-)
--
2.38.0
--
Ville Syrjälä
Intel
Re: [RFC PATCH 0/3] Support for Solid Fill Planes
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.
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.
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(-)
--
2.38.0
--
Ville Syrjälä
Intel
--
With best wishes
Dmitry
Re: [RFC PATCH 0/3] Support for Solid Fill Planes
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;
}
// ...
}
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.
[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.
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
Re: [RFC PATCH 0/3] Support for Solid Fill Planes
On Tue, 27 Jun 2023 at 03:45, Jessica Zhang 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
