Re: [PATCH v5 6/6] drm/doc: Define KMS atomic state set
On Fri, 7 Jul 2023 19:40:59 -0300 André Almeida wrote: > From: Pekka Paalanen > > Specify how the atomic state is maintained between userspace and > kernel, plus the special case for async flips. > > Signed-off-by: Pekka Paalanen > Signed-off-by: André Almeida > --- > v4: total rework by Pekka > --- > Documentation/gpu/drm-uapi.rst | 41 ++ > 1 file changed, 41 insertions(+) Thank you for polishing that email into a proper patch! For patches 1 and 2: Acked-by: Pekka Paalanen Thanks, pq > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst > index 65fb3036a580..6a1662c08901 100644 > --- a/Documentation/gpu/drm-uapi.rst > +++ b/Documentation/gpu/drm-uapi.rst > @@ -486,3 +486,44 @@ and the CRTC index is its position in this array. > > .. kernel-doc:: include/uapi/drm/drm_mode.h > :internal: > + > +KMS atomic state > + > + > +An atomic commit can change multiple KMS properties in an atomic fashion, > +without ever applying intermediate or partial state changes. Either the > whole > +commit succeeds or fails, and it will never be applied partially. This is the > +fundamental improvement of the atomic API over the older non-atomic API > which is > +referred to as the "legacy API". Applying intermediate state could > unexpectedly > +fail, cause visible glitches, or delay reaching the final state. > + > +An atomic commit can be flagged with DRM_MODE_ATOMIC_TEST_ONLY, which means > the > +complete state change is validated but not applied. Userspace should use > this > +flag to validate any state change before asking to apply it. If validation > fails > +for any reason, userspace should attempt to fall back to another, perhaps > +simpler, final state. This allows userspace to probe for various > configurations > +without causing visible glitches on screen and without the need to undo a > +probing change. > + > +The changes recorded in an atomic commit apply on top the current KMS state > in > +the kernel. Hence, the complete new KMS state is the complete old KMS state > with > +the committed property settings done on top. The kernel will automatically > avoid > +no-operation changes, so it is safe and even expected for userspace to send > +redundant property settings. No-operation changes do not count towards > actually > +needed changes, e.g. setting MODE_ID to a different blob with identical > +contents as the current KMS state shall not be a modeset on its own. > + > +A "modeset" is a change in KMS state that might enable, disable, or > temporarily > +disrupt the emitted video signal, possibly causing visible glitches on > screen. A > +modeset may also take considerably more time to complete than other kinds of > +changes, and the video sink might also need time to adapt to the new signal > +properties. Therefore a modeset must be explicitly allowed with the flag > +DRM_MODE_ATOMIC_ALLOW_MODESET. This in combination with > +DRM_MODE_ATOMIC_TEST_ONLY allows userspace to determine if a state change is > +likely to cause visible disruption on screen and avoid such changes when end > +users do not expect them. > + > +An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is allowed to > +effectively change only the FB_ID property on any planes. No-operation > changes > +are ignored as always. Changing any other property will cause the commit to > be > +rejected. pgpIHjwcc8zEB.pgp Description: OpenPGP digital signature
Re: Wayland Governance Meeting
Hi everyone, Wednesday 12 of July 2023, 11:00 CEST has been selected for this weeks meeting. Best regards, Victoria Friday, Jul 07 2023 at 16:38 +0200 Victoria Brekenfeld wrote: > This is a meeting annoucement. > > The Wayland Governance Meeting is semi-regular meeting to drive > discussion on wayland-protocols forward. > > Agenda for the next meeting: > ext-layer-shell > ext-screencopy > ext-foreign-toplevel > ext-workspaces > > > Link to the doodle: > https://nuudel.digitalcourage.de/ckEBoLoIuGiO4VrU > The final date will be annouced on Monday 10th of July. > > Notes of previous meetings can be found here: > https://gitlab.freedesktop.org/wayland/wayland-protocols/-/wikis/meetings > > In the interest of not having the meeting-link publically available, > please contact me under victo...@system76.com, if you wish to > participate. > > Best regards, > Victoria Brekenfeld >
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
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?
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".
@@ -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.
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 n
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
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.
@@ -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.
+ *
+ *
Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property
On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
On Thu, 29 Jun 2023 17:25:00 -0700
Jessica Zhang wrote:
Document and add support for solid_fill property to drm_plane. In
addition, add support for setting and getting the values for solid_fill.
To enable solid fill planes, userspace must assign a property blob to
the "solid_fill" plane property containing the following information:
struct drm_solid_fill_info {
u8 version;
u32 r, g, b;
};
Signed-off-by: Jessica Zhang
Hi Jessica,
I've left some general UAPI related comments here.
---
drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
drivers/gpu/drm/drm_atomic_uapi.c | 55 +++
drivers/gpu/drm/drm_blend.c | 33 +++
include/drm/drm_blend.h | 1 +
include/drm/drm_plane.h | 43
5 files changed, 141 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..fe14be2bd2b2 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -253,6 +253,11 @@ 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;
+ if (plane_state->solid_fill_blob) {
+ drm_property_blob_put(plane_state->solid_fill_blob);
+ plane_state->solid_fill_blob = NULL;
+ }
+
if (plane->color_encoding_property) {
if (!drm_object_property_get_default_value(&plane->base,
plane->color_encoding_property,
@@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct
drm_plane *plane,
if (state->fb)
drm_framebuffer_get(state->fb);
+ if (state->solid_fill_blob)
+ drm_property_blob_get(state->solid_fill_blob);
+
state->fence = NULL;
state->commit = NULL;
state->fb_damage_clips = NULL;
@@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct
drm_plane_state *state)
drm_crtc_commit_put(state->commit);
drm_property_blob_put(state->fb_damage_clips);
+ drm_property_blob_put(state->solid_fill_blob);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d867e7f9f2cd..a28b4ee79444 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct
drm_connector_state *conn_state,
}
EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
+static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
+ struct drm_property_blob *blob)
+{
+ int ret = 0;
+ int blob_version;
+
+ if (blob == state->solid_fill_blob)
+ return 0;
+
+ drm_property_blob_put(state->solid_fill_blob);
+ state->solid_fill_blob = NULL;
Is it ok to destroy old state before it is guaranteed that the new
state is accepted?
Hi Pekka,
Good point. I'll change this behavior so that an error case will keep
the old solid_fill_blob value.
+
+ memset(&state->solid_fill, 0, sizeof(state->solid_fill));
+
+ if (blob) {
+ struct drm_solid_fill_info *user_info = (struct
drm_solid_fill_info *)blob->data;
+
+ if (blob->length != sizeof(struct drm_solid_fill_info)) {
+ drm_dbg_atomic(state->plane->dev,
+ "[PLANE:%d:%s] bad solid fill blob length:
%zu\n",
+ state->plane->base.id,
state->plane->name,
+ blob->length);
+ return -EINVAL;
+ }
+
+ blob_version = user_info->version;
+
+ /* Add more versions if necessary */
+ if (blob_version == 1) {
+ state->solid_fill.r = user_info->r;
+ state->solid_fill.g = user_info->g;
+ state->solid_fill.b = user_info->b;
+ } else {
+ drm_dbg_atomic(state->plane->dev,
+ "[PLANE:%d:%s] failed to set solid fill
(ret=%d)\n",
+ state->plane->base.id,
state->plane->name,
+ ret);
+ return -EINVAL;
Btw. how does the atomic check machinery work here?
I expect that a TEST_ONLY atomic commit will do all the above checks
and return failure if anything is not right. Right?
Correct, drm_atomic_set_property() will still be called for a TEST_ONLY
commit, so these checks will still happen and return an error (or set
the property) when appropri
