Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes
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 wrote: On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote: On Fri, 6 Jan 2023 at 02:38, Jessica Zhang 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. 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(). Let us know if its a strong preference on your side to re-use FB_ID and if so why. 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
Re: [RFC PATCH 0/3] Support for Solid Fill Planes
On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
On 28/06/2023 00:27, Jessica Zhang wrote:
On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
On Mon, 26 Jun 2023 16:02:50 -0700
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.
Right, that won't work.
There is no ordering between each property being set inside a single
atomic commit. They can all be applied to kernel-internal state
theoretically simultaneously, or any arbitrary random order, and the
end result must always be the same. Hence, setting one property cannot
change the state of another mutable property. I believe that doing
otherwise would make userspace fragile and hard to get right.
I guess there might be an exception to that rule when the same property
is set multiple times in a single atomic commit; the last setting in
the array prevails. That's universal and not a special-case between two
specific properties.
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.
Sounds fine to me at first hand, but isn't there the enum property that
says if the kernel must look at solid_fill blob *or* FB_ID?
If enum prop says "use solid_fill prop", the why would changes to FB_ID
do anything? Is it for backwards-compatibility with KMS clients that do
not know about the enum prop?
It seems like that kind of backwards-compatiblity will cause problems
in trying to reason about the atomic state, as explained above, leading
to very delicate and fragile conditions where things work intuitively.
Hence, I'm not sure backwards-compatibility is wanted. This won't be
the first or the last KMS property where an unexpected value left over
will make old atomic KMS clients silently malfunction up to showing no
recognisable picture at all. *If* that problem needs solving, there
have been ideas floating around about resetting everything to nice
values so that userspace can ignore what it does not understand. So far
there has been no real interest in solving that problem in the kernel
though.
Legacy non-atomic UAPI wrappers can do whatever they want, and program
any (new) properties they want in order to implement the legacy
expectations, so that does not seem to be a problem.
Hi Pekka and Dmitry,
After reading through both of your comments, I think I have a better
understanding of the pixel_source implementation now.
So to summarize, we want to expose another property called
"pixel_source" to userspace that will default to FB (as to not break
legacy userspace).
If userspace wants to use solid fill planes, it will set both the
solid_fill *and* pixel_source properties to a valid blob and COLOR
respectively. If it wants to use FB, it will set FB_ID and
pixel_source to a valid FB and FB.
Here's a table illustrating what I've described above:
+-+-+-+
| Use Case | Legacy Userspace | solid_fill-aware |
| | | Userspace |
+=+=+=+
| Valid FB | pixel_source = FB | pixel_source = FB |
| | FB_ID = valid FB | FB_ID = valid FB |
+-+-+-+
| Valid | pixel_source = COLOR | N/A |
| solid_fill blob | solid_fill = valid blob | |
Probably these two cells were swapped.
Ack, yes they were swapped.
+-+-+-+
| NULL commit | pixel_source = FB | pixel_source = FB |
| | FB_ID = NULL | FB_ID = NULL
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On 7/11/2023 3:19 PM, Dmitry Baryshkov wrote:
On 12/07/2023 01:07, Jessica Zhang wrote:
On 7/10/2023 1:11 PM, Dmitry Baryshkov wrote:
On 10/07/2023 22:51, Jessica Zhang wrote:
On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
On Fri, 30 Jun 2023 03:42:28 +0300
Dmitry Baryshkov wrote:
On 30/06/2023 03:25, Jessica Zhang wrote:
Add support for pixel_source property to drm_plane and related
documentation.
This enum property will allow user to specify a pixel source for the
plane. Possible pixel sources will be defined in the
drm_plane_pixel_source enum.
The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
I think, this should come before the solid fill property addition.
First
you tell that there is a possibility to define other pixel
sources, then
additional sources are defined.
Hi,
that would be logical indeed.
Hi Dmitry and Pekka,
Sorry for the delay in response, was out of office last week.
Acked.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
drivers/gpu/drm/drm_blend.c | 81
+++
include/drm/drm_blend.h | 2 +
include/drm/drm_plane.h | 21
5 files changed, 109 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index fe14be2bd2b2..86fb876efbe6 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -252,6 +252,7 @@ void
__drm_atomic_helper_plane_state_reset(struct drm_plane_state
*plane_state,
plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+ plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
if (plane_state->solid_fill_blob) {
drm_property_blob_put(plane_state->solid_fill_blob);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index a28b4ee79444..6e59c21af66b 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -596,6 +596,8 @@ static int
drm_atomic_plane_set_property(struct drm_plane *plane,
drm_property_blob_put(solid_fill);
return ret;
+ } else if (property == plane->pixel_source_property) {
+ state->pixel_source = val;
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
I think, it was pointed out in the discussion that
drm_mode_setplane()
(a pre-atomic IOCTL to turn the plane on and off) should also reset
pixel_source to FB.
I don't remember drm_mode_setplane() being mentioned in the
pixel_source discussion... can you share where it was mentioned?
https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/
Let me quote it here:
"Legacy non-atomic UAPI wrappers can do whatever they want, and program
any (new) properties they want in order to implement the legacy
expectations, so that does not seem to be a problem."
I'd prefer to avoid having driver change the pixel_source directly
as it could cause some unexpected side effects. In general, I would
like userspace to assign the value of pixel_source without driver
doing anything "under the hood".
s/driver/drm core/
We have to remain compatible with old userspace, especially with the
non-atomic one. If the userspace calls
ioctl(DRM_IOCTL_MODE_SETPLANE), we have to display the specified FB,
no matter what was the value of PIXEL_SOURCE before this ioctl.
Got it, thanks the clarification -- I see your point.
I'm already setting plane_state->pixel_source to FB in
__drm_atomic_helper_plane_reset() and it seems to me that all drivers
are calling that within their respective plane_funcs->reset().
Since (as far as I know) reset() is being called for both the atomic
and non-atomic paths, shouldn't that be enough to default pixel_source
to FB for old userspace?
No, this will not clean up the state between userspace apps. Currently
the rule is simple: call DRM_IOCTL_MODE_SETPLANE, get the image from FB
displayed. We should keep it so.
Ok, so you are considering a use-case where we bootup with a userspace
(which is aware of pixel_source), that one uses the pixel_source to
switch the property to solid_color and then we kill this userspace and
bootup one which is unaware of this property and uses
DRM_IOCTL_MODE_SETPLANE, then we should default back to FB.
@@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct
drm_plane *plane,
} else if (property == plane->solid_fill_property) {
*val =state->solid_fill_blob ?
state->solid_fill_blob->base.id : 0;
+ } else if (property == plane->pixel_source_property) {
+ *val = state->pixel_source;
} else if (property == plane->
Re: (subset) [PATCH RFC v7 00/10] Support for Solid Fill Planes
Hi Simon On 12/3/2023 4:15 AM, Simon Ser wrote: On Saturday, December 2nd, 2023 at 22:41, Dmitry Baryshkov wrote: On Fri, 27 Oct 2023 15:32:50 -0700, Jessica Zhang wrote: 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 test app. In order to expose this capability to userspace, this series will: [...] Applied to drm-misc-next, thanks! Where are the IGT and userspace for this uAPI addition? Yes, we made IGT changes to test and validate this. We will post them on the IGT dev list shortly and CC you. We do not have a compositor change yet for this as we primarily used IGT to validate this. Can we re-try to land this once IGT changes are accepted? Thanks Abhinav
Re: (subset) [PATCH RFC v7 00/10] Support for Solid Fill Planes
On 12/4/2023 9:57 AM, Simon Ser wrote: On Monday, December 4th, 2023 at 18:51, Abhinav Kumar wrote: Where are the IGT and userspace for this uAPI addition? Yes, we made IGT changes to test and validate this. We will post them on the IGT dev list shortly and CC you. We do not have a compositor change yet for this as we primarily used IGT to validate this. Yes, please post the IGT. Can we re-try to land this once IGT changes are accepted? There will need to be a user-space implementation as well, since this is a hard requirement for new uAPI [1]. Maybe I'll give this a go if I have time. Much appreciated. [1]: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
