Re: 2024 X.Org Foundation Membership deadline for voting in the election
On Tue, 26 Mar 2024 11:42:48 -0400 Christopher Michael wrote: > The 2024 X.Org Foundation membership renewal period has been extended > one additional week and elections will start the following week on 01 > April 2024. > > Please note that only current members can vote in the upcoming election, > and that the deadline for new memberships or renewals to vote in the > upcoming election is 01 April 2024 at 23:59 UTC. > > If you are interested in joining the X.Org Foundation or in renewing > your membership, please visit the membership system site at: > https://members.x.org/ > > Christopher Michael, on behalf of the X.Org elections committee Hi everyone, given that the year's first email reminding everyone to renew their memberships was sent on Feb 7 when the renewal was NOT open yet, I wonder how many people thought they had already renewed and are now thinking they don't need to do anything? I fell for that: On Feb 7, I went to members.x.org to check my status, it said I was registered for "2023-2024" and there was no button to renew, so I closed the page confident that I was a member for 2024. After all, it said 2024. This was a mistake I realised only after being personally poked to renew. I know for sure of one other person falling for the same. Now, the members page for this year says "Application for the period: 02/2024-02/2025". Thanks to the people adding the month to reduce confusion. Thanks, pq pgpkC6Q9ExEVu.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v6 07/10] drm/atomic: Loosen FB atomic checks
On Mon, 28 Aug 2023 17:05:13 -0700
Jessica Zhang wrote:
> Loosen the requirements for atomic and legacy commit so that, in cases
> where pixel_source != FB, the commit can still go through.
>
> This includes adding framebuffer NULL checks in other areas to account for
> FB being NULL when non-FB pixel sources are enabled.
>
> To disable a plane, the pixel_source must be NONE or the FB must be NULL
> if pixel_source == FB.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic.c| 20 +++-
> drivers/gpu/drm/drm_atomic_helper.c | 36
> include/drm/drm_atomic_helper.h | 4 ++--
> include/drm/drm_plane.h | 29 +
> 4 files changed, 62 insertions(+), 27 deletions(-)
...
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index a58f84b6bd5e..4c5b7bcdb25c 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -992,6 +992,35 @@ static inline struct drm_plane *drm_plane_find(struct
> drm_device *dev,
> #define drm_for_each_plane(plane, dev) \
> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>
> +/**
> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on plane
> + * @state: plane state
> + *
> + * Returns:
> + * Whether the plane has been assigned a solid_fill_blob
> + */
> +static inline bool drm_plane_solid_fill_enabled(struct drm_plane_state
> *state)
> +{
> + if (!state)
> + return false;
> + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL &&
> state->solid_fill_blob;
> +}
> +
> +static inline bool drm_plane_has_visible_data(const struct drm_plane_state
> *state)
> +{
> + switch (state->pixel_source) {
> + case DRM_PLANE_PIXEL_SOURCE_NONE:
> + return false;
> + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL:
> + return state->solid_fill_blob != NULL;
This reminds me, new UAPI docs did not say what the requirements are for
choosing solid fill pixel source. Is the atomic commit rejected if
pixel source is solid fill, but solid_fill property has no blob?
This should be doc'd.
Thanks,
pq
> + case DRM_PLANE_PIXEL_SOURCE_FB:
> + default:
> + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB);
> + }
> +
> + return state->fb != NULL;
> +}
> +
> bool drm_any_plane_has_format(struct drm_device *dev,
> u32 format, u64 modifier);
>
>
pgpnBQJa4lnTk.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v6 05/10] drm/atomic: Add solid fill data to plane state dump
On Mon, 28 Aug 2023 17:05:11 -0700
Jessica Zhang wrote:
> Add solid_fill property data to the atomic plane state dump.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic.c | 4
> drivers/gpu/drm/drm_plane.c | 8
> include/drm/drm_plane.h | 3 +++
> 3 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index bcecb64ccfad..3cb599b3304a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -717,6 +717,10 @@ static void drm_atomic_plane_print_state(struct
> drm_printer *p,
> drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
> if (state->fb)
> drm_framebuffer_print_info(p, 2, state->fb);
> + drm_printf(p, "\tsolid_fill=%u\n",
> + state->solid_fill_blob ?
> state->solid_fill_blob->base.id : 0);
> + if (state->solid_fill_blob)
> + drm_plane_solid_fill_print_info(p, 2, state);
> drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
> drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src));
> drm_printf(p, "\trotation=%x\n", state->rotation);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 559d101162ba..6244b622a21a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1495,6 +1495,14 @@ __drm_plane_get_damage_clips(const struct
> drm_plane_state *state)
> state->fb_damage_clips->data : NULL);
> }
>
> +void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned int
> indent,
> + const struct drm_plane_state *state)
> +{
> + drm_printf_indent(p, indent, "r=0x%x\n", state->solid_fill.r);
> + drm_printf_indent(p, indent, "g=0x%x\n", state->solid_fill.g);
> + drm_printf_indent(p, indent, "b=0x%x\n", state->solid_fill.b);
I'd recommend %08x format, so that leading zeroes are explicit. That
makes it easier to see the difference between e.g. 0x and
0x0fff.
Thanks,
pq
> +}
> +
> /**
> * drm_plane_get_damage_clips - Returns damage clips.
> * @state: Plane state.
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 49995c4be2ab..a58f84b6bd5e 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -1001,6 +1001,9 @@ drm_plane_get_damage_clips_count(const struct
> drm_plane_state *state);
> struct drm_mode_rect *
> drm_plane_get_damage_clips(const struct drm_plane_state *state);
>
> +void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned int
> indent,
> + const struct drm_plane_state *state);
> +
> int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
>unsigned int supported_filters);
>
>
pgpCVL2IUjmzq.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v6 03/10] drm: Add solid fill pixel source
On Mon, 28 Aug 2023 17:05:09 -0700
Jessica Zhang wrote:
> Add "SOLID_FILL" as a valid pixel source. If the pixel_source property is
> set to "SOLID_FILL", it will display data from the drm_plane "solid_fill"
> blob property.
>
> Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_blend.c | 10 +-
> include/drm/drm_plane.h | 1 +
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 273021cc21c8..1016a206ca0c 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -200,6 +200,9 @@
> * "FB":
> * Framebuffer source set by the "FB_ID" property.
> *
> + * "SOLID_FILL":
> + * Solid fill color source set by the "solid_fill" property.
> + *
> * solid_fill:
> * solid_fill is set up with drm_plane_create_solid_fill_property(). It
> * contains pixel data that drivers can use to fill a plane.
> @@ -638,6 +641,7 @@ EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> static const struct drm_prop_enum_list drm_pixel_source_enum_list[] = {
> { DRM_PLANE_PIXEL_SOURCE_NONE, "NONE" },
> { DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
> + { DRM_PLANE_PIXEL_SOURCE_SOLID_FILL, "SOLID_FILL" },
> };
>
> /**
> @@ -662,6 +666,9 @@ static const struct drm_prop_enum_list
> drm_pixel_source_enum_list[] = {
> * "FB":
> * Framebuffer pixel source
> *
> + * "SOLID_FILL":
> + * Solid fill color pixel source
> + *
> * Returns:
> * Zero on success, negative errno on failure.
> */
> @@ -671,7 +678,8 @@ int drm_plane_create_pixel_source_property(struct
> drm_plane *plane,
> struct drm_device *dev = plane->dev;
> struct drm_property *prop;
> static const unsigned int valid_source_mask =
> BIT(DRM_PLANE_PIXEL_SOURCE_FB) |
> -
> BIT(DRM_PLANE_PIXEL_SOURCE_NONE);
> +
> BIT(DRM_PLANE_PIXEL_SOURCE_NONE) |
> +
> BIT(DRM_PLANE_PIXEL_SOURCE_SOLID_FILL);
> int i;
>
> /* FB is supported by default */
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index a38e18bfb43e..49995c4be2ab 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -43,6 +43,7 @@ enum drm_scaling_filter {
> enum drm_plane_pixel_source {
> DRM_PLANE_PIXEL_SOURCE_NONE,
> DRM_PLANE_PIXEL_SOURCE_FB,
> + DRM_PLANE_PIXEL_SOURCE_SOLID_FILL,
> DRM_PLANE_PIXEL_SOURCE_MAX
> };
>
>
This UAPI:
Acked-by: Pekka Paalanen
Thanks,
pq
pgpMr0DOxFDEg.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v6 02/10] drm: Introduce solid fill DRM plane property
On Mon, 28 Aug 2023 17:05:08 -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_mode_solid_fill {
> u32 r, g, b;
> };
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 9
> drivers/gpu/drm/drm_atomic_uapi.c | 26 ++
> drivers/gpu/drm/drm_blend.c | 30 ++
> include/drm/drm_blend.h | 1 +
> include/drm/drm_plane.h | 36
> +++
> include/uapi/drm/drm_mode.h | 24 +
> 6 files changed, 126 insertions(+)
...
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index c3c57bae06b7..273021cc21c8 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -200,6 +200,10 @@
> * "FB":
> * Framebuffer source set by the "FB_ID" property.
> *
> + * solid_fill:
> + * solid_fill is set up with drm_plane_create_solid_fill_property(). It
> + * contains pixel data that drivers can use to fill a plane.
> + *
> * 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).
...
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 43691058d28f..1fd92886d66c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> char name[DRM_DISPLAY_MODE_LEN];
> };
>
> +/**
> + * struct drm_mode_solid_fill - User info for solid fill planes
> + *
> + * This is the userspace API solid fill information structure.
> + *
> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> + * property to a blob containing a single drm_mode_solid_fill struct
> populated with an RGB323232
> + * color and setting the pixel source to "SOLID_FILL".
> + *
> + * For information on the plane property, see
> drm_plane_create_solid_fill_property()
> + *
> + * @r: Red color value of single pixel
> + * @g: Green color value of single pixel
> + * @b: Blue color value of single pixel
> + * @pad: padding
Document that padding must be zero, and ensure userspace obeys that. If
there is ever need to re-purpose the pad field, requiring it to be zero
today makes re-purposing possible.
Alternatively, if it is likely that it might be used as alpha if
alpha-ful format is added, then it would make more sense to require it
to be 0x instead of zero. Then the kernel could internally
interpret it as alpha always without special-casing zero. Or, it could
be straight out called "alpha" to begin with, but document and verify
that it must be set to 0x which means opaque.
> + */
> +struct drm_mode_solid_fill {
> + __u32 r;
> + __u32 g;
> + __u32 b;
> + __u32 pad;
> +};
> +
> +
> struct drm_mode_card_res {
> __u64 fb_id_ptr;
> __u64 crtc_id_ptr;
>
Looking good.
Thanks,
pq
pgpPzTOhaxwNV.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v6 01/10] drm: Introduce pixel_source DRM plane property
On Mon, 28 Aug 2023 17:05:07 -0700 Jessica Zhang wrote: > Add support for pixel_source property to drm_plane and related > documentation. In addition, force pixel_source to > DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break > legacy userspace. > > 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. > > Currently, the only pixel sources are DRM_PLANE_PIXEL_SOURCE_FB (the > default value) and DRM_PLANE_PIXEL_SOURCE_NONE. > > 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 | 90 > +++ > drivers/gpu/drm/drm_plane.c | 19 +-- > include/drm/drm_blend.h | 2 + > include/drm/drm_plane.h | 21 > 6 files changed, 133 insertions(+), 4 deletions(-) ... > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > index 6e74de833466..c3c57bae06b7 100644 > --- a/drivers/gpu/drm/drm_blend.c > +++ b/drivers/gpu/drm/drm_blend.c > @@ -185,6 +185,21 @@ > *plane does not expose the "alpha" property, then this is > *assumed to be 1.0 > * > + * pixel_source: > + * pixel_source is set up with drm_plane_create_pixel_source_property(). > + * It is used to toggle the active source of pixel data for the plane. > + * The plane will only display data from the set pixel_source -- any > + * data from other sources will be ignored. > + * > + * Possible values: > + * > + * "NONE": > + * No active pixel source. > + * Committing with a NONE pixel source will disable the plane. > + * > + * "FB": > + * Framebuffer source set by the "FB_ID" property. > + * > * 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). This UAPI: Acked-by: Pekka Paalanen Thanks, pq pgpIXA2uzDNp7.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Mon, 21 Aug 2023 17:30:21 +0300
Dmitry Baryshkov wrote:
> On Fri, 18 Aug 2023 at 16:55, Pekka Paalanen wrote:
> >
> > On Fri, 18 Aug 2023 14:03:14 +0300
> > Dmitry Baryshkov wrote:
> >
> > > On 18/08/2023 13:51, Pekka Paalanen wrote:
> > > > On Fri, 4 Aug 2023 16:59:00 +0300
> > > > Dmitry Baryshkov wrote:
> > > >
> > > >> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick
> > > >> wrote:
> > > >>>
> > > >>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> > > >>> wrote:
> > > >>>>
> > > >>>> On Fri, 28 Jul 2023 at 20:03, 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_mode_solid_fill {
> > > >>>>> u32 version;
> > > >>>>> u32 r, g, b;
> > > >>>>> };
> > > >>>>>
> > > >>>>> Signed-off-by: Jessica Zhang
> > > >>>>> ---
> > > >>>>> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> > > >>>>> drivers/gpu/drm/drm_atomic_uapi.c | 55
> > > >>>>> +++
> > > >>>>> drivers/gpu/drm/drm_blend.c | 30 +
> > > >>>>> include/drm/drm_blend.h | 1 +
> > > >>>>> include/drm/drm_plane.h | 35
> > > >>>>>
> > > >>>>> include/uapi/drm/drm_mode.h | 24 ++
> > > >>>>> 6 files changed, 154 insertions(+)
> > > >>>>>
> > > >>>>
> > > >>>> [skipped most of the patch]
> >
> > ...
> >
> > > >>> Maybe another COLOR_FILL enum value
> > > >>> with alpha might be better? Maybe just doing the alpha via the alpha
> > > >>> property is good enough.
> > > >>
> > > >> One of our customers has a use case for setting the opaque solid fill,
> > > >> while keeping the plane's alpha intact.
> > > >
> > > > Could you explain more about why they must keep plane alpha intact
> > > > instead of reprogramming everything with atomic? Is there some
> > > > combination that just cannot reach the same end result via userspace
> > > > manipulation of the solid fill values with plane alpha?
> > > >
> > > > Or is it a matter of userspace architecture where you have independent
> > > > components responsible for different KMS property values?
> >
> > > The latter one. The goal is to be able to switch between pixel sources
> > > without touching any additional properties (including plane's alpha
> > > value).
> >
> > Sorry, but that does not seem like a good justification for KMS UAPI
> > design.
> >
> > It is even in conflict with how atomic KMS UAPI was designed to work:
> > collect all your changes into a single commit, and push it at once.
> > Here we are talking about separate components changing the different
> > properties of the same KMS plane even. If you want to change both plane
> > opacity and contents, does it mean you need two refresh cycles, one at
> > a time? Could the two components be even racing with each other,
> > stalling each other randomly?
>
> Most likely I was not verbose enough.
>
> We want to setup the blending scene, including the FB and the solid
> fill properties for the plane. FB is set up in the RGBA format, each
> pixel having its own alpha value in addition to the plane's alpha
> value. Then under certain circumstances, the plane gets hidden by the
> solid fill (think of a curtain). We do not want to touch the global
> scene setup (including plane al
Re: [Freedreno] [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Fri, 18 Aug 2023 14:03:14 +0300
Dmitry Baryshkov wrote:
> On 18/08/2023 13:51, Pekka Paalanen wrote:
> > On Fri, 4 Aug 2023 16:59:00 +0300
> > Dmitry Baryshkov wrote:
> >
> >> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick
> >> wrote:
> >>>
> >>> On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> >>> wrote:
> >>>>
> >>>> On Fri, 28 Jul 2023 at 20:03, 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_mode_solid_fill {
> >>>>> u32 version;
> >>>>> u32 r, g, b;
> >>>>> };
> >>>>>
> >>>>> Signed-off-by: Jessica Zhang
> >>>>> ---
> >>>>> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> >>>>> drivers/gpu/drm/drm_atomic_uapi.c | 55
> >>>>> +++
> >>>>> drivers/gpu/drm/drm_blend.c | 30 +
> >>>>> include/drm/drm_blend.h | 1 +
> >>>>> include/drm/drm_plane.h | 35
> >>>>> include/uapi/drm/drm_mode.h | 24 ++
> >>>>> 6 files changed, 154 insertions(+)
> >>>>>
> >>>>
> >>>> [skipped most of the patch]
...
> >>> Maybe another COLOR_FILL enum value
> >>> with alpha might be better? Maybe just doing the alpha via the alpha
> >>> property is good enough.
> >>
> >> One of our customers has a use case for setting the opaque solid fill,
> >> while keeping the plane's alpha intact.
> >
> > Could you explain more about why they must keep plane alpha intact
> > instead of reprogramming everything with atomic? Is there some
> > combination that just cannot reach the same end result via userspace
> > manipulation of the solid fill values with plane alpha?
> >
> > Or is it a matter of userspace architecture where you have independent
> > components responsible for different KMS property values?
> The latter one. The goal is to be able to switch between pixel sources
> without touching any additional properties (including plane's alpha value).
Sorry, but that does not seem like a good justification for KMS UAPI
design.
It is even in conflict with how atomic KMS UAPI was designed to work:
collect all your changes into a single commit, and push it at once.
Here we are talking about separate components changing the different
properties of the same KMS plane even. If you want to change both plane
opacity and contents, does it mean you need two refresh cycles, one at
a time? Could the two components be even racing with each other,
stalling each other randomly?
Thanks,
pq
pgpfA5xjAsFdA.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Fri, 4 Aug 2023 16:59:00 +0300
Dmitry Baryshkov wrote:
> On Fri, 4 Aug 2023 at 16:44, Sebastian Wick wrote:
> >
> > On Fri, Aug 4, 2023 at 3:27 PM Dmitry Baryshkov
> > wrote:
> > >
> > > On Fri, 28 Jul 2023 at 20:03, 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_mode_solid_fill {
> > > > u32 version;
> > > > u32 r, g, b;
> > > > };
> > > >
> > > > Signed-off-by: Jessica Zhang
> > > > ---
> > > > drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> > > > drivers/gpu/drm/drm_atomic_uapi.c | 55
> > > > +++
> > > > drivers/gpu/drm/drm_blend.c | 30 +
> > > > include/drm/drm_blend.h | 1 +
> > > > include/drm/drm_plane.h | 35
> > > > include/uapi/drm/drm_mode.h | 24 ++
> > > > 6 files changed, 154 insertions(+)
> > > >
> > >
> > > [skipped most of the patch]
> > >
> > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > > index 43691058d28f..53c8efa5ad7f 100644
> > > > --- a/include/uapi/drm/drm_mode.h
> > > > +++ b/include/uapi/drm/drm_mode.h
> > > > @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> > > > char name[DRM_DISPLAY_MODE_LEN];
> > > > };
> > > >
> > > > +/**
> > > > + * struct drm_mode_solid_fill - User info for solid fill planes
> > > > + *
> > > > + * This is the userspace API solid fill information structure.
> > > > + *
> > > > + * Userspace can enable solid fill planes by assigning the plane
> > > > "solid_fill"
> > > > + * property to a blob containing a single drm_mode_solid_fill struct
> > > > populated with an RGB323232
> > > > + * color and setting the pixel source to "SOLID_FILL".
> > > > + *
> > > > + * For information on the plane property, see
> > > > drm_plane_create_solid_fill_property()
> > > > + *
> > > > + * @version: Version of the blob. Currently, there is only support for
> > > > version == 1
> > > > + * @r: Red color value of single pixel
> > > > + * @g: Green color value of single pixel
> > > > + * @b: Blue color value of single pixel
> > > > + */
> > > > +struct drm_mode_solid_fill {
> > > > + __u32 version;
> > > > + __u32 r;
> > > > + __u32 g;
> > > > + __u32 b;
> > >
> > > Another thought about the drm_mode_solid_fill uABI. I still think we
> > > should add alpha here. The reason is the following:
> > >
> > > It is true that we have drm_plane_state::alpha and the plane's
> > > "alpha" property. However it is documented as "the plane-wide opacity
> > > [...] It can be combined with pixel alpha. The pixel values in the
> > > framebuffers are expected to not be pre-multiplied by the global alpha
> > > associated to the plane.".
> > >
> > > I can imagine a use case, when a user might want to enable plane-wide
> > > opacity, set "pixel blend mode" to "Coverage" and then switch between
> > > partially opaque framebuffer and partially opaque solid-fill without
> > > touching the plane's alpha value.
> >
> > The only reason I see against this is that there might be some
> > hardware which supports only RGB but not alpha on planes and they
> > could then not use this property.
>
> Fair enough.
>
> > Maybe another COLOR_FILL enum value
> > with alpha might be better? Maybe just doing the alpha via the alpha
> > property is good enough.
>
> One of our customers has a use case for setting the opaque solid fill,
> while keeping the plane's alpha intact.
Could you explain more about why they must keep plane alpha intact
instead of reprogramming everything with atomic? Is there some
combination that just cannot reach the same end result via userspace
manipulation of the solid fill values with plane alpha?
Or is it a matter of userspace architecture where you have independent
components responsible for different KMS property values?
Thanks,
pq
pgpVbMklGc5IF.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Tue, 11 Jul 2023 15:42:28 -0700
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_proper
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, theoreti
Re: [Freedreno] [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 */
> >
>
pgpGLfbJDjoR1.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
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.
> >
> > 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.
>
> > @@ -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?
> > + *
> > + * Possible values:
Wouldn't hurt to explicitly mention here that this is an enum.
> > + *
> > + * "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".
Why "COLOR" and not, say, "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
Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property
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?
> +
> + 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?
> + }
> + state->solid_fill_blob = drm_property_blob_get(blob);
> + }
> +
> + return ret;
> +}
> +
> static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>
Re: [Freedreno] [PATCH RFC v4 7/7] drm/msm/dpu: Use DRM solid_fill property
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_FLAGBIT(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;
> +}
> +
> /**
> * _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 */
>
pgpXih8YBTDXZ.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v4 6/7] drm/msm/dpu: Allow NULL FBs in atomic commit
On Fri, 30 Jun 2023 03:52:37 +0300
Dmitry Baryshkov wrote:
> On 30/06/2023 03:25, Jessica Zhang wrote:
> > Since solid fill planes allow for a NULL framebuffer in a valid commit,
> > add NULL framebuffer checks to atomic commit calls within DPU.
> >
> > Signed-off-by: Jessica Zhang
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 45
> > +++
> > 2 files changed, 36 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 1edf2b6b0a26..d1b37d2cc202 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -451,6 +451,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> > *crtc,
> > struct drm_plane_state *state;
> > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> > struct dpu_plane_state *pstate = NULL;
> > + const struct msm_format *fmt;
> > struct dpu_format *format;
> > struct dpu_hw_ctl *ctl = mixer->lm_ctl;
> >
> > @@ -470,7 +471,13 @@ static void _dpu_crtc_blend_setup_mixer(struct
> > drm_crtc *crtc,
> > pstate = to_dpu_plane_state(state);
> > fb = state->fb;
> >
> > - format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> > + if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb)
> > + fmt = msm_framebuffer_format(pstate->base.fb);
> > + else
> > + fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
> > + DRM_FORMAT_RGBA, 0);
>
> The DRM_FORMAT_RGBA should be defined somewhere in patch 1 as format
> for the solid_fill, then that define can be used in this patch.
Isn't this just a driver-specific decision to convert a RGB323232
solid_fill to be presented as a RGBA?
Though, below there is ABGR used for something... inconsistent?
Thanks,
pq
>
> > +
> > + format = to_dpu_format(fmt);
> >
> > if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> > bg_alpha_enable = true;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 5f0984ce62b1..4476722f03bb 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -837,8 +837,10 @@ static int dpu_plane_atomic_check(struct drm_plane
> > *plane,
> >
> > pipe_cfg->dst_rect = new_plane_state->dst;
> >
> > - fb_rect.x2 = new_plane_state->fb->width;
> > - fb_rect.y2 = new_plane_state->fb->height;
> > + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB &&
> > new_plane_state->fb) {
> > + fb_rect.x2 = new_plane_state->fb->width;
> > + fb_rect.y2 = new_plane_state->fb->height;
> > + }
> >
> > /* Ensure fb size is supported */
> > if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH ||
> > @@ -848,10 +850,13 @@ static int dpu_plane_atomic_check(struct drm_plane
> > *plane,
> > return -E2BIG;
> > }
> >
> > - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> > -
> > max_linewidth = pdpu->catalog->caps->max_linewidth;
> >
> > + if (drm_plane_solid_fill_enabled(new_plane_state))
> > + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
> > + else
> > + fmt =
> > to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> > +
> > if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> > /*
> > * In parallel multirect case only the half of the usual width
> > @@ -1082,21 +1087,32 @@ static void dpu_plane_sspp_atomic_update(struct
> > drm_plane *plane)
> > struct drm_crtc *crtc = state->crtc;
> > struct drm_framebuffer *fb = state->fb;
> > bool is_rt_pipe;
> > - const struct dpu_format *fmt =
> > - to_dpu_format(msm_framebuffer_format(fb));
> > + const struct dpu_format *fmt;
> > struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg;
> > struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> > struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
> > struct msm_gem_address_space *aspace = kms->base.aspace;
> > struct dpu_hw_fmt_layout layout;
> > bool layout_valid = false;
> > - int ret;
> >
> > - ret = dpu_format_populate_layout(aspace, fb, &layout);
> > - if (ret)
> > - DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
> > - else
> > - layout_valid = true;
> > + if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) {
> > + int ret;
> > +
> > + fmt = to_dpu_format(msm_framebuffer_format(fb));
> > +
> > + ret = dpu_format_populate_layout(aspace, fb, &layout);
> > + if (ret)
> > + DPU_ERROR_PLANE(pdpu, "fai
Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
On Wed, 28 Jun 2023 09:40:21 -0700
Jessica Zhang wrote:
> On 6/28/2023 12:34 AM, Pekka Paalanen wrote:
> > On Tue, 27 Jun 2023 15:10:19 -0700
> > Abhinav Kumar wrote:
> >
> >> 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 e
Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
On Tue, 27 Jun 2023 15:10:19 -0700
Abhinav Kumar wrote:
> 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
Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
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.
Thanks,
pq
>
> Would be interested in any feedback on this.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >> In addition, loosen the NULL FB checks within the atomic commit callstack
> >> to allow a NULL FB when color_fill is nonzero 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.color_fill and
> >> drm_plane_state.color_fill_format 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 color_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 color_fill value by setting COLOR_FILL_FORMAT to a
> >> DRM format, setting COLOR_FILL to a color fill value, and setting the
> >> framebuffer to NULL.
> >
> > Is there some real reason for the format property? Ie. why not
> > just do what was the plan for the crttc background color and
> > specify the color in full 16bpc format and just pick as many
> > msbs from that as the hw can use?
> >
> >>
> >> Jessica Zhang (3):
> >>drm: Introduce color fill properties for drm plane
> >>drm: Adjust atomic checks for solid fill color
> >>drm/msm/dpu: Use color_fill property for DPU planes
>
Re: [Freedreno] [PATCH 00/36] drm/amd/display: add AMD driver-specific properties for color mgmt
On Tue, 30 May 2023 01:55:21 +0300 Dmitry Baryshkov wrote: > On 24/05/2023 01:14, Melissa Wen wrote: > > This series is a refined version of our RFC [1] for AMD driver-specific > > color management properties. It is a collection of contributions from > > Joshua, Harry and I to enhance AMD KMS color pipeline for Steam > > Deck/SteamOS by exposing the large set of color caps available in AMD > > display HW. > > > > Considering RFC feedback, this patchset differs from the previous one by > > removing the KConfig option and just guarding driver-specific properties > > with `AMD_PRIVATE_COLOR` - but we also removed the guards from internal > > elements and operations. We stopped to advertise CRTC shaper and 3D LUTs > > properties since they aren't in use in the Steam Deck color pipeline[2]. > > On the other hand, we keep mapping CRTC shaper and 3D LUTs (DM) to DC > > MPC setup. We also improved curve calculations to take into account HW > > color caps. > > > > In short, for pre-blending, we added the following properties: > > - plane degamma LUT and predefined transfer function; > > - plane HDR multiplier > > - plane shaper LUT/transfer function; > > - plane 3D LUT; and finally, > > - plane blend LUT/transfer function, just before blending. > > This set of properties sounds interesting and not fully AMD-specific. > Could you please consider moving them to the more generic location? No, please see the following thread for plans for more generic UAPI: https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html AMD just needs something right now, so they try their own stuff first without exposing it to userspace. Thanks, pq > For the reference, MSM (Qualcomm) display hardware supports > degamma/gamma LUTs for planes too. One of the suggested usecases for > these degamma/gamma units is to support colorspace transfer functions. > > Thus, at least some of these properties can be implemented in drm/msm > driver too. > > > After blending, we already have DRM CRTC degamma/gamma LUTs and CTM, > > therefore, we extend post-blending color pipeline with CRTC gamma > > transfer function. > > > > The first three patches are on DRM KMS side. We expose DRM property > > helper for blob lookup and replacement so that we can use it for > > managing driver-specific properties. We add a tracked for plane color > > mgmt changes and increase the maximum number of properties to > > accommodate this expansion. > > > > The userspace case here is Gamescope which is the compositor for > > SteamOS. It's already using all of this functionality to implement its > > color management pipeline right now [3]. > > > > Current IGT tests kms_color and amdgpu/amd_color on DCN301 and DCN21 HW > > preserve the same results with and without the guard. > > > > Finally, I may have missed something, please let me know if that's the > > case. > > > > Best Regards, > > > > Melissa Wen > > > > [1] > > https://lore.kernel.org/dri-devel/20230423141051.702990-1-m...@igalia.com > > [2] > > https://github.com/ValveSoftware/gamescope/blob/master/src/docs/Steam%20Deck%20Display%20Pipeline.png > > [3] https://github.com/ValveSoftware/gamescope > > > > > > Harry Wentland (2): > >drm/amd/display: fix segment distribution for linear LUTs > >drm/amd/display: fix the delta clamping for shaper LUT > > > > Joshua Ashton (13): > >drm/amd/display: add plane degamma TF driver-specific property > >drm/amd/display: add plane HDR multiplier driver-specific property > >drm/amd/display: add plane blend LUT and TF driver-specific properties > >drm/amd/display: copy 3D LUT settings from crtc state to stream_update > >drm/amd/display: dynamically acquire 3DLUT resources for color changes > >drm/amd/display: add CRTC regamma TF support > >drm/amd/display: set sdr_ref_white_level to 80 for out_transfer_func > >drm/amd/display: add support for plane degamma TF and LUT properties > >drm/amd/display: add dc_fixpt_from_s3132 helper > >drm/adm/display: add HDR multiplier support > >drm/amd/display: handle empty LUTs in __set_input_tf > >drm/amd/display: add DRM plane blend LUT and TF support > >drm/amd/display: allow newer DC hardware to use degamma ROM for PQ/HLG > > > > Melissa Wen (21): > >drm/drm_mode_object: increase max objects to accommodate new color > > props > >drm/drm_property: make replace_property_blob_from_id a DRM helper > >drm/drm_plane: track color mgmt changes per plane > >drm/amd/display: add CRTC driver-specific property for gamma TF > >drm/amd/display: add plane driver-specific properties for degamma LUT > >drm/amd/display: add plane 3D LUT driver-specific properties > >drm/amd/display: add plane shaper LUT driver-specific properties > >drm/amd/display: add plane shaper TF driver-private property > >drm/amd/display: add comments to describe DM crtc color mgmt behavior > >drm/amd/display: encapsulate atomic regamma operation >
Re: [Freedreno] [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Thu, 16 Mar 2023 23:22:24 +0100 Sebastian Wick wrote: > On Thu, Mar 16, 2023 at 5:29 PM Rob Clark wrote: > > > > On Thu, Mar 16, 2023 at 2:26 AM Jonas Ådahl wrote: > > > > > > On Wed, Mar 15, 2023 at 09:19:49AM -0700, Rob Clark wrote: > > > > On Wed, Mar 15, 2023 at 6:53 AM Jonas Ådahl wrote: > > > > > > > > > > On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote: > > > > > > On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl > > > > > > wrote: > > > > > > > > > > > > > > On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote: > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > Add a way to hint to the fence signaler of an upcoming > > > > > > > > deadline, such as > > > > > > > > vblank, which the fence waiter would prefer not to miss. This > > > > > > > > is to aid > > > > > > > > the fence signaler in making power management decisions, like > > > > > > > > boosting > > > > > > > > frequency as the deadline approaches and awareness of missing > > > > > > > > deadlines > > > > > > > > so that can be factored in to the frequency scaling. > > > > > > > > > > > > > > > > v2: Drop dma_fence::deadline and related logic to filter > > > > > > > > duplicate > > > > > > > > deadlines, to avoid increasing dma_fence size. The > > > > > > > > fence-context > > > > > > > > implementation will need similar logic to track deadlines > > > > > > > > of all > > > > > > > > the fences on the same timeline. [ckoenig] > > > > > > > > v3: Clarify locking wrt. set_deadline callback > > > > > > > > v4: Clarify in docs comment that this is a hint > > > > > > > > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > > > > > > > > v6: More docs > > > > > > > > v7: Fix typo, clarify past deadlines > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > Reviewed-by: Christian König > > > > > > > > Acked-by: Pekka Paalanen > > > > > > > > Reviewed-by: Bagas Sanjaya > > > > > > > > --- > > > > > > > > > > > > > > Hi Rob! > > > > > > > > > > > > > > > Documentation/driver-api/dma-buf.rst | 6 +++ > > > > > > > > drivers/dma-buf/dma-fence.c | 59 > > > > > > > > > > > > > > > > include/linux/dma-fence.h| 22 +++ > > > > > > > > 3 files changed, 87 insertions(+) > > > > > > > > > > > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst > > > > > > > > b/Documentation/driver-api/dma-buf.rst > > > > > > > > index 622b8156d212..183e480d8cea 100644 > > > > > > > > --- a/Documentation/driver-api/dma-buf.rst > > > > > > > > +++ b/Documentation/driver-api/dma-buf.rst > > > > > > > > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations > > > > > > > > .. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > > > > :doc: fence signalling annotation > > > > > > > > > > > > > > > > +DMA Fence Deadline Hints > > > > > > > > + > > > > > > > > + > > > > > > > > +.. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > > > > + :doc: deadline hints > > > > > > > > + > > > > > > > > DMA Fences Functions Reference > > > > > > > > ~~ > > > > > > > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c > > > > > > > > b/drivers/dma-buf/dma-fence.c > > > > > > > > index 0de0482cd36e..f177c56269bb 100644 > > > > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > >
Re: [Freedreno] [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Fri, 17 Mar 2023 11:17:37 +0200 Pekka Paalanen wrote: > On Fri, 17 Mar 2023 11:09:21 +0200 > Pekka Paalanen wrote: > > > On Thu, 16 Mar 2023 23:22:24 +0100 > > Sebastian Wick wrote: > > > > Vblank can be really long, especially with VRR where the additional > > > time you get to finish the frame comes from making vblank longer. > > Btw. VRR extends front porch, not vblank. Need to correct myself too. vblank includes front porch, vsync does not. https://electronics.stackexchange.com/questions/166681/how-exactly-does-a-vga-cable-work Thanks, pq pgpYUaunRBg06.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Fri, 17 Mar 2023 11:09:21 +0200 Pekka Paalanen wrote: > On Thu, 16 Mar 2023 23:22:24 +0100 > Sebastian Wick wrote: > > Vblank can be really long, especially with VRR where the additional > > time you get to finish the frame comes from making vblank longer. Btw. VRR extends front porch, not vblank. Thanks, pq pgpVxuE8kGfuk.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v10 00/15] dma-fence: Deadline awareness
On Wed, 8 Mar 2023 07:52:51 -0800 Rob Clark wrote: > From: Rob Clark > > This series adds a deadline hint to fences, so realtime deadlines > such as vblank can be communicated to the fence signaller for power/ > frequency management decisions. > > This is partially inspired by a trick i915 does, but implemented > via dma-fence for a couple of reasons: > > 1) To continue to be able to use the atomic helpers > 2) To support cases where display and gpu are different drivers > > This iteration adds a dma-fence ioctl to set a deadline (both to > support igt-tests, and compositors which delay decisions about which > client buffer to display), and a sw_sync ioctl to read back the > deadline. IGT tests utilizing these can be found at: > > > https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline > > > v1: https://patchwork.freedesktop.org/series/93035/ > v2: Move filtering out of later deadlines to fence implementation > to avoid increasing the size of dma_fence > v3: Add support in fence-array and fence-chain; Add some uabi to > support igt tests and userspace compositors. > v4: Rebase, address various comments, and add syncobj deadline > support, and sync_file EPOLLPRI based on experience with perf/ > freq issues with clvk compute workloads on i915 (anv) > v5: Clarify that this is a hint as opposed to a more hard deadline > guarantee, switch to using u64 ns values in UABI (still absolute > CLOCK_MONOTONIC values), drop syncobj related cap and driver > feature flag in favor of allowing count_handles==0 for probing > kernel support. > v6: Re-work vblank helper to calculate time of _start_ of vblank, > and work correctly if the last vblank event was more than a > frame ago. Add (mostly unrelated) drm/msm patch which also > uses the vblank helper. Use dma_fence_chain_contained(). More > verbose syncobj UABI comments. Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > v7: Fix kbuild complaints about vblank helper. Add more docs. > v8: Add patch to surface sync_file UAPI, and more docs updates. > v9: Drop (E)POLLPRI support.. I still like it, but not essential and > it can always be revived later. Fix doc build warning. > v10: Update 11/15 to handle multiple CRTCs Hi Rob, it is very nice to keep revision numbers and list the changes in each patch. If I looked at series v8 last, and I now see series v10, and I look at a patch that lists changes done in v7, how do I know if that change was made between series v8 and v10 or earlier? At least in some previous revision, series might have been v8 and a patch have new changes listed as v5 (because it was the 5th time that one patch was changed) instead of v8. Am I expected to keep track of vN of each individual patch independently? Thanks, pq pgpQnQqkAF60p.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v8 08/16] dma-buf/sw_sync: Add fence deadline support
On Tue, 28 Feb 2023 14:58:12 -0800
Rob Clark wrote:
> From: Rob Clark
>
> This consists of simply storing the most recent deadline, and adding an
> ioctl to retrieve the deadline. This can be used in conjunction with
> the SET_DEADLINE ioctl on a fence fd for testing. Ie. create various
> sw_sync fences, merge them into a fence-array, set deadline on the
> fence-array and confirm that it is propagated properly to each fence.
>
> v2: Switch UABI to express deadline as u64
> v3: More verbose UAPI docs, show how to convert from timespec
> v4: Better comments, track the soonest deadline, as a normal fence
> implementation would, return an error if no deadline set.
>
> Signed-off-by: Rob Clark
> Reviewed-by: Christian König
> ---
> drivers/dma-buf/sw_sync.c| 81
> drivers/dma-buf/sync_debug.h | 2 +
> 2 files changed, 83 insertions(+)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 348b3a9170fa..f53071bca3af 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -52,12 +52,33 @@ struct sw_sync_create_fence_data {
> __s32 fence; /* fd of new fence */
> };
>
> +/**
> + * struct sw_sync_get_deadline - get the deadline hint of a sw_sync fence
> + * @deadline_ns: absolute time of the deadline
> + * @pad: must be zero
> + * @fence_fd:the sw_sync fence fd (in)
> + *
> + * Return the earliest deadline set on the fence. The timebase for the
> + * deadline is CLOCK_MONOTONIC (same as vblank). If there is no deadline
> + * set on the fence, this ioctl will return -ENOENT.
> + */
> +struct sw_sync_get_deadline {
> + __u64 deadline_ns;
> + __u32 pad;
> + __s32 fence_fd;
> +};
Sounds good.
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 6176e52ba2d7..2e0146d0bdbb 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -55,11 +55,13 @@ static inline struct sync_timeline
> *dma_fence_parent(struct dma_fence *fence)
> * @base: base fence object
> * @link: link on the sync timeline's list
> * @node: node in the sync timeline's tree
> + * @deadline: the most recently set fence deadline
Now it's the earliest deadline.
> */
> struct sync_pt {
> struct dma_fence base;
> struct list_head link;
> struct rb_node node;
> + ktime_t deadline;
> };
>
> extern const struct file_operations sw_sync_debugfs_fops;
Acked-by: Pekka Paalanen
Thanks,
pq
pgplxgMTKVCVN.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v8 05/16] dma-buf/sync_file: Surface sync-file uABI
On Tue, 28 Feb 2023 14:58:09 -0800
Rob Clark wrote:
> From: Rob Clark
>
> We had all of the internal driver APIs, but not the all important
> userspace uABI, in the dma-buf doc. Fix that. And re-arrange the
> comments slightly as otherwise the comments for the ioctl nr defines
> would not show up.
>
> Signed-off-by: Rob Clark
> ---
> Documentation/driver-api/dma-buf.rst | 10 ++--
> include/uapi/linux/sync_file.h | 35 +++-
> 2 files changed, 22 insertions(+), 23 deletions(-)
>
Sounds good.
Acked-by: Pekka Paalanen
Thanks,
pq
> diff --git a/Documentation/driver-api/dma-buf.rst
> b/Documentation/driver-api/dma-buf.rst
> index 183e480d8cea..ff3f8da296af 100644
> --- a/Documentation/driver-api/dma-buf.rst
> +++ b/Documentation/driver-api/dma-buf.rst
> @@ -203,8 +203,8 @@ DMA Fence unwrap
> .. kernel-doc:: include/linux/dma-fence-unwrap.h
> :internal:
>
> -DMA Fence uABI/Sync File
> -
> +DMA Fence Sync File
> +~~~
>
> .. kernel-doc:: drivers/dma-buf/sync_file.c
> :export:
> @@ -212,6 +212,12 @@ DMA Fence uABI/Sync File
> .. kernel-doc:: include/linux/sync_file.h
> :internal:
>
> +DMA Fence Sync File uABI
> +
> +
> +.. kernel-doc:: include/uapi/linux/sync_file.h
> + :internal:
> +
> Indefinite DMA Fences
> ~
>
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index ee2dcfb3d660..eced40c204d7 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -16,12 +16,16 @@
> #include
>
> /**
> - * struct sync_merge_data - data passed to merge ioctl
> + * struct sync_merge_data - SYNC_IOC_MERGE: merge two fences
> * @name:name of new fence
> * @fd2: file descriptor of second fence
> * @fence: returns the fd of the new fence to userspace
> * @flags: merge_data flags
> * @pad: padding for 64-bit alignment, should always be zero
> + *
> + * Creates a new fence containing copies of the sync_pts in both
> + * the calling fd and sync_merge_data.fd2. Returns the new fence's
> + * fd in sync_merge_data.fence
> */
> struct sync_merge_data {
> charname[32];
> @@ -34,8 +38,8 @@ struct sync_merge_data {
> /**
> * struct sync_fence_info - detailed fence information
> * @obj_name:name of parent sync_timeline
> -* @driver_name: name of driver implementing the parent
> -* @status: status of the fence 0:active 1:signaled <0:error
> + * @driver_name: name of driver implementing the parent
> + * @status: status of the fence 0:active 1:signaled <0:error
> * @flags: fence_info flags
> * @timestamp_ns:timestamp of status change in nanoseconds
> */
> @@ -48,14 +52,19 @@ struct sync_fence_info {
> };
>
> /**
> - * struct sync_file_info - data returned from fence info ioctl
> + * struct sync_file_info - SYNC_IOC_FILE_INFO: get detailed information on a
> sync_file
> * @name:name of fence
> * @status: status of fence. 1: signaled 0:active <0:error
> * @flags: sync_file_info flags
> * @num_fences number of fences in the sync_file
> * @pad: padding for 64-bit alignment, should always be zero
> - * @sync_fence_info: pointer to array of structs sync_fence_info with all
> + * @sync_fence_info: pointer to array of struct &sync_fence_info with all
> *fences in the sync_file
> + *
> + * Takes a struct sync_file_info. If num_fences is 0, the field is updated
> + * with the actual number of fences. If num_fences is > 0, the system will
> + * use the pointer provided on sync_fence_info to return up to num_fences of
> + * struct sync_fence_info, with detailed fence information.
> */
> struct sync_file_info {
> charname[32];
> @@ -76,23 +85,7 @@ struct sync_file_info {
> * no upstream users available.
> */
>
> -/**
> - * DOC: SYNC_IOC_MERGE - merge two fences
> - *
> - * Takes a struct sync_merge_data. Creates a new fence containing copies of
> - * the sync_pts in both the calling fd and sync_merge_data.fd2. Returns the
> - * new fence's fd in sync_merge_data.fence
> - */
> #define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct
> sync_merge_data)
> -
> -/**
> - * DOC: SYNC_IOC_FILE_INFO - get detailed information on a sync_file
> - *
> - * Takes a struct sync_file_info. If num_fences is 0, the field is updated
> - * with the actual number of fences. If num_fences is > 0, the system will
> - * use the pointer provided on sync_fence_info to return up to num_fences of
> - * struct sync_fence_info, with detailed fence information.
> - */
> #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>
> #endif /* _UAPI_LINUX_SYNC_H */
pgpVPqtS7gNBS.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v7 07/15] dma-buf/sw_sync: Add fence deadline support
On Mon, 27 Feb 2023 11:35:13 -0800
Rob Clark wrote:
> From: Rob Clark
>
> This consists of simply storing the most recent deadline, and adding an
> ioctl to retrieve the deadline. This can be used in conjunction with
> the SET_DEADLINE ioctl on a fence fd for testing. Ie. create various
> sw_sync fences, merge them into a fence-array, set deadline on the
> fence-array and confirm that it is propagated properly to each fence.
>
> v2: Switch UABI to express deadline as u64
> v3: More verbose UAPI docs, show how to convert from timespec
>
> Signed-off-by: Rob Clark
> Reviewed-by: Christian König
> ---
> drivers/dma-buf/sw_sync.c | 58 ++
> drivers/dma-buf/sync_debug.h | 2 ++
> include/uapi/linux/sync_file.h | 6 +++-
> 3 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 348b3a9170fa..3e2315ee955b 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -52,12 +52,28 @@ struct sw_sync_create_fence_data {
> __s32 fence; /* fd of new fence */
> };
>
> +/**
> + * struct sw_sync_get_deadline - get the deadline hint of a sw_sync fence
> + * @deadline_ns: absolute time of the deadline
> + * @pad: must be zero
> + * @fence_fd:the sw_sync fence fd (in)
> + *
> + * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
Hi,
the commit message explains this returns the "most recent" deadline,
but the doc here forgets to mention that. I suppose that means the
most recently set deadline and not the deadline furthest forward in
time (largest value).
Is "most recent" the appropriate behaviour when multiple deadlines have
been set? Would you not want the earliest deadline set so far instead?
What if none has been set?
> + */
> +struct sw_sync_get_deadline {
> + __u64 deadline_ns;
> + __u32 pad;
> + __s32 fence_fd;
> +};
> +
> #define SW_SYNC_IOC_MAGIC'W'
>
> #define SW_SYNC_IOC_CREATE_FENCE _IOWR(SW_SYNC_IOC_MAGIC, 0,\
> struct sw_sync_create_fence_data)
>
> #define SW_SYNC_IOC_INC _IOW(SW_SYNC_IOC_MAGIC, 1,
> __u32)
> +#define SW_SYNC_GET_DEADLINE _IOWR(SW_SYNC_IOC_MAGIC, 2, \
> + struct sw_sync_get_deadline)
>
> static const struct dma_fence_ops timeline_fence_ops;
>
> @@ -171,6 +187,13 @@ static void timeline_fence_timeline_value_str(struct
> dma_fence *fence,
> snprintf(str, size, "%d", parent->value);
> }
>
> +static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t
> deadline)
> +{
> + struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> +
> + pt->deadline = deadline;
> +}
> +
> static const struct dma_fence_ops timeline_fence_ops = {
> .get_driver_name = timeline_fence_get_driver_name,
> .get_timeline_name = timeline_fence_get_timeline_name,
> @@ -179,6 +202,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
> .release = timeline_fence_release,
> .fence_value_str = timeline_fence_value_str,
> .timeline_value_str = timeline_fence_timeline_value_str,
> + .set_deadline = timeline_fence_set_deadline,
> };
>
> /**
> @@ -387,6 +411,37 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj,
> unsigned long arg)
> return 0;
> }
>
> +static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned
> long arg)
> +{
> + struct sw_sync_get_deadline data;
> + struct dma_fence *fence;
> + struct sync_pt *pt;
> +
> + if (copy_from_user(&data, (void __user *)arg, sizeof(data)))
> + return -EFAULT;
> +
> + if (data.deadline_ns || data.pad)
> + return -EINVAL;
> +
> + fence = sync_file_get_fence(data.fence_fd);
> + if (!fence)
> + return -EINVAL;
> +
> + pt = dma_fence_to_sync_pt(fence);
> + if (!pt)
> + return -EINVAL;
> +
> +
> + data.deadline_ns = ktime_to_ns(pt->deadline);
> +
> + dma_fence_put(fence);
> +
> + if (copy_to_user((void __user *)arg, &data, sizeof(data)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> static long sw_sync_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -399,6 +454,9 @@ static long sw_sync_ioctl(struct file *file, unsigned int
> cmd,
> case SW_SYNC_IOC_INC:
> return sw_sync_ioctl_inc(obj, arg);
>
> + case SW_SYNC_GET_DEADLINE:
> + return sw_sync_ioctl_get_deadline(obj, arg);
> +
> default:
> return -ENOTTY;
> }
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 6176e52ba2d7..2e0146d0bdbb 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -55,11 +55,13 @@ static inline struct sync_timeline
> *dma_fence_parent(struct dma_fence *fence)
> * @base: base fence object
> * @link: link on the sync timeline's list
> * @node: node
Re: [Freedreno] [PATCH v7 06/15] dma-buf/sync_file: Support (E)POLLPRI
On Mon, 27 Feb 2023 11:35:12 -0800
Rob Clark wrote:
> From: Rob Clark
>
> Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent
> wait (as opposed to a "housekeeping" wait to know when to cleanup after
> some work has completed). Usermode components of GPU driver stacks
> often poll() on fence fd's to know when it is safe to do things like
> free or reuse a buffer, but they can also poll() on a fence fd when
> waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag
> lets the kernel differentiate these two cases.
>
> Signed-off-by: Rob Clark
> ---
> drivers/dma-buf/sync_file.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 418021cfb87c..cbe96295373b 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file,
> poll_table *wait)
> {
> struct sync_file *sync_file = file->private_data;
>
> + /*
> + * The POLLPRI/EPOLLPRI flag can be used to signal that
> + * userspace wants the fence to signal ASAP, express this
> + * as an immediate deadline.
> + */
> + if (poll_requested_events(wait) & EPOLLPRI)
> + dma_fence_set_deadline(sync_file->fence, ktime_get());
Hi,
I don't think this doc will appear anywhere where it could be found,
maybe not in kernel HTML doc at all?
I also think this is not a good idea, but not my call.
Thanks,
pq
> +
> poll_wait(file, &sync_file->wq, wait);
>
> if (list_empty(&sync_file->cb.node) &&
pgp0kx6rGxhY6.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v7 05/15] dma-buf/sync_file: Add SET_DEADLINE ioctl
On Mon, 27 Feb 2023 11:35:11 -0800
Rob Clark wrote:
> From: Rob Clark
>
> The initial purpose is for igt tests, but this would also be useful for
> compositors that wait until close to vblank deadline to make decisions
> about which frame to show.
>
> The igt tests can be found at:
>
> https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline
>
> v2: Clarify the timebase, add link to igt tests
> v3: Use u64 value in ns to express deadline.
> v4: More doc
>
> Signed-off-by: Rob Clark
> ---
> drivers/dma-buf/dma-fence.c| 3 ++-
> drivers/dma-buf/sync_file.c| 19 +++
> include/uapi/linux/sync_file.h | 22 ++
> 3 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index e103e821d993..7761ceeae620 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -933,7 +933,8 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
> * the GPU's devfreq to reduce frequency, when in fact the opposite is
> what is
> * needed.
> *
> - * To this end, deadline hint(s) can be set on a &dma_fence via
> &dma_fence_set_deadline.
> + * To this end, deadline hint(s) can be set on a &dma_fence via
> &dma_fence_set_deadline
> + * (or indirectly via userspace facing ioctls like &SYNC_IOC_SET_DEADLINE).
> * The deadline hint provides a way for the waiting driver, or userspace, to
> * convey an appropriate sense of urgency to the signaling driver.
Hi,
when the kernel HTML doc is generated, I assume the above becomes a
link to "DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence", right?
> *
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index af57799c86ce..418021cfb87c 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
> return ret;
> }
>
> +static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
> + unsigned long arg)
> +{
> + struct sync_set_deadline ts;
> +
> + if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
> + return -EFAULT;
> +
> + if (ts.pad)
> + return -EINVAL;
> +
> + dma_fence_set_deadline(sync_file->fence, ns_to_ktime(ts.deadline_ns));
> +
> + return 0;
> +}
> +
> static long sync_file_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned
> int cmd,
> case SYNC_IOC_FILE_INFO:
> return sync_file_ioctl_fence_info(sync_file, arg);
>
> + case SYNC_IOC_SET_DEADLINE:
> + return sync_file_ioctl_set_deadline(sync_file, arg);
> +
> default:
> return -ENOTTY;
> }
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index ee2dcfb3d660..49325cf6749b 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -67,6 +67,21 @@ struct sync_file_info {
> __u64 sync_fence_info;
> };
>
> +/**
> + * struct sync_set_deadline - set a deadline hint on a fence
> + * @deadline_ns: absolute time of the deadline
Is it legal to pass zero as deadline_ns?
> + * @pad: must be zero
> + *
> + * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
Does something here provide doc links to "DOC: SYNC_IOC_SET_DEADLINE -
set a deadline on a fence" and to the "DOC: deadline hints"?
> + */
> +struct sync_set_deadline {
> + __u64 deadline_ns;
> + /* Not strictly needed for alignment but gives some possibility
> + * for future extension:
> + */
> + __u64 pad;
> +};
> +
> #define SYNC_IOC_MAGIC '>'
>
> /**
> @@ -95,4 +110,11 @@ struct sync_file_info {
> */
> #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>
> +/**
> + * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
> + *
> + * Allows userspace to set a deadline on a fence, see
> dma_fence_set_deadline()
Does something here provide doc links to struct sync_set_deadline and
to the "DOC: deadline hints"?
> + */
> +#define SYNC_IOC_SET_DEADLINE_IOW(SYNC_IOC_MAGIC, 5, struct
> sync_set_deadline)
> +
> #endif /* _UAPI_LINUX_SYNC_H */
With all those links added:
Acked-by: Pekka Paalanen
Thanks,
pq
pgpQ1QDPNtFre.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v7 01/15] dma-buf/dma-fence: Add deadline awareness
opes for the fence to be
> + *signaled
> + *
> + * Give the fence signaler a hint about an upcoming deadline, such as
> + * vblank, by which point the waiter would prefer the fence to be
> + * signaled by. This is intended to give feedback to the fence signaler
> + * to aid in power management decisions, such as boosting GPU frequency
> + * if a periodic vblank deadline is approaching but the fence is not
> + * yet signaled..
> + */
> +void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
> +{
> + if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
> + fence->ops->set_deadline(fence, deadline);
> +}
> +EXPORT_SYMBOL(dma_fence_set_deadline);
> +
> /**
> * dma_fence_describe - Dump fence describtion into seq_file
> * @fence: the 6fence to describe
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 775cdc0b4f24..87c0d846dbb4 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -257,6 +257,24 @@ struct dma_fence_ops {
>*/
> void (*timeline_value_str)(struct dma_fence *fence,
> char *str, int size);
> +
> + /**
> + * @set_deadline:
> + *
> + * Callback to allow a fence waiter to inform the fence signaler of
> + * an upcoming deadline, such as vblank, by which point the waiter
> + * would prefer the fence to be signaled by. This is intended to
> + * give feedback to the fence signaler to aid in power management
> + * decisions, such as boosting GPU frequency.
> + *
> + * This is called without &dma_fence.lock held, it can be called
> + * multiple times and from any context. Locking is up to the callee
> + * if it has some state to manage. If multiple deadlines are set,
> + * the expectation is to track the soonest one.
> + *
> + * This callback is optional.
> + */
> + void (*set_deadline)(struct dma_fence *fence, ktime_t deadline);
> };
>
> void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
> @@ -583,6 +601,8 @@ static inline signed long dma_fence_wait(struct dma_fence
> *fence, bool intr)
> return ret < 0 ? ret : 0;
> }
>
> +void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline);
> +
> struct dma_fence *dma_fence_get_stub(void);
> struct dma_fence *dma_fence_allocate_private_stub(void);
> u64 dma_fence_context_alloc(unsigned num);
This is exactly what I wanted to see. Already
Acked-by: Pekka Paalanen
Thanks,
pq
pgp1h5rM3FOH1.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Fri, 24 Feb 2023 11:44:53 -0800 Rob Clark wrote: > On Fri, Feb 24, 2023 at 2:24 AM Pekka Paalanen wrote: > > > > On Fri, 24 Feb 2023 09:41:46 + > > Tvrtko Ursulin wrote: > > > > > On 24/02/2023 09:26, Pekka Paalanen wrote: > > > > On Thu, 23 Feb 2023 10:51:48 -0800 > > > > Rob Clark wrote: > > > > > > > >> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen > > > >> wrote: > > > >>> > > > >>> On Wed, 22 Feb 2023 07:37:26 -0800 > > > >>> Rob Clark wrote: > > > >>> > > > >>>> On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > > > >>>> wrote: > > > > > > > > ... > > > > > > > >>>>> On another matter, if the application uses SET_DEADLINE with one > > > >>>>> timestamp, and the compositor uses SET_DEADLINE on the same thing > > > >>>>> with > > > >>>>> another timestamp, what should happen? > > > >>>> > > > >>>> The expectation is that many deadline hints can be set on a fence. > > > >>>> The fence signaller should track the soonest deadline. > > > >>> > > > >>> You need to document that as UAPI, since it is observable to > > > >>> userspace. > > > >>> It would be bad if drivers or subsystems would differ in behaviour. > > > >>> > > > >> > > > >> It is in the end a hint. It is about giving the driver more > > > >> information so that it can make better choices. But the driver is > > > >> even free to ignore it. So maybe "expectation" is too strong of a > > > >> word. Rather, any other behavior doesn't really make sense. But it > > > >> could end up being dictated by how the hw and/or fw works. > > > > > > > > It will stop being a hint once it has been implemented and used in the > > > > wild long enough. The kernel userspace regression rules make sure of > > > > that. > > > > > > Yeah, tricky and maybe a gray area in this case. I think we eluded > > > elsewhere in the thread that renaming the thing might be an option. > > > > > > So maybe instead of deadline, which is a very strong word, use something > > > along the lines of "present time hint", or "signalled time hint"? Maybe > > > reads clumsy. Just throwing some ideas for a start. > > > > You can try, but I fear that if it ever changes behaviour and > > someone notices that, it's labelled as a kernel regression. I don't > > think documentation has ever been the authoritative definition of UABI > > in Linux, it just guides drivers and userspace towards a common > > understanding and common usage patterns. > > > > So even if the UABI contract is not documented (ugh), you need to be > > prepared to set the UABI contract through kernel implementation. > > > > If you do not document the UABI contract, then different drivers are > > likely to implement it differently, leading to differing behaviour. > > Also userspace will invent wild ways to abuse the UABI if there is no > > documentation guiding it on proper use. If userspace or end users > > observe different behaviour, that's bad even if it's not a regression. > > > > I don't like the situation either, but it is what it is. UABI stability > > trumps everything regardless of whether it was documented or not. > > > > I bet userspace is going to use this as a "make it faster, make it > > hotter" button. I would not be surprised if someone wrote a LD_PRELOAD > > library that stamps any and all fences with an expired deadline to > > just squeeze out a little more through some weird side-effect. > > Userspace already has various (driver specific) debugfs/sysfs that it > can use if it wants to make it hotter and drain batteries faster, so > I'm not seeing a strong need to cater to the "turn it up to eleven" > crowd here. And really your point feels like a good reason to _not_ > document this as anything more than a hint. My point is that no matter what you say in documentation or leave unsaid, people can and will abuse this by the behaviour it provides anyway, like every other UABI. So why not just document what it is supposed to do? It cannot get any worse. Maybe you get lucky instead and people don't abuse it that much if they read the docs. E.g. can it affect GPU job scheduling, can it affect GPU clocks, can it affect power consumption, and so on. > Back in the real world, mobile games are already well aware of the fps > vs battery-life (and therefore gameplay) tradeoff. But what is > missing is a way to inform the kernel of userspace's intentions, so > that gpu dvfs can make intelligent decisions. This series is meant to > bridge that gap. Then document that. As long as you document it properly: what you expect it to be used for and how. Or if this is reserved strictly for Mesa drivers, then document that. You can also stop CC'ing me if you don't want attention to UABI docs. I don't read dri-devel@ unless I'm explicitly CC'd nowadays. Thanks, pq pgpeZOjTsYxQF.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Fri, 24 Feb 2023 10:50:51 + Tvrtko Ursulin wrote: > On 24/02/2023 10:24, Pekka Paalanen wrote: > > On Fri, 24 Feb 2023 09:41:46 + > > Tvrtko Ursulin wrote: > > > >> On 24/02/2023 09:26, Pekka Paalanen wrote: > >>> On Thu, 23 Feb 2023 10:51:48 -0800 > >>> Rob Clark wrote: > >>> > >>>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen > >>>> wrote: > >>>>> > >>>>> On Wed, 22 Feb 2023 07:37:26 -0800 > >>>>> Rob Clark wrote: > >>>>> > >>>>>> On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > >>>>>> wrote: > >>> > >>> ... > >>> > >>>>>>> On another matter, if the application uses SET_DEADLINE with one > >>>>>>> timestamp, and the compositor uses SET_DEADLINE on the same thing with > >>>>>>> another timestamp, what should happen? > >>>>>> > >>>>>> The expectation is that many deadline hints can be set on a fence. > >>>>>> The fence signaller should track the soonest deadline. > >>>>> > >>>>> You need to document that as UAPI, since it is observable to userspace. > >>>>> It would be bad if drivers or subsystems would differ in behaviour. > >>>>> > >>>> > >>>> It is in the end a hint. It is about giving the driver more > >>>> information so that it can make better choices. But the driver is > >>>> even free to ignore it. So maybe "expectation" is too strong of a > >>>> word. Rather, any other behavior doesn't really make sense. But it > >>>> could end up being dictated by how the hw and/or fw works. > >>> > >>> It will stop being a hint once it has been implemented and used in the > >>> wild long enough. The kernel userspace regression rules make sure of > >>> that. > >> > >> Yeah, tricky and maybe a gray area in this case. I think we eluded > >> elsewhere in the thread that renaming the thing might be an option. > >> > >> So maybe instead of deadline, which is a very strong word, use something > >> along the lines of "present time hint", or "signalled time hint"? Maybe > >> reads clumsy. Just throwing some ideas for a start. > > > > You can try, but I fear that if it ever changes behaviour and > > someone notices that, it's labelled as a kernel regression. I don't > > think documentation has ever been the authoritative definition of UABI > > in Linux, it just guides drivers and userspace towards a common > > understanding and common usage patterns. > > > > So even if the UABI contract is not documented (ugh), you need to be > > prepared to set the UABI contract through kernel implementation. > > To be the devil's advocate it probably wouldn't be an ABI regression but > just an regression. Same way as what nice(2) priorities mean hasn't > always been the same over the years, I don't think there is a strict > contract. > > Having said that, it may be different with latency sensitive stuff such > as UIs though since it is very observable and can be very painful to users. > > > If you do not document the UABI contract, then different drivers are > > likely to implement it differently, leading to differing behaviour. > > Also userspace will invent wild ways to abuse the UABI if there is no > > documentation guiding it on proper use. If userspace or end users > > observe different behaviour, that's bad even if it's not a regression. > > > > I don't like the situation either, but it is what it is. UABI stability > > trumps everything regardless of whether it was documented or not. > > > > I bet userspace is going to use this as a "make it faster, make it > > hotter" button. I would not be surprised if someone wrote a LD_PRELOAD > > library that stamps any and all fences with an expired deadline to > > just squeeze out a little more through some weird side-effect. > > > > Well, that's hopefully overboard in scaring, but in the end, I would > > like to see UABI documented so I can have a feeling of what it is for > > and how it was intended to be used. That's all. > > We share the same concern. If you read elsewhere in these threads you > will notice I have been calling this an "arms race". If the ability to > make yourself go faster does not required additional privilege I also > worry everyone will do it at which point it becomes pointless. So yes, I > do share this concern about exposing any of this as an unprivileged uapi. > > Is it possible to limit access to only compositors in some sane way? > Sounds tricky when dma-fence should be disconnected from DRM.. Maybe it's not that bad in this particular case, because we are talking only about boosting GPU clocks which benefits everyone (except battery life) and it does not penalize other programs like e.g. job priorities do. Drivers are not going to use the deadline for scheduling priorities, right? I don't recall seeing any mention of that. ...right? Thanks, pq pgpVo2EngByFO.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Fri, 24 Feb 2023 09:41:46 + Tvrtko Ursulin wrote: > On 24/02/2023 09:26, Pekka Paalanen wrote: > > On Thu, 23 Feb 2023 10:51:48 -0800 > > Rob Clark wrote: > > > >> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen > >> wrote: > >>> > >>> On Wed, 22 Feb 2023 07:37:26 -0800 > >>> Rob Clark wrote: > >>> > >>>> On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > >>>> wrote: > > > > ... > > > >>>>> On another matter, if the application uses SET_DEADLINE with one > >>>>> timestamp, and the compositor uses SET_DEADLINE on the same thing with > >>>>> another timestamp, what should happen? > >>>> > >>>> The expectation is that many deadline hints can be set on a fence. > >>>> The fence signaller should track the soonest deadline. > >>> > >>> You need to document that as UAPI, since it is observable to userspace. > >>> It would be bad if drivers or subsystems would differ in behaviour. > >>> > >> > >> It is in the end a hint. It is about giving the driver more > >> information so that it can make better choices. But the driver is > >> even free to ignore it. So maybe "expectation" is too strong of a > >> word. Rather, any other behavior doesn't really make sense. But it > >> could end up being dictated by how the hw and/or fw works. > > > > It will stop being a hint once it has been implemented and used in the > > wild long enough. The kernel userspace regression rules make sure of > > that. > > Yeah, tricky and maybe a gray area in this case. I think we eluded > elsewhere in the thread that renaming the thing might be an option. > > So maybe instead of deadline, which is a very strong word, use something > along the lines of "present time hint", or "signalled time hint"? Maybe > reads clumsy. Just throwing some ideas for a start. You can try, but I fear that if it ever changes behaviour and someone notices that, it's labelled as a kernel regression. I don't think documentation has ever been the authoritative definition of UABI in Linux, it just guides drivers and userspace towards a common understanding and common usage patterns. So even if the UABI contract is not documented (ugh), you need to be prepared to set the UABI contract through kernel implementation. If you do not document the UABI contract, then different drivers are likely to implement it differently, leading to differing behaviour. Also userspace will invent wild ways to abuse the UABI if there is no documentation guiding it on proper use. If userspace or end users observe different behaviour, that's bad even if it's not a regression. I don't like the situation either, but it is what it is. UABI stability trumps everything regardless of whether it was documented or not. I bet userspace is going to use this as a "make it faster, make it hotter" button. I would not be surprised if someone wrote a LD_PRELOAD library that stamps any and all fences with an expired deadline to just squeeze out a little more through some weird side-effect. Well, that's hopefully overboard in scaring, but in the end, I would like to see UABI documented so I can have a feeling of what it is for and how it was intended to be used. That's all. Thanks, pq pgptX5jK8Yo4f.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Thu, 23 Feb 2023 10:51:48 -0800 Rob Clark wrote: > On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen wrote: > > > > On Wed, 22 Feb 2023 07:37:26 -0800 > > Rob Clark wrote: > > > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > > > wrote: ... > > > > On another matter, if the application uses SET_DEADLINE with one > > > > timestamp, and the compositor uses SET_DEADLINE on the same thing with > > > > another timestamp, what should happen? > > > > > > The expectation is that many deadline hints can be set on a fence. > > > The fence signaller should track the soonest deadline. > > > > You need to document that as UAPI, since it is observable to userspace. > > It would be bad if drivers or subsystems would differ in behaviour. > > > > It is in the end a hint. It is about giving the driver more > information so that it can make better choices. But the driver is > even free to ignore it. So maybe "expectation" is too strong of a > word. Rather, any other behavior doesn't really make sense. But it > could end up being dictated by how the hw and/or fw works. It will stop being a hint once it has been implemented and used in the wild long enough. The kernel userspace regression rules make sure of that. See the topic of implementing triple-buffering in Mutter in order to put more work to the GPU in order to have the GPU ramp up clocks in order to not miss rendering deadlines. I don't think that patch set has landed in Mutter upstream, but I hear distributions in downstream are already carrying it. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1383 https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1441 Granted, GPU clocks are just one side of that story it seems, and triple-buffering may have other benefits. If SET_DEADLINE would fix that problem without triple-buffering, it is definitely userspace observable, expected and eventually required behaviour. Thanks, pq pgpvqVGbQ3KLU.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Wed, 22 Feb 2023 07:37:26 -0800 Rob Clark wrote: > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen wrote: > > > > On Tue, 21 Feb 2023 09:53:56 -0800 > > Rob Clark wrote: > > > > > On Tue, Feb 21, 2023 at 8:48 AM Luben Tuikov > > > wrote: > > > > > > > > On 2023-02-20 11:14, Rob Clark wrote: > > > > > On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen > > > > > wrote: > > > > >> > > > > >> On Sat, 18 Feb 2023 13:15:49 -0800 > > > > >> Rob Clark wrote: > > > > >> > > > > >>> From: Rob Clark > > > > >>> > > > > >>> Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an > > > > >>> urgent > > > > >>> wait (as opposed to a "housekeeping" wait to know when to cleanup > > > > >>> after > > > > >>> some work has completed). Usermode components of GPU driver stacks > > > > >>> often poll() on fence fd's to know when it is safe to do things like > > > > >>> free or reuse a buffer, but they can also poll() on a fence fd when > > > > >>> waiting to read back results from the GPU. The EPOLLPRI/POLLPRI > > > > >>> flag > > > > >>> lets the kernel differentiate these two cases. > > > > >>> > > > > >>> Signed-off-by: Rob Clark > > > > >> > > > > >> Hi, > > > > >> > > > > >> where would the UAPI documentation of this go? > > > > >> It seems to be missing. > > > > > > > > > > Good question, I am not sure. The poll() man page has a description, > > > > > but my usage doesn't fit that _exactly_ (but OTOH the description is a > > > > > bit vague). > > > > > > > > > >> If a Wayland compositor is polling application fences to know which > > > > >> client buffer to use in its rendering, should the compositor poll > > > > >> with > > > > >> PRI or not? If a compositor polls with PRI, then all fences from all > > > > >> applications would always be PRI. Would that be harmful somehow or > > > > >> would it be beneficial? > > > > > > > > > > I think a compositor would rather use the deadline ioctl and then poll > > > > > without PRI. Otherwise you are giving an urgency signal to the fence > > > > > signaller which might not necessarily be needed. > > > > > > > > > > The places where I expect PRI to be useful is more in mesa (things > > > > > like glFinish(), readpix, and other similar sorts of blocking APIs) > > > > Hi, > > > > > > > > Hmm, but then user-space could do the opposite, namely, submit work as > > > > usual--never > > > > using the SET_DEADLINE ioctl, and then at the end, poll using > > > > (E)POLLPRI. That seems > > > > like a possible usage pattern, unintended--maybe, but possible. Do we > > > > want to discourage > > > > this? Wouldn't SET_DEADLINE be enough? I mean, one can call > > > > SET_DEADLINE with the current > > > > time, and then wouldn't that be equivalent to (E)POLLPRI? > > > > > > Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is > > > slightly more convenient if you want an immediate deadline (single > > > syscall instead of two), but not strictly needed. OTOH it piggy-backs > > > on existing UABI. > > > > In that case, I would be conservative, and not add the POLLPRI > > semantics. An UAPI addition that is not strictly needed and somewhat > > unclear if it violates any design principles is best not done, until it > > is proven to be beneficial. > > > > Besides, a Wayland compositor does not necessary need to add the fd > > to its main event loop for poll. It could just SET_DEADLINE, and then > > when it renders simply check if the fence passed or not already. Not > > polling means the compositor does not need to wake up at the moment the > > fence signals to just record a flag. > > poll(POLLPRI) isn't intended for wayland.. but is a thing I want in > mesa for fence waits. I _could_ use SET_DEADLINE but it is two > syscalls and correspondingly more code ;-) But is it actually beneficial? "More code" seems quite irrelevant. Would there be a hundred or more of those per frame? Or would it be always limited to one or two? Or totally depend on what the application is doing? Is it a significant impact? > > On another matter, if the application uses SET_DEADLINE with one > > timestamp, and the compositor uses SET_DEADLINE on the same thing with > > another timestamp, what should happen? > > The expectation is that many deadline hints can be set on a fence. > The fence signaller should track the soonest deadline. You need to document that as UAPI, since it is observable to userspace. It would be bad if drivers or subsystems would differ in behaviour. Thanks, pq pgp7aIX0EMl5C.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Tue, 21 Feb 2023 09:25:18 -0800
Rob Clark wrote:
> On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen wrote:
> >
> > On Mon, 20 Feb 2023 12:18:56 -0800
> > Rob Clark wrote:
> >
> > > From: Rob Clark
> > >
> > > Add a new flag to let userspace provide a deadline as a hint for syncobj
> > > and timeline waits. This gives a hint to the driver signaling the
> > > backing fences about how soon userspace needs it to compete work, so it
> > > can addjust GPU frequency accordingly. An immediate deadline can be
> > > given to provide something equivalent to i915 "wait boost".
> > >
> > > v2: Use absolute u64 ns value for deadline hint, drop cap and driver
> > > feature flag in favor of allowing count_handles==0 as a way for
> > > userspace to probe kernel for support of new flag
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/drm_syncobj.c | 59 +++
> > > include/uapi/drm/drm.h| 5 +++
> > > 2 files changed, 51 insertions(+), 13 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > index 642808520d92..aefc8cc743e0 100644
> > > --- a/include/uapi/drm/drm.h
> > > +++ b/include/uapi/drm/drm.h
> > > @@ -887,6 +887,7 @@ struct drm_syncobj_transfer {
> > > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
> > > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
> > > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time
> > > point to become available */
> > > +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE (1 << 3) /* set fence
> > > deadline based to deadline_nsec/sec */
> >
> > Hi,
> >
> > where is the UAPI documentation explaining what is a "fence deadline"
> > and what setting it does here?
>
> It's with the rest of the drm_syncobj UAPI docs ;-)
Is that
https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#dma-fence-uabi-sync-file
?
That whole page never mentions e.g. WAIT_AVAILABLE, so at least the
flags are not there. Does not mention syncobj_wait either.
I could ask where the real non-IGT userspace is or the plan for it,
too, since this is new DRM UAPI.
Thanks,
pq
>
> BR,
> -R
>
> > btw. no nsec/sec anymore.
> >
> >
> > Thanks,
> > pq
> >
> >
> > > struct drm_syncobj_wait {
> > > __u64 handles;
> > > /* absolute timeout */
> > > @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> > > __u32 flags;
> > > __u32 first_signaled; /* only valid when not waiting all */
> > > __u32 pad;
> > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > > + __u64 deadline_ns;
> > > };
> > >
> > > struct drm_syncobj_timeline_wait {
> > > @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> > > __u32 flags;
> > > __u32 first_signaled; /* only valid when not waiting all */
> > > __u32 pad;
> > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > > + __u64 deadline_ns;
> > > };
> > >
> > >
> >
pgptlCPk3mW32.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, 21 Feb 2023 09:50:20 -0800 Rob Clark wrote: > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > wrote: > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > Rob Clark wrote: > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > > atomic update is waiting on. > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > --- > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > drm_crtc *crtc, > > > > > > } > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > +/** > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > vblank > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > timestamp. > > > > > > + * > > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > > previous > > > > > > + * vblank and frame duration > > > > > > > > > > Hi, > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > VRR mode, right? > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > I don't know. :-) > > > > At least for i915 this will give you the maximum frame > > duration. > > I suppose one could argue that maximum frame duration is the actual > deadline. Anything less is just moar fps, but not going to involve > stalling until vblank N+1, AFAIU > > > Also this does not calculate the the start of vblank, it > > calculates the start of active video. > > Probably something like end of previous frame's video.. might not be > _exactly_ correct (because some buffering involved), but OTOH on the > GPU side, I expect the driver to set a timer for a few ms or so before > the deadline. So there is some wiggle room. The vblank timestamp is defined to be the time of the first active pixel of the frame in the video signal. At least that's the one that UAPI carries (when not tearing?). It is not the start of vblank period. With VRR, the front porch before the first active pixel can be multiple milliseconds. The difference between 144 Hz and 60 Hz is 9.7 ms for example. Thanks, pq pgpDN6UIgZk3W.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, 21 Feb 2023 09:53:56 -0800 Rob Clark wrote: > On Tue, Feb 21, 2023 at 8:48 AM Luben Tuikov wrote: > > > > On 2023-02-20 11:14, Rob Clark wrote: > > > On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen > > > wrote: > > >> > > >> On Sat, 18 Feb 2023 13:15:49 -0800 > > >> Rob Clark wrote: > > >> > > >>> From: Rob Clark > > >>> > > >>> Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent > > >>> wait (as opposed to a "housekeeping" wait to know when to cleanup after > > >>> some work has completed). Usermode components of GPU driver stacks > > >>> often poll() on fence fd's to know when it is safe to do things like > > >>> free or reuse a buffer, but they can also poll() on a fence fd when > > >>> waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag > > >>> lets the kernel differentiate these two cases. > > >>> > > >>> Signed-off-by: Rob Clark > > >> > > >> Hi, > > >> > > >> where would the UAPI documentation of this go? > > >> It seems to be missing. > > > > > > Good question, I am not sure. The poll() man page has a description, > > > but my usage doesn't fit that _exactly_ (but OTOH the description is a > > > bit vague). > > > > > >> If a Wayland compositor is polling application fences to know which > > >> client buffer to use in its rendering, should the compositor poll with > > >> PRI or not? If a compositor polls with PRI, then all fences from all > > >> applications would always be PRI. Would that be harmful somehow or > > >> would it be beneficial? > > > > > > I think a compositor would rather use the deadline ioctl and then poll > > > without PRI. Otherwise you are giving an urgency signal to the fence > > > signaller which might not necessarily be needed. > > > > > > The places where I expect PRI to be useful is more in mesa (things > > > like glFinish(), readpix, and other similar sorts of blocking APIs) > > Hi, > > > > Hmm, but then user-space could do the opposite, namely, submit work as > > usual--never > > using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. > > That seems > > like a possible usage pattern, unintended--maybe, but possible. Do we want > > to discourage > > this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE > > with the current > > time, and then wouldn't that be equivalent to (E)POLLPRI? > > Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is > slightly more convenient if you want an immediate deadline (single > syscall instead of two), but not strictly needed. OTOH it piggy-backs > on existing UABI. In that case, I would be conservative, and not add the POLLPRI semantics. An UAPI addition that is not strictly needed and somewhat unclear if it violates any design principles is best not done, until it is proven to be beneficial. Besides, a Wayland compositor does not necessary need to add the fd to its main event loop for poll. It could just SET_DEADLINE, and then when it renders simply check if the fence passed or not already. Not polling means the compositor does not need to wake up at the moment the fence signals to just record a flag. On another matter, if the application uses SET_DEADLINE with one timestamp, and the compositor uses SET_DEADLINE on the same thing with another timestamp, what should happen? Maybe it's a soft-realtime app whose primary goal is not display, and it needs the result faster than the window server? Maybe SET_DEADLINE should set the deadline only to an earlier timestamp and never later? Thanks, pq pgpuFLuposEbE.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, 21 Feb 2023 15:01:35 +0200
Ville Syrjälä wrote:
> On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote:
> > On Mon, 20 Feb 2023 07:55:41 -0800
> > Rob Clark wrote:
> >
> > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen
> > > wrote:
> > > >
> > > > On Sat, 18 Feb 2023 13:15:53 -0800
> > > > Rob Clark wrote:
> > > >
> > > > > From: Rob Clark
> > > > >
> > > > > Will be used in the next commit to set a deadline on fences that an
> > > > > atomic update is waiting on.
> > > > >
> > > > > Signed-off-by: Rob Clark
> > > > > ---
> > > > > drivers/gpu/drm/drm_vblank.c | 32
> > > > > include/drm/drm_vblank.h | 1 +
> > > > > 2 files changed, 33 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c
> > > > > b/drivers/gpu/drm/drm_vblank.c
> > > > > index 2ff31717a3de..caf25ebb34c5 100644
> > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct
> > > > > drm_crtc *crtc,
> > > > > }
> > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
> > > > >
> > > > > +/**
> > > > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank
> > > > > + * @crtc: the crtc for which to calculate next vblank time
> > > > > + * @vblanktime: pointer to time to receive the next vblank timestamp.
> > > > > + *
> > > > > + * Calculate the expected time of the next vblank based on time of
> > > > > previous
> > > > > + * vblank and frame duration
> > > >
> > > > Hi,
> > > >
> > > > for VRR this targets the highest frame rate possible for the current
> > > > VRR mode, right?
> > > >
> > >
> > > It is based on vblank->framedur_ns which is in turn based on
> > > mode->crtc_clock. Presumably for VRR that ends up being a maximum?
> >
> > I don't know. :-)
>
> At least for i915 this will give you the maximum frame
> duration.
Really maximum duration? So minimum VRR frequency?
> Also this does not calculate the the start of vblank, it
> calculates the start of active video.
Oh indeed, so it's too late. What one would actually need for the
deadline is the driver's deadline to present for the immediately next
start of active video.
And with VRR that should probably aim for the maximum frame frequency,
not minimum?
Thanks,
pq
>
> >
> > You need a number of clock cycles in addition to the clock frequency,
> > and that could still be minimum, maximum, the last realized one, ...
> >
> > VRR works by adjusting the front porch length IIRC.
> >
> >
> > Thanks,
> > pq
> >
> > > BR,
> > > -R
> > >
> > >
> > > >
> > > > Thanks,
> > > > pq
> > > >
> > > > > + */
> > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t
> > > > > *vblanktime)
> > > > > +{
> > > > > + unsigned int pipe = drm_crtc_index(crtc);
> > > > > + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
> > > > > + u64 count;
> > > > > +
> > > > > + if (!vblank->framedur_ns)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime);
> > > > > +
> > > > > + /*
> > > > > + * If we don't get a valid count, then we probably also don't
> > > > > + * have a valid time:
> > > > > + */
> > > > > + if (!count)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + *vblanktime = ktime_add(*vblanktime,
> > > > > ns_to_ktime(vblank->framedur_ns));
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time);
> > > > > +
> > > > > static void send_vblank_event(struct drm_device *dev,
> > > > > struct drm_pending_vblank_event *e,
> > > > > u64 seq, ktime_t now)
> > > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > > > index 733a3e2d1d10..a63bc2c92f3c 100644
> > > > > --- a/include/drm/drm_vblank.h
> > > > > +++ b/include/drm/drm_vblank.h
> > > > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device
> > > > > *dev);
> > > > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
> > > > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> > > > > ktime_t *vblanktime);
> > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t
> > > > > *vblanktime);
> > > > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> > > > > struct drm_pending_vblank_event *e);
> > > > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> > > >
> >
>
>
>
pgpxhj3PjORdS.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Mon, 20 Feb 2023 12:18:56 -0800
Rob Clark wrote:
> From: Rob Clark
>
> Add a new flag to let userspace provide a deadline as a hint for syncobj
> and timeline waits. This gives a hint to the driver signaling the
> backing fences about how soon userspace needs it to compete work, so it
> can addjust GPU frequency accordingly. An immediate deadline can be
> given to provide something equivalent to i915 "wait boost".
>
> v2: Use absolute u64 ns value for deadline hint, drop cap and driver
> feature flag in favor of allowing count_handles==0 as a way for
> userspace to probe kernel for support of new flag
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/drm_syncobj.c | 59 +++
> include/uapi/drm/drm.h| 5 +++
> 2 files changed, 51 insertions(+), 13 deletions(-)
...
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..aefc8cc743e0 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -887,6 +887,7 @@ struct drm_syncobj_transfer {
> #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
> #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
> #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time
> point to become available */
> +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE (1 << 3) /* set fence deadline
> based to deadline_nsec/sec */
Hi,
where is the UAPI documentation explaining what is a "fence deadline"
and what setting it does here?
btw. no nsec/sec anymore.
Thanks,
pq
> struct drm_syncobj_wait {
> __u64 handles;
> /* absolute timeout */
> @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> __u32 flags;
> __u32 first_signaled; /* only valid when not waiting all */
> __u32 pad;
> + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> + __u64 deadline_ns;
> };
>
> struct drm_syncobj_timeline_wait {
> @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> __u32 flags;
> __u32 first_signaled; /* only valid when not waiting all */
> __u32 pad;
> + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> + __u64 deadline_ns;
> };
>
>
pgpGBUnbID2Bl.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Mon, 20 Feb 2023 07:55:41 -0800
Rob Clark wrote:
> On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen wrote:
> >
> > On Sat, 18 Feb 2023 13:15:53 -0800
> > Rob Clark wrote:
> >
> > > From: Rob Clark
> > >
> > > Will be used in the next commit to set a deadline on fences that an
> > > atomic update is waiting on.
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/drm_vblank.c | 32
> > > include/drm/drm_vblank.h | 1 +
> > > 2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > index 2ff31717a3de..caf25ebb34c5 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct drm_crtc
> > > *crtc,
> > > }
> > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
> > >
> > > +/**
> > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank
> > > + * @crtc: the crtc for which to calculate next vblank time
> > > + * @vblanktime: pointer to time to receive the next vblank timestamp.
> > > + *
> > > + * Calculate the expected time of the next vblank based on time of
> > > previous
> > > + * vblank and frame duration
> >
> > Hi,
> >
> > for VRR this targets the highest frame rate possible for the current
> > VRR mode, right?
> >
>
> It is based on vblank->framedur_ns which is in turn based on
> mode->crtc_clock. Presumably for VRR that ends up being a maximum?
I don't know. :-)
You need a number of clock cycles in addition to the clock frequency,
and that could still be minimum, maximum, the last realized one, ...
VRR works by adjusting the front porch length IIRC.
Thanks,
pq
> BR,
> -R
>
>
> >
> > Thanks,
> > pq
> >
> > > + */
> > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t *vblanktime)
> > > +{
> > > + unsigned int pipe = drm_crtc_index(crtc);
> > > + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
> > > + u64 count;
> > > +
> > > + if (!vblank->framedur_ns)
> > > + return -EINVAL;
> > > +
> > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime);
> > > +
> > > + /*
> > > + * If we don't get a valid count, then we probably also don't
> > > + * have a valid time:
> > > + */
> > > + if (!count)
> > > + return -EINVAL;
> > > +
> > > + *vblanktime = ktime_add(*vblanktime,
> > > ns_to_ktime(vblank->framedur_ns));
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time);
> > > +
> > > static void send_vblank_event(struct drm_device *dev,
> > > struct drm_pending_vblank_event *e,
> > > u64 seq, ktime_t now)
> > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > index 733a3e2d1d10..a63bc2c92f3c 100644
> > > --- a/include/drm/drm_vblank.h
> > > +++ b/include/drm/drm_vblank.h
> > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device *dev);
> > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
> > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> > > ktime_t *vblanktime);
> > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t
> > > *vblanktime);
> > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> > > struct drm_pending_vblank_event *e);
> > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> >
pgpfgT0gd3E7p.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 05/14] dma-buf/sync_file: Add SET_DEADLINE ioctl
On Mon, 20 Feb 2023 08:09:04 -0800
Rob Clark wrote:
> On Mon, Feb 20, 2023 at 12:27 AM Christian König
> wrote:
> >
> > Am 18.02.23 um 22:15 schrieb Rob Clark:
> > > From: Rob Clark
> > >
> > > The initial purpose is for igt tests, but this would also be useful for
> > > compositors that wait until close to vblank deadline to make decisions
> > > about which frame to show.
> > >
> > > The igt tests can be found at:
> > >
> > > https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline
> > >
> > > v2: Clarify the timebase, add link to igt tests
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/dma-buf/sync_file.c| 19 +++
> > > include/uapi/linux/sync_file.h | 22 ++
> > > 2 files changed, 41 insertions(+)
> > >
> > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> > > index af57799c86ce..fb6ca1032885 100644
> > > --- a/drivers/dma-buf/sync_file.c
> > > +++ b/drivers/dma-buf/sync_file.c
> > > @@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct
> > > sync_file *sync_file,
> > > return ret;
> > > }
> > >
> > > +static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
> > > + unsigned long arg)
> > > +{
> > > + struct sync_set_deadline ts;
> > > +
> > > + if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
> > > + return -EFAULT;
> > > +
> > > + if (ts.pad)
> > > + return -EINVAL;
> > > +
> > > + dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec,
> > > ts.tv_nsec));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static long sync_file_ioctl(struct file *file, unsigned int cmd,
> > > unsigned long arg)
> > > {
> > > @@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file,
> > > unsigned int cmd,
> > > case SYNC_IOC_FILE_INFO:
> > > return sync_file_ioctl_fence_info(sync_file, arg);
> > >
> > > + case SYNC_IOC_SET_DEADLINE:
> > > + return sync_file_ioctl_set_deadline(sync_file, arg);
> > > +
> > > default:
> > > return -ENOTTY;
> > > }
> > > diff --git a/include/uapi/linux/sync_file.h
> > > b/include/uapi/linux/sync_file.h
> > > index ee2dcfb3d660..c8666580816f 100644
> > > --- a/include/uapi/linux/sync_file.h
> > > +++ b/include/uapi/linux/sync_file.h
> > > @@ -67,6 +67,20 @@ struct sync_file_info {
> > > __u64 sync_fence_info;
> > > };
> > >
> > > +/**
> > > + * struct sync_set_deadline - set a deadline on a fence
> > > + * @tv_sec: seconds elapsed since epoch
> > > + * @tv_nsec: nanoseconds elapsed since the time given by the tv_sec
> > > + * @pad: must be zero
> > > + *
> > > + * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
> > > + */
> > > +struct sync_set_deadline {
> > > + __s64 tv_sec;
> > > + __s32 tv_nsec;
> > > + __u32 pad;
> >
> > IIRC struct timespec defined this as time_t/long (which is horrible for
> > an UAPI because of the sizeof(long) dependency), one possible
> > alternative is to use 64bit nanoseconds from CLOCK_MONOTONIC (which is
> > essentially ktime).
> >
> > Not 100% sure if there is any preferences documented, but I think the
> > later might be better.
>
> The original thought is that this maps directly to clock_gettime()
> without extra conversion needed, and is similar to other pre-ktime_t
> UAPI. But OTOH if userspace wants to add an offset, it is maybe
> better to convert completely to ns in userspace and use a u64 (as that
> is what ns_to_ktime() uses).. (and OFC whatever decision here also
> applies to the syncobj wait ioctls)
>
> I'm leaning towards u64 CLOCK_MONOTONIC ns if no one has a good
> argument against that.
No, no good argument against that, it's just different from any other
UAPI so far, but a new style has to start somewhere. It's good for 584
years after the epoch.
Just make sure the documentation is explicit on how struct timespec is
converted to and from u64 (signedness issues, overflow and whatnot).
Thanks,
pq
>
> BR,
> -R
>
> > Either way the patch is Acked-by: Christian König
> > for this patch.
> >
> > Regards,
> > Christian.
> >
> > > +};
> > > +
> > > #define SYNC_IOC_MAGIC '>'
> > >
> > > /**
> > > @@ -95,4 +109,12 @@ struct sync_file_info {
> > >*/
> > > #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct
> > > sync_file_info)
> > >
> > > +
> > > +/**
> > > + * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
> > > + *
> > > + * Allows userspace to set a deadline on a fence, see
> > > dma_fence_set_deadline()
> > > + */
> > > +#define SYNC_IOC_SET_DEADLINE_IOW(SYNC_IOC_MAGIC, 5, struct
> > > sync_set_deadline)
> > > +
> > > #endif /* _UAPI_LINUX_SYNC_H */
> >
pgp9dWg_hK18L.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Mon, 20 Feb 2023 09:31:41 +0100
Christian König wrote:
> Am 18.02.23 um 22:15 schrieb Rob Clark:
> > From: Rob Clark
> >
> > Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent
> > wait (as opposed to a "housekeeping" wait to know when to cleanup after
> > some work has completed). Usermode components of GPU driver stacks
> > often poll() on fence fd's to know when it is safe to do things like
> > free or reuse a buffer, but they can also poll() on a fence fd when
> > waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag
> > lets the kernel differentiate these two cases.
> >
> > Signed-off-by: Rob Clark
>
> The code looks clean, but the different poll flags and their meaning are
> certainly not my field of expertise.
A good question. epoll_ctl manual refers to poll(2) which says:
POLLPRI
There is some exceptional condition on the file descriptor.
Possibilities include:
• There is out-of-band data on a TCP socket (see tcp(7)).
• A pseudoterminal master in packet mode has seen a state change
on the slave (see ioctl_tty(2)).
• A cgroup.events file has been modified (see cgroups(7)).
It seems to be about selecting what events will trigger the poll,
more than how (fast) to poll. At least it is not documented to be
ignored in 'events', so I guess it should work.
Thanks,
pq
> Feel free to add Acked-by: Christian König ,
> somebody with more background in this should probably take a look as well.
>
> Regards,
> Christian.
>
> > ---
> > drivers/dma-buf/sync_file.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> > index fb6ca1032885..c30b2085ee0a 100644
> > --- a/drivers/dma-buf/sync_file.c
> > +++ b/drivers/dma-buf/sync_file.c
> > @@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file,
> > poll_table *wait)
> > {
> > struct sync_file *sync_file = file->private_data;
> >
> > + /*
> > +* The POLLPRI/EPOLLPRI flag can be used to signal that
> > +* userspace wants the fence to signal ASAP, express this
> > +* as an immediate deadline.
> > +*/
> > + if (poll_requested_events(wait) & EPOLLPRI)
> > + dma_fence_set_deadline(sync_file->fence, ktime_get());
> > +
> > poll_wait(file, &sync_file->wq, wait);
> >
> > if (list_empty(&sync_file->cb.node) &&
>
pgpPUXXpDEAPq.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Mon, 20 Feb 2023 08:14:47 -0800
Rob Clark wrote:
> On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen wrote:
> >
> > On Sat, 18 Feb 2023 13:15:49 -0800
> > Rob Clark wrote:
> >
> > > From: Rob Clark
> > >
> > > Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent
> > > wait (as opposed to a "housekeeping" wait to know when to cleanup after
> > > some work has completed). Usermode components of GPU driver stacks
> > > often poll() on fence fd's to know when it is safe to do things like
> > > free or reuse a buffer, but they can also poll() on a fence fd when
> > > waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag
> > > lets the kernel differentiate these two cases.
> > >
> > > Signed-off-by: Rob Clark
> >
> > Hi,
> >
> > where would the UAPI documentation of this go?
> > It seems to be missing.
>
> Good question, I am not sure. The poll() man page has a description,
> but my usage doesn't fit that _exactly_ (but OTOH the description is a
> bit vague).
>
> > If a Wayland compositor is polling application fences to know which
> > client buffer to use in its rendering, should the compositor poll with
> > PRI or not? If a compositor polls with PRI, then all fences from all
> > applications would always be PRI. Would that be harmful somehow or
> > would it be beneficial?
>
> I think a compositor would rather use the deadline ioctl and then poll
> without PRI. Otherwise you are giving an urgency signal to the fence
> signaller which might not necessarily be needed.
>
> The places where I expect PRI to be useful is more in mesa (things
> like glFinish(), readpix, and other similar sorts of blocking APIs)
Sounds good. Docs... ;-)
Hmm, so a compositor should set the deadline when it processes the
wl_surface.commit, and not when it actually starts repainting, to give
time for the driver to react and the GPU to do some more work. The
deadline would be the time when the compositor starts its repaint, so
it knows if the buffer is ready or not.
Thanks,
pq
>
> BR,
> -R
>
> >
> >
> > Thanks,
> > pq
> >
> > > ---
> > > drivers/dma-buf/sync_file.c | 8
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> > > index fb6ca1032885..c30b2085ee0a 100644
> > > --- a/drivers/dma-buf/sync_file.c
> > > +++ b/drivers/dma-buf/sync_file.c
> > > @@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file,
> > > poll_table *wait)
> > > {
> > > struct sync_file *sync_file = file->private_data;
> > >
> > > + /*
> > > + * The POLLPRI/EPOLLPRI flag can be used to signal that
> > > + * userspace wants the fence to signal ASAP, express this
> > > + * as an immediate deadline.
> > > + */
> > > + if (poll_requested_events(wait) & EPOLLPRI)
> > > + dma_fence_set_deadline(sync_file->fence, ktime_get());
> > > +
> > > poll_wait(file, &sync_file->wq, wait);
> > >
> > > if (list_empty(&sync_file->cb.node) &&
> >
pgpnVJYkLApzI.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Sat, 18 Feb 2023 13:15:53 -0800
Rob Clark wrote:
> From: Rob Clark
>
> Will be used in the next commit to set a deadline on fences that an
> atomic update is waiting on.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/drm_vblank.c | 32
> include/drm/drm_vblank.h | 1 +
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 2ff31717a3de..caf25ebb34c5 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> }
> EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
>
> +/**
> + * drm_crtc_next_vblank_time - calculate the time of the next vblank
> + * @crtc: the crtc for which to calculate next vblank time
> + * @vblanktime: pointer to time to receive the next vblank timestamp.
> + *
> + * Calculate the expected time of the next vblank based on time of previous
> + * vblank and frame duration
Hi,
for VRR this targets the highest frame rate possible for the current
VRR mode, right?
Thanks,
pq
> + */
> +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t *vblanktime)
> +{
> + unsigned int pipe = drm_crtc_index(crtc);
> + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
> + u64 count;
> +
> + if (!vblank->framedur_ns)
> + return -EINVAL;
> +
> + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime);
> +
> + /*
> + * If we don't get a valid count, then we probably also don't
> + * have a valid time:
> + */
> + if (!count)
> + return -EINVAL;
> +
> + *vblanktime = ktime_add(*vblanktime, ns_to_ktime(vblank->framedur_ns));
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_next_vblank_time);
> +
> static void send_vblank_event(struct drm_device *dev,
> struct drm_pending_vblank_event *e,
> u64 seq, ktime_t now)
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 733a3e2d1d10..a63bc2c92f3c 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device *dev);
> u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
> u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> ktime_t *vblanktime);
> +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t *vblanktime);
> void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> struct drm_pending_vblank_event *e);
> void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
pgpwAIOSmcdCX.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 09/14] drm/syncobj: Add deadline support for syncobj waits
On Sat, 18 Feb 2023 13:15:52 -0800
Rob Clark wrote:
> From: Rob Clark
>
> Add a new flag to let userspace provide a deadline as a hint for syncobj
> and timeline waits. This gives a hint to the driver signaling the
> backing fences about how soon userspace needs it to compete work, so it
> can addjust GPU frequency accordingly. An immediate deadline can be
> given to provide something equivalent to i915 "wait boost".
>
> Signed-off-by: Rob Clark
> ---
>
> I'm a bit on the fence about the addition of the DRM_CAP, but it seems
> useful to give userspace a way to probe whether the kernel and driver
> supports the new wait flag, especially since we have vk-common code
> dealing with syncobjs. But open to suggestions.
>
> drivers/gpu/drm/drm_ioctl.c | 3 ++
> drivers/gpu/drm/drm_syncobj.c | 59 ---
> include/drm/drm_drv.h | 6
> include/uapi/drm/drm.h| 16 --
> 4 files changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7c9d66ee917d..1c5c942cf0f9 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -254,6 +254,9 @@ static int drm_getcap(struct drm_device *dev, void *data,
> struct drm_file *file_
> case DRM_CAP_SYNCOBJ_TIMELINE:
> req->value = drm_core_check_feature(dev,
> DRIVER_SYNCOBJ_TIMELINE);
> return 0;
> + case DRM_CAP_SYNCOBJ_DEADLINE:
> + req->value = drm_core_check_feature(dev,
> DRIVER_SYNCOBJ_TIMELINE);
Hi,
is that a typo for DRIVER_SYNCOBJ_DEADLINE?
> + return 0;
> }
>
> /* Other caps only work with KMS drivers */
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 0c2be8360525..61cf97972a60 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -973,7 +973,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
> drm_syncobj **syncobjs,
> uint32_t count,
> uint32_t flags,
> signed long timeout,
> - uint32_t *idx)
> + uint32_t *idx,
> + ktime_t *deadline)
> {
> struct syncobj_wait_entry *entries;
> struct dma_fence *fence;
> @@ -1053,6 +1054,15 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> drm_syncobj_fence_add_wait(syncobjs[i], &entries[i]);
> }
>
> + if (deadline) {
> + for (i = 0; i < count; ++i) {
> + fence = entries[i].fence;
> + if (!fence)
> + continue;
> + dma_fence_set_deadline(fence, *deadline);
> + }
> + }
> +
> do {
> set_current_state(TASK_INTERRUPTIBLE);
>
> @@ -1151,7 +1161,8 @@ static int drm_syncobj_array_wait(struct drm_device
> *dev,
> struct drm_file *file_private,
> struct drm_syncobj_wait *wait,
> struct drm_syncobj_timeline_wait
> *timeline_wait,
> - struct drm_syncobj **syncobjs, bool timeline)
> + struct drm_syncobj **syncobjs, bool timeline,
> + ktime_t *deadline)
> {
> signed long timeout = 0;
> uint32_t first = ~0;
> @@ -1162,7 +1173,8 @@ static int drm_syncobj_array_wait(struct drm_device
> *dev,
>NULL,
>wait->count_handles,
>wait->flags,
> - timeout, &first);
> + timeout, &first,
> + deadline);
> if (timeout < 0)
> return timeout;
> wait->first_signaled = first;
> @@ -1172,7 +1184,8 @@ static int drm_syncobj_array_wait(struct drm_device
> *dev,
>
> u64_to_user_ptr(timeline_wait->points),
>
> timeline_wait->count_handles,
>timeline_wait->flags,
> - timeout, &first);
> + timeout, &first,
> + deadline);
> if (timeout < 0)
> return timeout;
> timeline_wait->first_signaled = first;
> @@ -1243,13 +1256,2
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Sat, 18 Feb 2023 13:15:49 -0800
Rob Clark wrote:
> From: Rob Clark
>
> Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent
> wait (as opposed to a "housekeeping" wait to know when to cleanup after
> some work has completed). Usermode components of GPU driver stacks
> often poll() on fence fd's to know when it is safe to do things like
> free or reuse a buffer, but they can also poll() on a fence fd when
> waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag
> lets the kernel differentiate these two cases.
>
> Signed-off-by: Rob Clark
Hi,
where would the UAPI documentation of this go?
It seems to be missing.
If a Wayland compositor is polling application fences to know which
client buffer to use in its rendering, should the compositor poll with
PRI or not? If a compositor polls with PRI, then all fences from all
applications would always be PRI. Would that be harmful somehow or
would it be beneficial?
Thanks,
pq
> ---
> drivers/dma-buf/sync_file.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index fb6ca1032885..c30b2085ee0a 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file,
> poll_table *wait)
> {
> struct sync_file *sync_file = file->private_data;
>
> + /*
> + * The POLLPRI/EPOLLPRI flag can be used to signal that
> + * userspace wants the fence to signal ASAP, express this
> + * as an immediate deadline.
> + */
> + if (poll_requested_events(wait) & EPOLLPRI)
> + dma_fence_set_deadline(sync_file->fence, ktime_get());
> +
> poll_wait(file, &sync_file->wq, wait);
>
> if (list_empty(&sync_file->cb.node) &&
pgpissQ7ejTcj.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v4 05/14] dma-buf/sync_file: Add SET_DEADLINE ioctl
On Sat, 18 Feb 2023 13:15:48 -0800
Rob Clark wrote:
> From: Rob Clark
>
> The initial purpose is for igt tests, but this would also be useful for
> compositors that wait until close to vblank deadline to make decisions
> about which frame to show.
>
> The igt tests can be found at:
>
> https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline
>
> v2: Clarify the timebase, add link to igt tests
>
> Signed-off-by: Rob Clark
> ---
> drivers/dma-buf/sync_file.c| 19 +++
> include/uapi/linux/sync_file.h | 22 ++
> 2 files changed, 41 insertions(+)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index af57799c86ce..fb6ca1032885 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
> return ret;
> }
>
> +static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
> + unsigned long arg)
> +{
> + struct sync_set_deadline ts;
> +
> + if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
> + return -EFAULT;
> +
> + if (ts.pad)
> + return -EINVAL;
> +
> + dma_fence_set_deadline(sync_file->fence, ktime_set(ts.tv_sec,
> ts.tv_nsec));
> +
> + return 0;
> +}
> +
> static long sync_file_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned
> int cmd,
> case SYNC_IOC_FILE_INFO:
> return sync_file_ioctl_fence_info(sync_file, arg);
>
> + case SYNC_IOC_SET_DEADLINE:
> + return sync_file_ioctl_set_deadline(sync_file, arg);
> +
> default:
> return -ENOTTY;
> }
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index ee2dcfb3d660..c8666580816f 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -67,6 +67,20 @@ struct sync_file_info {
> __u64 sync_fence_info;
> };
>
> +/**
> + * struct sync_set_deadline - set a deadline on a fence
> + * @tv_sec: seconds elapsed since epoch
> + * @tv_nsec: nanoseconds elapsed since the time given by the tv_sec
Hi,
should tv_sec,tv_nsec be validated like clock_settime() does?
It requires:
- tv_sec >= 0
- tv_nsec >= 0
- tv_nsec < 1e9
Thanks,
pq
> + * @pad: must be zero
> + *
> + * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
> + */
> +struct sync_set_deadline {
> + __s64 tv_sec;
> + __s32 tv_nsec;
> + __u32 pad;
> +};
> +
> #define SYNC_IOC_MAGIC '>'
>
> /**
> @@ -95,4 +109,12 @@ struct sync_file_info {
> */
> #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>
> +
> +/**
> + * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
> + *
> + * Allows userspace to set a deadline on a fence, see
> dma_fence_set_deadline()
> + */
> +#define SYNC_IOC_SET_DEADLINE_IOW(SYNC_IOC_MAGIC, 5, struct
> sync_set_deadline)
> +
> #endif /* _UAPI_LINUX_SYNC_H */
pgp9Nl7MqH8FF.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Wed, 1 Feb 2023 18:06:41 -0800 Jessica Zhang wrote: > On 1/31/2023 4:49 AM, Pekka Paalanen wrote: > > On Tue, 31 Jan 2023 11:21:18 + > > Simon Ser wrote: > > > >> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen > >> wrote: > >> > >>> On Tue, 31 Jan 2023 10:06:39 + > >>> Simon Ser wrote: > >>> > >>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen > >>>> wrote: > >>>> > >>>>> indeed, what about simply using a 1x1 framebuffer for real? Why was that > >>>>> approach rejected? > >>>> > >>>> Ideally we don't want to allocate any GPU memory for the solid-fill > >>>> stuff. And if we special-case 1x1 FB creation to not be backed by real > >>>> GPU memory then we hit several situations where user-space expects a > >>>> real FB but there isn't: for instance, GETFB2 converts from FB object > >>>> ID to GEM handles. Even if we make GETFB2 fail and accept that this > >>>> breaks user-space, then there is no way for user-space to recover the > >>>> FB color for flicker-free transitions and such. > >>>> > >>>> This is all purely from a uAPI PoV, completely ignoring the potential > >>>> issues with the internal kernel abstractions which might not be suitable > >>>> for this either. > >>> > >>> I mean a real 1x1 buffer: a dumb buffer. > >>> > >>> It would be absolutely compatible with anything existing, because it is > >>> a real FB. As a dumb buffer it would be trivial to write into and read > >>> out. As 1x1 it would be tiny (one page?). Even if something needs to > >>> raw-access uncached memory over 33 MHz PCI bus or whatever the worst > >>> case is, it's just one pixel, so it's fast enough, right? And it only > >>> needs to be read once when set, like USB display drivers do. The driver > >>> does not need to manually apply any color operations, because none are > >>> supported in this special case. > >>> > >>> One can put all these limitations and even pixel format in the plane > >>> property that tells userspace that a 1x1 FB works here. > >>> > >>> To recap, the other alternatives under discussion I see right now are: > >>> > >>> - this proposal of dedicated fill color property > >>> - stuffing something new into FB_ID property > >>> > >>> There is also the question of other kinds of plane content sources like > >>> live camera feeds where userspace won't be shovelling each frame > >>> individually like we do now. > >>> > >>> 1x1 dumb buffer is not as small and lean as a dedicated fill color > >>> property, but the UAPI design questions seem to be much less. What's > >>> the best trade-off and for whom? > >> > >> By "real memory" yes I mean the 1 page. > >> > >> Using a real buffer also brings back other discussions, e.g. the one about > >> which pixel formats to accept. > > > > Yeah, which is why I wrote: "One can put all these limitations and even > > pixel format in the plane property". It doesn't even need to be a > > variable in the UAPI, it can be hardcoded in the UAPI doc. > > > > Please, do not understand this as me strongly advocating for the real FB > > approach! I just don't want that option to be misunderstood. > > > > I don't really care which design is chosen, but I do care about > > documenting why other designs were rejected. If the rejection reasons > > were false, they should be revised, even if the decision does not > > change. > > Hi Pekka/Daniel, > > Looks like the general sentiment is to keep solid fill as a separate > property, so I will stick with that implementation for v4. > > I can document the reason why we chose this approach over 1x1 FB in the > cover letter, but to summarize here: > > Allocating an FB for solid_fill brings in unnecessary overhead (ex. > having to allocate memory for the FB). In addition, since memory fetch > is disabled when solid fill is enabled, having a separate property that > doesn't do any memory allocation for solid fill better reflects the > behavior of this feature within driver. > > We also wanted to avoid having FB_ID accept a property blob as it would > involve loosening some drm_propert
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Tue, 31 Jan 2023 11:21:18 + Simon Ser wrote: > On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen > wrote: > > > On Tue, 31 Jan 2023 10:06:39 + > > Simon Ser wrote: > > > > > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen > > > wrote: > > > > > > > indeed, what about simply using a 1x1 framebuffer for real? Why was that > > > > approach rejected? > > > > > > Ideally we don't want to allocate any GPU memory for the solid-fill > > > stuff. And if we special-case 1x1 FB creation to not be backed by real > > > GPU memory then we hit several situations where user-space expects a > > > real FB but there isn't: for instance, GETFB2 converts from FB object > > > ID to GEM handles. Even if we make GETFB2 fail and accept that this > > > breaks user-space, then there is no way for user-space to recover the > > > FB color for flicker-free transitions and such. > > > > > > This is all purely from a uAPI PoV, completely ignoring the potential > > > issues with the internal kernel abstractions which might not be suitable > > > for this either. > > > > I mean a real 1x1 buffer: a dumb buffer. > > > > It would be absolutely compatible with anything existing, because it is > > a real FB. As a dumb buffer it would be trivial to write into and read > > out. As 1x1 it would be tiny (one page?). Even if something needs to > > raw-access uncached memory over 33 MHz PCI bus or whatever the worst > > case is, it's just one pixel, so it's fast enough, right? And it only > > needs to be read once when set, like USB display drivers do. The driver > > does not need to manually apply any color operations, because none are > > supported in this special case. > > > > One can put all these limitations and even pixel format in the plane > > property that tells userspace that a 1x1 FB works here. > > > > To recap, the other alternatives under discussion I see right now are: > > > > - this proposal of dedicated fill color property > > - stuffing something new into FB_ID property > > > > There is also the question of other kinds of plane content sources like > > live camera feeds where userspace won't be shovelling each frame > > individually like we do now. > > > > 1x1 dumb buffer is not as small and lean as a dedicated fill color > > property, but the UAPI design questions seem to be much less. What's > > the best trade-off and for whom? > > By "real memory" yes I mean the 1 page. > > Using a real buffer also brings back other discussions, e.g. the one about > which pixel formats to accept. Yeah, which is why I wrote: "One can put all these limitations and even pixel format in the plane property". It doesn't even need to be a variable in the UAPI, it can be hardcoded in the UAPI doc. Please, do not understand this as me strongly advocating for the real FB approach! I just don't want that option to be misunderstood. I don't really care which design is chosen, but I do care about documenting why other designs were rejected. If the rejection reasons were false, they should be revised, even if the decision does not change. Thanks, pq pgpnUpC_CC5h8.pgp Description: OpenPGP digital signature
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Tue, 31 Jan 2023 10:06:39 + Simon Ser wrote: > On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen > wrote: > > > indeed, what about simply using a 1x1 framebuffer for real? Why was that > > approach rejected? > > Ideally we don't want to allocate any GPU memory for the solid-fill > stuff. And if we special-case 1x1 FB creation to not be backed by real > GPU memory then we hit several situations where user-space expects a > real FB but there isn't: for instance, GETFB2 converts from FB object > ID to GEM handles. Even if we make GETFB2 fail and accept that this > breaks user-space, then there is no way for user-space to recover the > FB color for flicker-free transitions and such. > > This is all purely from a uAPI PoV, completely ignoring the potential > issues with the internal kernel abstractions which might not be suitable > for this either. I mean a real 1x1 buffer: a dumb buffer. It would be absolutely compatible with anything existing, because it is a real FB. As a dumb buffer it would be trivial to write into and read out. As 1x1 it would be tiny (one page?). Even if something needs to raw-access uncached memory over 33 MHz PCI bus or whatever the worst case is, it's just one pixel, so it's fast enough, right? And it only needs to be read once when set, like USB display drivers do. The driver does not need to manually apply any color operations, because none are supported in this special case. One can put all these limitations and even pixel format in the plane property that tells userspace that a 1x1 FB works here. To recap, the other alternatives under discussion I see right now are: - this proposal of dedicated fill color property - stuffing something new into FB_ID property There is also the question of other kinds of plane content sources like live camera feeds where userspace won't be shovelling each frame individually like we do now. 1x1 dumb buffer is not as small and lean as a dedicated fill color property, but the UAPI design questions seem to be much less. What's the best trade-off and for whom? Thanks, pq pgpHVFR5uo5jP.pgp Description: OpenPGP digital signature
Re: [Freedreno] [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Fri, 6 Jan 2023 23:49:34 +0200 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. > > 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. Hi, indeed, what about simply using a 1x1 framebuffer for real
Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On Wed, 23 Nov 2022 15:27:04 -0800
Jessica Zhang wrote:
> On 11/9/2022 1:18 AM, Pekka Paalanen wrote:
> > On Tue, 8 Nov 2022 23:01:47 +0100
> > Sebastian Wick wrote:
> >
> >> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser wrote:
> >>>
> >>> cc'ing Pekka and wayland-devel for userspace devs feedback on the new
> >>> uAPI.
> >
> > Hi all,
> >
> > thanks! Comments below.
>
> Thanks for the feedback!
>
> >
> >>>
> >>> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov
> >>> wrote:
> >>>
> >>>> On 29/10/2022 01:59, Jessica Zhang wrote:
> >>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> >>>>> drm_plane. In addition, add support for setting and getting the values
> >>>>> of these properties.
> >>>>>
> >>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> >>>>> represents the format of the color fill. Userspace can set enable solid
> >>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> >>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> >>>>> framebuffer to NULL.
> >>>>>
> >>>>> Signed-off-by: Jessica Zhang
> >>>>
> >>>> Planes report supported formats using the drm_mode_getplane(). You'd
> >>>> also need to tell userspace, which formats are supported for color fill.
> >>>> I don't think one supports e.g. YV12.
> >>>>
> >>>> A bit of generic comment for the discussion (this is an RFC anyway).
> >>>> Using color_fill/color_fill_format properties sounds simple, but this
> >>>> might be not generic enough. Limiting color_fill to 32 bits would
> >>>> prevent anybody from using floating point formats (e.g.
> >>>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> >>>> e.g. using 64-bit for the color_fill value, but then this doesn't sound
> >>>> extensible too much.
> >>>>
> >>>> So, a question for other hardware maintainers. Do we have hardware that
> >>>> supports such 'color filled' planes? Do we want to support format
> >>>> modifiers for filling color/data? Because what I have in mind is closer
> >>>> to the blob structure, which can then be used for filling the plane:
> >>>>
> >>>> struct color_fill_blob {
> >>>> u32 pixel_format;
> >>>> u64 modifiers4];
> >>>> u32 pixel_data_size; // fixme: is this necessary?
> >>>> u8 pixel_data[];
> >>>> };
> >>>>
> >>>> And then... This sounds a lot like a custom framebuffer.
> >>>>
> >>>> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> >>>> flag to the framebuffers, which would e.g. mean that the FB gets stamped
> >>>> all over the plane. This would also save us from changing if (!fb)
> >>>> checks all over the drm core.
> >>>>
> >>>> Another approach might be using a format modifier instead of the FB flag.
> >>>>
> >>>> What do you think?
> >>>
> >>> First off, we only need to represent the value of a single pixel here. So
> >>> I'm
> >>> not quite following why we need format modifiers. Format modifiers
> >>> describe how
> >>> pixels are laid out in memory. Since there's a single pixel described,
> >>> this
> >>> is non-sensical to me, the format modifier is always LINEAR.
> >
> > Agreed.
> >
> >>>
> >>> Then, I can understand why putting the pixel_format in there is tempting
> >>> to
> >>> guarantee future extensibility, but it also adds complexity. For
> >>> instance, how
> >>> does user-space figure out which formats can be used for COLOR_FILL? Can
> >>> user-space use any format supported by the plane? What does it mean for
> >>> multi-planar formats? Do we really want the kernel to have conversion
> >>> logic for
> >>> all existing formats? Do we need to also add a new read-only blob prop to
> >>> indicate supported COLOR_FILL formats?
>
> FWIW the formats supported by solid_fill
Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On Tue, 8 Nov 2022 23:01:47 +0100
Sebastian Wick wrote:
> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser wrote:
> >
> > cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
Hi all,
thanks! Comments below.
> >
> > On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov
> > wrote:
> >
> > > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > > drm_plane. In addition, add support for setting and getting the values
> > > > of these properties.
> > > >
> > > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > > represents the format of the color fill. Userspace can set enable solid
> > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > > framebuffer to NULL.
> > > >
> > > > Signed-off-by: Jessica Zhang
> > >
> > > Planes report supported formats using the drm_mode_getplane(). You'd
> > > also need to tell userspace, which formats are supported for color fill.
> > > I don't think one supports e.g. YV12.
> > >
> > > A bit of generic comment for the discussion (this is an RFC anyway).
> > > Using color_fill/color_fill_format properties sounds simple, but this
> > > might be not generic enough. Limiting color_fill to 32 bits would
> > > prevent anybody from using floating point formats (e.g.
> > > DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> > > e.g. using 64-bit for the color_fill value, but then this doesn't sound
> > > extensible too much.
> > >
> > > So, a question for other hardware maintainers. Do we have hardware that
> > > supports such 'color filled' planes? Do we want to support format
> > > modifiers for filling color/data? Because what I have in mind is closer
> > > to the blob structure, which can then be used for filling the plane:
> > >
> > > struct color_fill_blob {
> > > u32 pixel_format;
> > > u64 modifiers4];
> > > u32 pixel_data_size; // fixme: is this necessary?
> > > u8 pixel_data[];
> > > };
> > >
> > > And then... This sounds a lot like a custom framebuffer.
> > >
> > > So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> > > flag to the framebuffers, which would e.g. mean that the FB gets stamped
> > > all over the plane. This would also save us from changing if (!fb)
> > > checks all over the drm core.
> > >
> > > Another approach might be using a format modifier instead of the FB flag.
> > >
> > > What do you think?
> >
> > First off, we only need to represent the value of a single pixel here. So
> > I'm
> > not quite following why we need format modifiers. Format modifiers describe
> > how
> > pixels are laid out in memory. Since there's a single pixel described, this
> > is non-sensical to me, the format modifier is always LINEAR.
Agreed.
> >
> > Then, I can understand why putting the pixel_format in there is tempting to
> > guarantee future extensibility, but it also adds complexity. For instance,
> > how
> > does user-space figure out which formats can be used for COLOR_FILL? Can
> > user-space use any format supported by the plane? What does it mean for
> > multi-planar formats? Do we really want the kernel to have conversion logic
> > for
> > all existing formats? Do we need to also add a new read-only blob prop to
> > indicate supported COLOR_FILL formats?
Right. This does not seem to require pixel formats at all.
The point of pixel formats is to be able to feed large amounts of data
as-is into hardware and avoid the CPU ever touching it. You do that
with DRM FBs pointing to suitably allocated hardware buffers. But here
we have exactly one pixel, which I imagine will always be read by the
CPU so the driver will convert it into a hardware-specific format and
program it; probably the driver will not create an internal DRM FB for
it.
The above might also be a reason to not model this as a special-case
DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
DRM FB to avoid the need to allocate e.g. a dumb buffer or a
GPU-specific buffer.
What one does need is what Sebastian brought up: does it support alpha
or not?
Userspace would also be interested in the supported precision of the
values, but the hardware pixel component order is irrelevant because the
driver will always convert the one pixel with CPU anyway.
YUV vs. RGB is a another question. The KMS color pipeline is defined in
terms of RGBA as far as I know, and alpha-blending YUV values makes no
sense. So will there ever be any need to set an YUV fill? I have a hard
time imagining it.
If you do set an YUV fill, the KMS color pipeline most likely needs to
convert it to RGBA for blending, and then you need the plane properties
COLOR_ENCODING and COLOR_RANGE.
But why bother when userspace can convert that one pixel to RGBA itself
anyway?
> > We've recently-ish standardized a new Wayland protocol [1] w
