Re: [PATCH RFC v4 7/7] drm/msm/dpu: Use DRM solid_fill property
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_DEFAULT2
> >
> > -#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.
>
> 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.
>
>
> 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.
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 */
> >
>
pgpnIFUgxZcDV.pgp
Description: OpenPGP digital signature
Re: [PATCH v4 6/6] drm/doc: Define KMS atomic state set
On Fri, 30 Jun 2023 23:09:17 -0300 André Almeida wrote: > Specify how the atomic state is maintained between userspace and > kernel, plus the special case for async flips. > > Signed-off-by: André Almeida > --- > v4: new patch > --- > Documentation/gpu/drm-uapi.rst | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst > index 65fb3036a580..5464376051cc 100644 > --- a/Documentation/gpu/drm-uapi.rst > +++ b/Documentation/gpu/drm-uapi.rst > @@ -486,3 +486,22 @@ and the CRTC index is its position in this array. > > .. kernel-doc:: include/uapi/drm/drm_mode.h > :internal: > + > +KMS atomic state > + > + > +If a userspace using the DRM atomic API would like to change the modeset, it s/modeset/video mode/ > +needs to do in an atomic way, changing all desired properties in a single > +commit. This applies any and all state changes, not only video modes. Plane configuration is a good example, where video mode does not change. > Following commits may contain the same properties again, as if they were > +new. The kernel can then judge if those properties requires modesetting and > real > +changes, or it's just the same state as before. In summary, userspace > commits do > +not need to set the minimal state as possible and can commit redundant > +information, and the kernel will ignore it. Maybe the whole paragraph should be more like... ahem, looks I got a bit carried away in writing this. Please, take what's useful, I'm not sure if any of this has been actually documented before: 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 observation must be made for atomic operations with > DRM_MODE_PAGE_FLIP_ASYNC. > +In such scenarios properties values can be sent, but the if they change > +something, the kernel will reject the flip. This is done because property > +changes can lead to modesetting, that would defeat the goal of flipping as > fast > +as possible. The only exceptions are the framebuffer ID to be flipped to and > +mode IDs changes, which could be referring to an identical mode, thus not > +requiring modeset. That's a bit... roundabout way to describe it. Doesn't async flip want to prevent all kinds of other-than-FB changes, not only the modesetting ones? Modesets are already gated by ALLOW_MODESET anyway. How about something like: 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. If you want to take these and need my Sob, that would be Signed-off-by: Pekka Paalanen Thanks, pq pgphpWyv99zRR.pgp Description: OpenPGP digital signature
[ANNOUNCE] wayland-protocols 1.32
wayland-protocols 1.32 is now available. This release includes 3 new staging protocols: * ext-foreign-toplevel-list - get information about toplevels, * security-context-v1 - allow race free identification of sandboxed clients, * cursor-shape-v1 - set cursor sprite using a shape enum instead of surface. The xdg-shell protocol also now has a 'suspended' toplevel state, usually sent when a toplevel is "out of sight" to the user, meant to communicate to a toplevel can for example take power saving measures. This release also include the usual set of straightening of question marks and clarifications of ambiguities. Enjoy! Daniel Stone (1): xdg-shell: Add suspended toplevel state David Redondo (1): xdg-activation: Clarify that the token stays valid if the object is destroyed Faith Ekstrand (1): Add a .mailmap file Isaac Freund (3): ext-session-lock-v1: use RFC 2119 key words ext-session-lock-v1: clarify to fix race ext-session-lock-v1: relicense to MIT Jonas Ådahl (4): xdg-output: Remove and tweak contradicting examples xdg-shell: Clarify that geometry doesn't automatically change xdg-shell: Clarify window geometry bounds build: Bump version to 1.32 Kirill Chibisov (1): stable/xdg-shell: clarify initial wl_surface acknowledgement Mikhail Gusarov (1): xdg-shell: Clarify relationship between [un]set_maximized and configure Pekka Paalanen (1): CI: bump ci-templates Philip Withnall (1): Fix typo in xdg-activation-v1 Simon Ser (9): Add merge request template for new protocols xdg-output-unstable-v1: deprecate name and description xdg-output: clarify goal ci: use detached CI pipelines ci: skip ci-fairy checks on main branch tablet-v2: fix typo in set_cursor serial description cursor-shape-v1: new protocol build: add Wayland subproject security-context-v1: new protocol Vlad Zahorodnii (1): linux-dmabuf: Fix a couple of typos Xaver Hugl (3): staging/drm-lease: clarify connector naming unstable/xdg-shell v6: clarify when which errors are used stable/xdg-shell: clarify when which protocol errors are used i509VCB (1): Add ext-foreign-toplevel-list protocol git tag: 1.32 https://gitlab.freedesktop.org/wayland/wayland-protocols/-/releases/1.32/downloads/wayland-protocols-1.32.tar.xz SHA256: 7459799d340c8296b695ef857c07ddef24c5a09b09ab6a74f7b92640d2b1ba11 wayland-protocols-1.32.tar.xz SHA512: 90bbd52daf342b98823ddeed04e349ae242d2eaf925ab8d603cceb36c980c83b5681bb890961e0d49584cb5c2e60a33abf8821770c6ab87956383630bd5b7966 wayland-protocols-1.32.tar.xz PGP: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/releases/1.32/downloads/wayland-protocols-1.32.tar.xz.sig signature.asc Description: PGP signature
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Fri, Jun 30, 2023 at 11:27 PM Jessica Zhang
wrote:
>
>
>
> On 6/30/2023 7:43 AM, Sebastian Wick wrote:
> > On Fri, Jun 30, 2023 at 2:26 AM 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.
> >>
> >> 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) {
> >> @@ -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.
> >> + *
> >> + * Possible values:
> >> + *
> >> + * "FB":
> >> + * Framebuffer source
> >> + *
> >> + * "COLOR":
> >> + * solid_fill source
> >> + *
> >>* 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).
> >> + *
> >> + * This creates a new property describing the current source of pixel
> >> data for the
> >> + * plane.
> >> + *
> >> + * The property is exposed to userspace as an enumeration property called
> >> + * "pixel_source" and has the following enumeration values:
> >> + *
> >> + * "FB":
> >> + * Framebuffer pixel source
> >> + *
> >> + * "COLOR":
> >> + * Solid fill color pixel source
> >
> > Can we add a "NONE" value?
> >
> > I know it has been discussed ear
Wayland Governance Meeting
This is a meeting annoucement. Members of wayland-protocols are trying to establish a semi-regular meeting to drive discussion on new protocols forward. The next one will happen on Wednesday, 5th of July, 6 PM CEST. Agenda: - input-method v3 protocol Apologies for the short notice, further meetings and doodles leading up to them will be published on the mailing list. Meeting minutes will be published afterwards. 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 v4 6/6] drm/doc: Define KMS atomic state set
Em 03/07/2023 05:38, Pekka Paalanen escreveu: On Fri, 30 Jun 2023 23:09:17 -0300 André Almeida wrote: Specify how the atomic state is maintained between userspace and kernel, plus the special case for async flips. Signed-off-by: André Almeida [...] If you want to take these and need my Sob, that would be Signed-off-by: Pekka Paalanen Thank you very much! Your version is way better than mine, I'll use it in my next version. Thanks, pq
