Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property
On Mon, 10 Jul 2023 16:12:06 -0700
Jessica Zhang wrote:
> 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/include/drm/drm_blend.h b/include/drm/drm_blend.h
> >> index 88bdfec3bd88..0338a860b9c8 100644
> >> --- a/include/drm/drm_blend.h
> >> +++ b/include/drm/drm_blend.h
> >> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
> >> struct drm_atomic_state *state);
> >> int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> >> unsigned int supported_modes);
> >> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
> >> #endif
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 51291983ea44..f6ab313cb83e 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
> >>DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
> >> };
> >>
> >> +/**
> >> + * struct drm_solid_fill_info - User info for solid fill planes
> >> + */
> >> +struct drm_solid_fill_info {
> >> + __u8 version;
> >> + __u32 r, g, b;
> >> +};
> >
> > Shouldn't UAPI structs be in UAPI headers?
>
> Acked, will move this to uapi/drm_mode.h
>
> >
> > Shouldn't UAPI structs use explicit padding to not leave any gaps when
> > it's unavoidable? And the kernel to check that the gaps are indeed
> > zeroed?
>
> I don't believe so... From my understanding, padding will be taken care
> of by the compiler by default. Looking at the drm_mode_modeinfo UAPI
> struct [1], it also doesn't seem to do explicit padding. And the
> corresponding set_property() code doesn't seem check the gaps either.
>
> That being said, it's possible that I'm missing something here, so
> please let me know if that's the case.
>
> [1]
> https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242
I suspect that drm_mode_modeinfo predates the lessons learnt about
"botching up ioctls" by many years:
https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst
drm_mode_modeinfo goes all the way back to
commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef
Date: Fri Nov 7 14:05:41 2008 -0800
DRM: add mode setting support
and
commit e0c8463a8b00b467611607df0ff369d062528875
Date: Fri Dec 19 14:50:50 2008 +1000
drm: sanitise drm modesetting API + remove unused hotplug
and it got the proper types later in
commit 1d7f83d5ad6c30b385ba549c1c3a287cc872b7ae
Date: Thu Feb 26 00:51:42 2009 +0100
make drm headers use strict integer types
My personal feeling is that if you cannot avoid padding in a struct,
convert it into explicit fields anyway and require them to be zero.
That way if you ever need to extend or modify the struct, you already
have an "unused" field that old userspace guarantees to be zero, so you
can re-purpose it when it's not zero.
A struct for blob contents is maybe needing slightly less forward
planning than ioctl struct, because KMS properties are cheap compared
to ioctl numbers, I believe.
Maybe eliminating compiler induced padding in structs is not strictly
necessary, but it seems like a good idea to me, because compilers are
allowed to leave the padding bits undefined. If a struct was filled in
by the kernel and delivered to userspace, undefined padding could even
be a security leak, theoretically.
Anyway, don't take my word for it. Maybe kernel devs have a different
style.
Thanks,
pq
> >
> > It also needs more UAPI doc, like a link to the property doc that uses
> > this and an explanation of what the values mean.
>
> Acked.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >
> > Thanks,
> > pq
> >
> >> +
> >> +/**
> >> + * struct solid_fill_property - RGB values for solid fill plane
> >> + *
> >> + * Note: This is the V1 for this feature
> >> + */
> >> +struct drm_solid_fill {
> >> + uint32_t r;
> >> + uint32_t g;
> >> + uint32_t b;
> >> +};
> >> +
> >> /**
> >>* struct drm_plane_state
Re: [PATCH v5 6/6] drm/doc: Define KMS atomic state set
On Fri, Jul 07, 2023 at 07:40:59PM -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(+) > > 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. Small clarification: The kernel indeed tries very hard to make redundant changes a no-op, and I think we should consider any issues here bugs. But it still has to check, which means it needs to acquire the right locks and put in the right (cross-crtc) synchronization points, and due to implmentation challenges it's very hard to try to avoid that in all cases. So adding redundant changes especially across crtc (and their connected planes/connectors) might result in some oversynchronization issues, and userspace should therefore avoid them if feasible. With some sentences added to clarify this: Reviewed-by: Daniel Vetter > + > +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. > -- > 2.41.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: XDC 2023: Registration & Call for Proposals now open!
New reminder that the XDC 2023 Registration and Call for Proposals are open till the end of this week. The deadline is July 17. Please submit your proposals ASAP! You can find more information about XDC 2023 in previous emails below. Thanks again for your attention, -Ricardo On Tue, 2023-06-27 at 16:24 +0200, Ricardo Garcia wrote: > This is a reminder that the XDC 2023 Registration and the Call for > Proposals are still open and will be open for about 2 more weeks. Do not > forget to submit your proposals before the deadline (July 17)! > > The conference will take place in A Coruña this year, from October 17 to > 19. You can find more information about the conference in the links > below and you can also follow us on Mastodon for the latest updates. > > https://floss.social/@XOrgDevConf > > Thanks for your attention, > -Ricardo > > On Mon, 2023-04-17 at 13:41 +0200, Samuel Iglesias Gonsálvez wrote: > > > > Hello! > > > > Registration & Call for Proposals are now open for XDC 2023, which will > > take place on October 17-19, 2023. > > > > https://xdc2023.x.org > > > > As usual, the conference is free of charge and open to the general > > public. If you plan on attending, please make sure to register as early > > as possible! > > > > In order to register as attendee, you will therefore need to register > > via the XDC website. > > > > https://indico.freedesktop.org/event/4/registrations/ > > > > In addition to registration, the CfP is now open for talks, workshops > > and demos at XDC 2023. While any serious proposal will be gratefully > > considered, topics of interest to X.Org and freedesktop.org developers > > are encouraged. The program focus is on new development, ongoing > > challenges and anything else that will spark discussions among > > attendees in the hallway track. > > > > We are open to talks across all layers of the graphics stack, from the > > kernel to desktop environments / graphical applications and about how > > to make things better for the developers who build them. Head to the > > CfP page to learn more: > > > > https://indico.freedesktop.org/event/4/abstracts/ > > > > The deadline for submissions is Monday, 17 July 2023 (23:59 CEST) > > > > Check out our Reimbursement Policy to accept speaker expenses: > > > > https://www.x.org/wiki/XorgFoundation/Policies/Reimbursement/ > > > > If you have any questions, please send me an email to > > siglesias AT igalia.com, adding on Cc the X.org board (board > > at foundation.x.org). > > > > And please keep in mind, you can follow us on Twitter for all the latest > > updates and to stay connected: > > > > https://twitter.com/XOrgDevConf > > > > Best, > > > > Sam > > > > >
Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property
On 7/11/2023 12:42 AM, Pekka Paalanen wrote:
On Mon, 10 Jul 2023 16:12:06 -0700
Jessica Zhang wrote:
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/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 88bdfec3bd88..0338a860b9c8 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
struct drm_atomic_state *state);
int drm_plane_create_blend_mode_property(struct drm_plane *plane,
unsigned int supported_modes);
+int drm_plane_create_solid_fill_property(struct drm_plane *plane);
#endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 51291983ea44..f6ab313cb83e 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -40,6 +40,25 @@ enum drm_scaling_filter {
DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
};
+/**
+ * struct drm_solid_fill_info - User info for solid fill planes
+ */
+struct drm_solid_fill_info {
+ __u8 version;
+ __u32 r, g, b;
+};
Shouldn't UAPI structs be in UAPI headers?
Acked, will move this to uapi/drm_mode.h
Shouldn't UAPI structs use explicit padding to not leave any gaps when
it's unavoidable? And the kernel to check that the gaps are indeed
zeroed?
I don't believe so... From my understanding, padding will be taken care
of by the compiler by default. Looking at the drm_mode_modeinfo UAPI
struct [1], it also doesn't seem to do explicit padding. And the
corresponding set_property() code doesn't seem check the gaps either.
That being said, it's possible that I'm missing something here, so
please let me know if that's the case.
[1]
https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242
I suspect that drm_mode_modeinfo predates the lessons learnt about
"botching up ioctls" by many years:
https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst
drm_mode_modeinfo goes all the way back to
commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef
Date: Fri Nov 7 14:05:41 2008 -0800
DRM: add mode setting support
and
commit e0c8463a8b00b467611607df0ff369d062528875
Date: Fri Dec 19 14:50:50 2008 +1000
drm: sanitise drm modesetting API + remove unused hotplug
and it got the proper types later in
commit 1d7f83d5ad6c30b385ba549c1c3a287cc872b7ae
Date: Thu Feb 26 00:51:42 2009 +0100
make drm headers use strict integer types
My personal feeling is that if you cannot avoid padding in a struct,
convert it into explicit fields anyway and require them to be zero.
That way if you ever need to extend or modify the struct, you already
have an "unused" field that old userspace guarantees to be zero, so you
can re-purpose it when it's not zero.
A struct for blob contents is maybe needing slightly less forward
planning than ioctl struct, because KMS properties are cheap compared
to ioctl numbers, I believe.
Maybe eliminating compiler induced padding in structs is not strictly
necessary, but it seems like a good idea to me, because compilers are
allowed to leave the padding bits undefined. If a struct was filled in
by the kernel and delivered to userspace, undefined padding could even
be a security leak, theoretically.
Anyway, don't take my word for it. Maybe kernel devs have a different
style.
Ah, got it. Thanks for the info! Looking over more recent
implementations of blob properties, I do see that there's a precedent
for explicit padding [1].
I think I could also just make `version` a __u32 instead. Either way,
that seems to be how other structs declare `version`.
Thanks,
Jessica Zhang
[1]
https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/virtgpu_drm.h#L178
Thanks,
pq
It also needs more UAPI doc, like a link to the property doc that uses
this and an explanation of what the values mean.
Acked.
Thanks,
Jessica Zhang
Thanks,
pq
+
+/**
+ * struct solid_fill_property - RGB values for solid fill plane
+ *
+ * Note: This is the V1 for this feature
+ */
+struct drm_solid_fill {
+
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
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?
Thanks,
Jessica Zhang
@@ -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
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
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.
@@ -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(
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: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Wed, 12 Jul 2023 at 01:42, Abhinav Kumar wrote:
>
>
>
> 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 pi
Re: [PATCH RFC v4 7/7] drm/msm/dpu: Use DRM solid_fill property
On 7/3/2023 12:42 AM, Pekka Paalanen wrote:
On Fri, 30 Jun 2023 11:26:49 +0300
Pekka Paalanen wrote:
On Thu, 29 Jun 2023 17:25:06 -0700
Jessica Zhang wrote:
Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
determine if the plane is solid fill. In addition drop the DPU plane
color_fill field as we can now use drm_plane_state.solid_fill instead,
and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
allow userspace to configure the alpha value for the solid fill color.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 4476722f03bb..11d4fb771a1f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -42,7 +42,6 @@
#define SHARP_SMOOTH_THR_DEFAULT 8
#define SHARP_NOISE_THR_DEFAULT 2
-#define DPU_PLANE_COLOR_FILL_FLAG BIT(31)
#define DPU_ZPOS_MAX 255
/*
@@ -82,7 +81,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
- uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
const struct dpu_mdss_cfg *catalog;
@@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct
dpu_plane_state *pstate,
_dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation);
}
+static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)
+{
+ uint32_t ret = 0;
+
+ ret |= ((uint8_t) solid_fill.b) << 16;
+ ret |= ((uint8_t) solid_fill.g) << 8;
+ ret |= ((uint8_t) solid_fill.r);
solid_fill.r, g and b are uint32_t, yes?
What's the value encoding in the UAPI? That doc was missing.
Hi Pekka,
The solid fill blob will accept an RGB323232 value -- will document this
in the drm_solid_fill_info struct
I wouldn't expect the UAPI to use 32-bit variables if it was
essentially 8-bit, so this conversion looks wrong.
Nominal color value 1.0 in u8 is 0xff. The same in u32 is probably
0x? So a simple cast to u8 won't work. You'd want to take the
upper 8 bits instead.
Acked.
Thanks,
pq
+
+ return ret;
Btw. if your driver format is ABGR, then this function leaves alpha as
zero. That's confusing.
It would be nice to mention the exact pixel format in the function name
so the consistency is easier to check in both here and in callers.
Acked.
Thanks,
Jessica Zhang
Thanks,
pq
+}
+
/**
* _dpu_plane_color_fill - enables color fill on plane
* @pdpu: Pointer to DPU plane object
@@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane)
if (pdpu->is_error)
/* force white frame with 100% alpha pipe output on error */
_dpu_plane_color_fill(pdpu, 0xFF, 0xFF);
- else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
- /* force 100% alpha */
- _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+ else if (drm_plane_solid_fill_enabled(plane->state))
+ _dpu_plane_color_fill(pdpu,
_dpu_plane_get_fill_color(plane->state->solid_fill),
+ plane->state->alpha);
else {
dpu_plane_flush_csc(pdpu, &pstate->pipe);
dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
@@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane
*plane,
}
/* override for color fill */
- if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
+ if (drm_plane_solid_fill_enabled(plane->state)) {
_dpu_plane_set_qos_ctrl(plane, pipe, false);
/* skip remaining processing on color fill */
