Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
From: Dmitry Baryshkov
Date: Tue Jul 11 2023 - 18:19:30 EST
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 <dmitry.baryshkov@xxxxxxxxxx> 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 <quic_jesszhan@xxxxxxxxxxx>
---
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.
@@ -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->alpha_property) {
*val =state->alpha;
} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c
b/drivers/gpu/drm/drm_blend.c
index 38c3c5d6453a..8c100a957ee2 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -189,6 +189,18 @@
* solid_fill is set up with
drm_plane_create_solid_fill_property(). It
* contains pixel data that drivers can use to fill a plane.
*
+ * pixel_source:
+ * pixel_source is set up with
drm_plane_create_pixel_source_property().
+ * It is used to toggle the source of pixel data for the plane.
Other sources than the selected one are ignored?
Yep, the plane will only display the data from the set pixel_source.
So if pixel_source == FB and solid_fill_blob is non-NULL,
solid_fill_blob will be ignored and the plane will display the FB
that is set.
correct.
Will add a note about this in the comment docs.
+ *
+ * Possible values:
Wouldn't hurt to explicitly mention here that this is an enum.
Acked.
+ *
+ * "FB":
+ * Framebuffer source
+ *
+ * "COLOR":
+ * solid_fill source
I think these two should be more explicit. Framebuffer source is the
usual source from the property "FB_ID". Solid fill source comes from
the property "solid_fill".
Acked.
Why "COLOR" and not, say, "SOLID_FILL"?
Ah, that would make more sense :)
I'll change this to "SOLID_FILL".
+ *
* Note that all the property extensions described here apply
either to the
* plane or the CRTC (e.g. for the background color, which
currently is not
* exposed and assumed to be black).
@@ -648,3 +660,72 @@ int
drm_plane_create_solid_fill_property(struct drm_plane *plane)
return 0;
}
EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
+
+/**
+ * drm_plane_create_pixel_source_property - create a new pixel
source property
+ * @plane: drm plane
+ * @supported_sources: bitmask of supported pixel_sources for the
driver (NOT
+ * including DRM_PLANE_PIXEL_SOURCE_FB, as it
will be supported
+ * by default).
I'd say this is too strong. I'd suggest either renaming this to
extra_sources (mentioning that FB is enabled for all the planes) or
allowing any source bitmask (mentioning that FB should be enabled
by the
caller, unless there is a good reason not to do so).
Right. I don't see any problem with having planes of type OVERLAY that
support only solid_fill and no FB. Planes of type PRIMARY and CURSOR I
would expect to always support at least FB.
Atomic userspace is prepared to have an OVERLAY plane fail for any
arbitrary reason. Legacy userspace probably should not ever see a plane
that does not support FB.
Got it... If we allow the possibility of FB sources not being
supported, then should the default pixel_source per plane be decided
by the driver too?
I'd forced FB support so that I could set pixel_source to FB in
__drm_atomic_helper_plane_state_reset(). If we allow more flexibility
in the default pixel_source value, I guess we can also store a
default_pixel_source value in the plane_state.
I'd say, the FB is a sane default. It the driver has other needs, it
can override the value in drm_plane_funcs::reset().
[skipped the rest]
--
With best wishes
Dmitry
--
With best wishes
Dmitry