Re: [Freedreno] [PATCH RFC v6 01/10] drm: Introduce pixel_source DRM plane property
On Tue, Aug 29, 2023 at 10:48:16AM +0300, Pekka Paalanen wrote: > 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 Jessica, same for me Acked-by: Sebastian Wick > > > Thanks, > pq
Re: [Freedreno] [PATCH RFC v6 00/10] Support for Solid Fill Planes
On Mon, Aug 28, 2023 at 05:05:06PM -0700, Jessica Zhang wrote: > 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. > > In order to expose this capability to userspace, this series will: > > - Introduce solid_fill and pixel_source properties to allow userspace to > toggle between FB and solid fill sources > - Loosen NULL FB checks within the DRM atomic commit callstack to allow > for NULL FB when solid fill is enabled. > - Add NULL FB checks in methods where FB was previously assumed to be > non-NULL > - Have MSM DPU driver use drm_plane_state.solid_fill instead of > dpu_plane_state.color_fill > > Note: The solid fill planes feature depends on both the solid_fill *and* > pixel_source properties. > > To use this feature, userspace can set the solid_fill property to a blob > containing the appropriate version number and solid fill color (in > RGB323232 format) and and setting the pixel_source property to > DRM_PLANE_PIXEL_SOURCE_COLOR. This will disable memory fetch and the > resulting plane will display the color specified by the solid_fill blob. > > 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. > > This 2 property approach was chosen because passing in a special 1x1 FB > with the necessary color information would have unecessary overhead that > does not reflect the behavior of the solid fill feature. In addition, > assigning the solid fill blob to FB_ID would require loosening some core > drm_property checks that might cause unwanted side effects elsewhere. The cover letter is a bit outdated by now. Anyway, with Pekkas issues addressed the core drm parts are Acked-by: Sebastian Wick > --- > Changes in v6: > - Have _dpu_plane_color_fill() take in a single ABGR color instead > of having separate alpha and BGR color parameters (Dmitry) > - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check > in SetPlane ioctl (Dmitry) > - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian) > - Dropped versioning from solid fill property blob (Dmitry) > - Use DRM_ENUM_NAME_FN (Dmitry) > - Use drm_atomic_replace_property_blob_from_id() (Dmitry) > - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry) > - Group redundant NULL FB checks (Dmitry) > - Squashed drm_plane_needs_disable() implementation with > DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian) > - Add comment to support RGBA solid fill color in the future (Dmitry) > - Link to v5: > https://lore.kernel.org/r/20230728-solid-fill-v5-0-053dbefa9...@quicinc.com > > Changes in v5: > - Added support for PIXEL_SOURCE_NONE (Sebastian) > - Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't > set (Dmitry) > - Added debugfs support for both properties (Dmitry) > - Corrected u32 to u8 conversion (Pekka) > - Moved drm_solid_fill_info struct and related documentation to > include/uapi (Pekka) > - Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka) > - Added more detailed UAPI and kernel documentation (Pekka) > - Reordered patch series so that the pixel_source property is introduced > before solid_fill (Dmitry) > - Fixed inconsistent ABGR/RGBA format declaration (Pekka) > - Reset pixel_source to FB in drm_mode_setplane() (Dmitry) > - Rename supported_sources to extra_sources (Dmitry) > - Only destroy old solid_fill blob state if new state is valid (Pekka) > - Link to v4: > https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa7...@quicinc.com > > Changes in v4: > - Rebased onto latest kernel > - Reworded cover letter for clarity (Dmitry) > - Reworded commit messages for clarity > - Split existing changes into smaller commits > - Added pixel_source enum property (Dmitry, Pekka, Ville) > - Updated drm-kms comment docs with pixel_source and solid_fill > properties (Dmitry) > - Inlined drm_atomic_convert_solid_fill_info() (Dmitry) > - Passed in plane state alpha value to _dpu_plane_color_fill_pipe() > - Link to v3: > https://lore.kernel.org/r/20230104234036.636-1-quic_jessz...@quicinc.com > > Changes in v3: > - Fixed some logic errors in atomic checks (Dmitry) > - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper > methods (Dmitry) > - Fixed typo in drm_solid_fill struct documentation &
Re: [Freedreno] [PATCH RFC v6 02/10] drm: Introduce solid fill DRM plane property
On Mon, Aug 28, 2023 at 05:05:08PM -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_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 01638c51ce0a..86fb876efbe6 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
> 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);
> + plane_state->solid_fill_blob = NULL;
> + }
> +
> if (plane->color_encoding_property) {
> if (!drm_object_property_get_default_value(>base,
>
> plane->color_encoding_property,
> @@ -336,6 +341,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;
> @@ -385,6 +393,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 454f980e16c9..1cae596ab693 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -316,6 +316,20 @@ drm_atomic_set_crtc_for_connector(struct
> drm_connector_state *conn_state,
> }
> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>
> +static void drm_atomic_set_solid_fill_prop(struct drm_plane_state *state)
> +{
> + struct drm_mode_solid_fill *user_info;
> +
> + if (!state->solid_fill_blob)
> + return;
> +
> + user_info = (struct drm_mode_solid_fill *)state->solid_fill_blob->data;
> +
> + state->solid_fill.r = user_info->r;
> + state->solid_fill.g = user_info->g;
> + state->solid_fill.b = user_info->b;
> +}
> +
> static void set_out_fence_for_crtc(struct drm_atomic_state *state,
> struct drm_crtc *crtc, s32 __user *fence_ptr)
> {
> @@ -546,6 +560,15 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> state->src_h = val;
> } else if (property == plane->pixel_source_property) {
> state->pixel_source = val;
> + } else if (property == plane->solid_fill_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >solid_fill_blob,
> + val, sizeof(struct drm_mode_solid_fill),
> + -1, );
> + if (ret)
> + return ret;
> +
> + drm_atomic_set_solid_fill_prop(state);
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -620,6 +643,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->src_h;
> } else if (property == plane->pixel_source_property) {
> *val = state->pixel_source;
> + } else if (property == plane->solid_fill_property) {
> + *val = state->solid_fill_blob ?
> + state->solid_fill_blob->base.id : 0;
> } 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 c3c57bae06b7..273021cc21c8 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -200,6 +200,10 @@
Re: [Freedreno] [PATCH RFC v5 01/10] drm: Introduce pixel_source DRM plane property
On Mon, Aug 7, 2023 at 7:52 PM Jessica Zhang wrote:
>
>
>
> On 8/4/2023 6:15 AM, Sebastian Wick wrote:
> > On Fri, Jul 28, 2023 at 7:03 PM 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.
> >>
> >> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_NONE and
> >> DRM_PLANE_PIXEL_SOURCE_FB with *_PIXEL_SOURCE_FB being the default value.
> >>
> >> 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 | 85
> >> +++
> >> drivers/gpu/drm/drm_plane.c | 3 ++
> >> include/drm/drm_blend.h | 2 +
> >> include/drm/drm_plane.h | 21
> >> 6 files changed, 116 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> index 784e63d70a42..01638c51ce0a 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->color_encoding_property) {
> >> if (!drm_object_property_get_default_value(>base,
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index d867e7f9f2cd..454f980e16c9 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -544,6 +544,8 @@ static int drm_atomic_plane_set_property(struct
> >> drm_plane *plane,
> >> state->src_w = val;
> >> } else if (property == config->prop_src_h) {
> >> state->src_h = val;
> >> + } 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) {
> >> @@ -616,6 +618,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >> *val = state->src_w;
> >> } else if (property == config->prop_src_h) {
> >> *val = state->src_h;
> >> + } 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 6e74de833466..c500310a3d09 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
Re: [Freedreno] [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
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. Maybe another COLOR_FILL enum value
with alpha might be better? Maybe just doing the alpha via the alpha
property is good enough.
>
> --
> With best wishes
> Dmitry
>
Re: [Freedreno] [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Mon, Jul 31, 2023 at 6:01 AM Dmitry Baryshkov
wrote:
>
> On 28/07/2023 20:02, 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(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> > b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 01638c51ce0a..86fb876efbe6 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> > drm_plane_state *plane_state,
> > 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);
> > + plane_state->solid_fill_blob = NULL;
> > + }
> > +
> > if (plane->color_encoding_property) {
> > if (!drm_object_property_get_default_value(>base,
> >
> > plane->color_encoding_property,
> > @@ -336,6 +341,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;
> > @@ -385,6 +393,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 454f980e16c9..039686c78c2a 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 blob_version;
> > +
> > + if (blob == state->solid_fill_blob)
> > + return 0;
> > +
> > + if (blob) {
> > + struct drm_mode_solid_fill *user_info = (struct
> > drm_mode_solid_fill *)blob->data;
> > +
> > + if (blob->length != sizeof(struct drm_mode_solid_fill)) {
> > + 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;
>
> I remember that we had versioning for quite some time. However it stroke
> me while reviewing that we do not a way to negotiate supported
> SOLID_FILL versions. However as we now have support for different
> pixel_source properties, maybe we can drop version completely. If at
> some point a driver needs to support different kind of SOLID_FILL
> property (consider v2), we can add new pixel source to the enum.
Agreed. Let's drop the versioning.
>
> > +
> > + /* 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] unsupported solid fill
> > version (version=%d)\n",
> > +state->plane->base.id,
> > state->plane->name,
> > +
Re: [Freedreno] [PATCH RFC v5 01/10] drm: Introduce pixel_source DRM plane property
On Fri, Jul 28, 2023 at 7:03 PM 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.
>
> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_NONE and
> DRM_PLANE_PIXEL_SOURCE_FB with *_PIXEL_SOURCE_FB being the default value.
>
> 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 | 85
> +++
> drivers/gpu/drm/drm_plane.c | 3 ++
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 21
> 6 files changed, 116 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..01638c51ce0a 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->color_encoding_property) {
> if (!drm_object_property_get_default_value(>base,
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd..454f980e16c9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -544,6 +544,8 @@ static int drm_atomic_plane_set_property(struct drm_plane
> *plane,
> state->src_w = val;
> } else if (property == config->prop_src_h) {
> state->src_h = val;
> + } 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) {
> @@ -616,6 +618,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->src_w;
> } else if (property == config->prop_src_h) {
> *val = state->src_h;
> + } 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 6e74de833466..c500310a3d09 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).
> @@ -615,3 +630,73 @@ int drm_plane_create_blend_mode_property(struct
> drm_plane *plane,
> return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> +
> +/**
> + * drm_plane_create_pixel_source_property - create a new pixel source
> property
> + * @plane: DRM plane
> + * @extra_sources: Bitmask of additional supported pixel_sources for the
> driver.
> + *DRM_PLANE_PIXEL_SOURCE_FB always be enabled as a supported
> + *source.
> + *
> + * This creates a new property describing the current source of pixel data
> for the
> + * plane. The pixel_source will be initialized as DRM_PLANE_PIXEL_SOURCE_FB
> by default.
> + *
> + * Drivers can set a custom default source by overriding the pixel_source
> value in
> + * drm_plane_funcs.reset()
> + *
> + * The property is exposed to userspace as an enumeration property called
> + * "pixel_source" and has the following enumeration values:
> + *
> + * "NONE":
> + * No active pixel source
> + *
> + * "FB":
> + * Framebuffer
Re: [Freedreno] [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Fri, Jun 30, 2023 at 11:27 PM Jessica Zhang
wrote:
>
>
>
> On 6/30/2023 7:43 AM, Sebastian Wick wrote:
> > On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang
> > wrote:
> >>
> >> Add support for pixel_source property to drm_plane and related
> >> documentation.
> >>
> >> This enum property will allow user to specify a pixel source for the
> >> plane. Possible pixel sources will be defined in the
> >> drm_plane_pixel_source enum.
> >>
> >> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> >> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
> >>
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> >> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> >> drivers/gpu/drm/drm_blend.c | 81
> >> +++
> >> include/drm/drm_blend.h | 2 +
> >> include/drm/drm_plane.h | 21
> >> 5 files changed, 109 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> index fe14be2bd2b2..86fb876efbe6 100644
> >> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
> >> drm_plane_state *plane_state,
> >>
> >> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> >> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
> >>
> >> if (plane_state->solid_fill_blob) {
> >> drm_property_blob_put(plane_state->solid_fill_blob);
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index a28b4ee79444..6e59c21af66b 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct
> >> drm_plane *plane,
> >> drm_property_blob_put(solid_fill);
> >>
> >> return ret;
> >> + } else if (property == plane->pixel_source_property) {
> >> + state->pixel_source = val;
> >> } else if (property == plane->alpha_property) {
> >> state->alpha = val;
> >> } else if (property == plane->blend_mode_property) {
> >> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >> } else if (property == plane->solid_fill_property) {
> >> *val = state->solid_fill_blob ?
> >> state->solid_fill_blob->base.id : 0;
> >> + } else if (property == plane->pixel_source_property) {
> >> + *val = state->pixel_source;
> >> } else if (property == plane->alpha_property) {
> >> *val = state->alpha;
> >> } else if (property == plane->blend_mode_property) {
> >> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> >> index 38c3c5d6453a..8c100a957ee2 100644
> >> --- a/drivers/gpu/drm/drm_blend.c
> >> +++ b/drivers/gpu/drm/drm_blend.c
> >> @@ -189,6 +189,18 @@
> >>* solid_fill is set up with drm_plane_create_solid_fill_property().
> >> It
> >>* contains pixel data that drivers can use to fill a plane.
> >>*
> >> + * pixel_source:
> >> + * pixel_source is set up with
> >> drm_plane_create_pixel_source_property().
> >> + * It is used to toggle the source of pixel data for the plane.
> >> + *
> >> + * Possible values:
> >> + *
> >> + * "FB":
> >> + * Framebuffer source
> >> + *
> >> + * "COLOR":
> >> + * solid_fill source
> >> + *
> >>* Note that all the property extensions described here apply either to
> >> the
> >>* plane or the CRTC (e.g. for the background color, which currently is
> >> not
> >>* exposed and assumed to be black).
> >> @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct
> >> drm_plane *plane)
> >> return 0;
> >
Re: [Freedreno] [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang wrote:
>
> Add support for pixel_source property to drm_plane and related
> documentation.
>
> This enum property will allow user to specify a pixel source for the
> plane. Possible pixel sources will be defined in the
> drm_plane_pixel_source enum.
>
> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_blend.c | 81
> +++
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 21
> 5 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index fe14be2bd2b2..86fb876efbe6 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
>
> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
>
> if (plane_state->solid_fill_blob) {
> drm_property_blob_put(plane_state->solid_fill_blob);
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a28b4ee79444..6e59c21af66b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane
> *plane,
> drm_property_blob_put(solid_fill);
>
> return ret;
> + } else if (property == plane->pixel_source_property) {
> + state->pixel_source = val;
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> } else if (property == plane->solid_fill_property) {
> *val = state->solid_fill_blob ?
> state->solid_fill_blob->base.id : 0;
> + } else if (property == plane->pixel_source_property) {
> + *val = state->pixel_source;
> } else if (property == plane->alpha_property) {
> *val = state->alpha;
> } else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 38c3c5d6453a..8c100a957ee2 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -189,6 +189,18 @@
> * solid_fill is set up with drm_plane_create_solid_fill_property(). It
> * contains pixel data that drivers can use to fill a plane.
> *
> + * pixel_source:
> + * pixel_source is set up with drm_plane_create_pixel_source_property().
> + * It is used to toggle the source of pixel data for the plane.
> + *
> + * Possible values:
> + *
> + * "FB":
> + * Framebuffer source
> + *
> + * "COLOR":
> + * solid_fill source
> + *
> * Note that all the property extensions described here apply either to the
> * plane or the CRTC (e.g. for the background color, which currently is not
> * exposed and assumed to be black).
> @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct
> drm_plane *plane)
> return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
> +
> +/**
> + * drm_plane_create_pixel_source_property - create a new pixel source
> property
> + * @plane: drm plane
> + * @supported_sources: bitmask of supported pixel_sources for the driver (NOT
> + * including DRM_PLANE_PIXEL_SOURCE_FB, as it will be
> supported
> + * by default).
> + *
> + * This creates a new property describing the current source of pixel data
> for the
> + * plane.
> + *
> + * The property is exposed to userspace as an enumeration property called
> + * "pixel_source" and has the following enumeration values:
> + *
> + * "FB":
> + * Framebuffer pixel source
> + *
> + * "COLOR":
> + * Solid fill color pixel source
Can we add a "NONE" value?
I know it has been discussed earlier if we *need* this and I don't
think we do. I just think it would be better API design to disable
planes this way than having to know that a framebuffer pixel source
with a NULL framebuffer disables the plane. Obviously also keep the
old behavior for backwards compatibility.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_plane_create_pixel_source_property(struct drm_plane *plane,
> + unsigned int supported_sources)
> +{
> +
Re: [Freedreno] [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Thu, Mar 16, 2023 at 11:59 PM Rob Clark wrote: > > On Thu, Mar 16, 2023 at 3:22 PM 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 > > > > > > > > > ~~~
Re: [Freedreno] [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
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 > > > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > > > @@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence > > > > > > > **fences, uint32_t count, > > > > > > > } > > > > > > > EXPORT_SYMBOL(dma_fence_wait_any_timeout); > > > > > > > > > > > > > > +/** > > > > > > > + * DOC: deadline hints > > > > > > > + * > > > > > > > + * In an ideal world, it would be possible to pipeline a > > > > > > > workload sufficiently > > > > > > > + * that a utilization based device frequency governor could > > > > > > > arrive at a minimum > > > > > > > + * frequency that meets the requirements of the use-case, in > > > > > > > order to minimize > > > > > > > + * power consumption. But in the real world there are many > > > > > > > workloads which > > > > > > > + * defy this ideal. For example, but not limited to: > > > > > > > + * > > > > > > > + * * Workloads that ping-pong between device and CPU, with > > > > > > > alternating periods > > > > > > > + * of CPU waiting for device, and device waiting on CPU. This > > > > > > > can result in > > > > > > > + * devfreq and cpufreq seeing idle time in their respective > > > > > > > domains and in > > > > > > > + * result reduce frequency. > > > > > > > + * > > > > > > > + * * Workloads that interact with a periodic time based > > > > > > > deadline, such as double > > > > > > > + * buffered GPU rendering vs vblank sync'd page flipping. In > > > > > > > this scenario, > > > > > > > + * missing a vblank deadline results in an *increase* in idle > > > > > > > time on the GPU > > > > > > > + * (since it has to wait an additional vblank period), sending > > > > > > > a signal to > > > > > > > + * the GPU's devfreq to reduce frequency, when in fact the > > > > > > > opposite is what is > > > > > > > + * needed. > > > > > > > > > > > > This is the use case I'd like to get some
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 28, 2023 at 11:52 PM Rob Clark wrote: > > On Tue, Feb 28, 2023 at 6:30 AM Sebastian Wick > wrote: > > > > On Tue, Feb 28, 2023 at 12:48 AM Rob Clark wrote: > > > > > > On Mon, Feb 27, 2023 at 2:44 PM Sebastian Wick > > > wrote: > > > > > > > > On Mon, Feb 27, 2023 at 11:20 PM Rob Clark wrote: > > > > > > > > > > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi > > > > > wrote: > > > > > > > > > > > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote: > > > > > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov > > > > > > > wrote: > > > > > > > > > > > > > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote: > > > > > > > > >> 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 ig
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 28, 2023 at 12:48 AM Rob Clark wrote: > > On Mon, Feb 27, 2023 at 2:44 PM Sebastian Wick > wrote: > > > > On Mon, Feb 27, 2023 at 11:20 PM Rob Clark wrote: > > > > > > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi > > > wrote: > > > > > > > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote: > > > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov > > > > > wrote: > > > > > > > > > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote: > > > > > > >> 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 m
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Mon, Feb 27, 2023 at 11:20 PM Rob Clark wrote: > > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi wrote: > > > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote: > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov wrote: > > > > > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote: > > > > > > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote: > > > > >> 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
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 21, 2023 at 9:38 AM Pekka Paalanen wrote:
>
> 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.
Technically we don't know when the commit is supposed to be shown.
Just passing a deadline of the next possible deadline however is
probably a good enough guess for this feature to be useful.
One thing that neither API allows us to do is tell the kernel in
advance when we're going to submit work and what the deadline for it
is and unfortunately that work is the most timing sensitive.
>
>
> 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, _file->wq, wait);
> > > >
> > > > if (list_empty(_file->cb.node) &&
> > >
>
Re: [Freedreno] [RFC PATCH v2 3/3] drm/msm/dpu: Use color_fill property for DPU planes
On Wed, Jan 4, 2023 at 2:10 AM Jessica Zhang wrote:
>
>
>
> On 12/22/2022 7:12 PM, Dmitry Baryshkov wrote:
> > On 23/12/2022 00:14, Jessica Zhang wrote:
> >> Initialize and use the color_fill properties for planes in DPU driver. In
> >> addition, relax framebuffer requirements within atomic commit path and
> >> add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG
> >> as it's unused.
> >>
> >> Changes since V2:
> >> - Fixed dropped 'const' warning
> >> - Dropped use of solid_fill_format
> >> - Switched to using drm_plane_solid_fill_enabled helper method
> >> - Added helper to convert color fill to BGR888 (Rob)
> >> - Added support for solid fill on planes of varying sizes
> >> - Removed DPU_PLANE_COLOR_FILL_FLAG
> >>
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++-
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++-
> >> 2 files changed, 49 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> index 13ce321283ff..0695b70ea1b7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> @@ -409,6 +409,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;
> >> @@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct
> >> drm_crtc *crtc,
> >> sspp_idx - SSPP_VIG0,
> >> state->fb ? state->fb->base.id : -1);
> >> -format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> >> +if (pstate->base.fb)
> >> +fmt = msm_framebuffer_format(pstate->base.fb);
> >> +else
> >> +fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
> >> +DRM_FORMAT_ABGR, 0);
> >> +
> >> +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 86719020afe2..51a7507373f7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> @@ -44,7 +44,6 @@
> >> #define DPU_NAME_SIZE 12
> >> -#define DPU_PLANE_COLOR_FILL_FLAGBIT(31)
> >> #define DPU_ZPOS_MAX 255
> >> /* multirect rect index */
> >> @@ -105,7 +104,6 @@ struct dpu_plane {
> >> enum dpu_sspp pipe;
> >> struct dpu_hw_pipe *pipe_hw;
> >> -uint32_t color_fill;
> >> bool is_error;
> >> bool is_rt_pipe;
> >> const struct dpu_mdss_cfg *catalog;
> >> @@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct
> >> dpu_plane *pdpu,
> >> _cfg);
> >> }
> >> +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);
> >> +
> >> +return ret;
> >> +}
> >> +
> >> /**
> >>* _dpu_plane_color_fill - enables color fill on plane
> >>* @pdpu: Pointer to DPU plane object
> >> @@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >> dst = drm_plane_state_dest(new_plane_state);
> >> -fb_rect.x2 = new_plane_state->fb->width;
> >> -fb_rect.y2 = new_plane_state->fb->height;
> >> +if (new_plane_state->fb) {
> >> +fb_rect.x2 = new_plane_state->fb->width;
> >> +fb_rect.y2 = new_plane_state->fb->height;
> >> +}
> >> max_linewidth = pdpu->catalog->caps->max_linewidth;
> >> -fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +if (new_plane_state->fb)
> >> +fmt =
> >> to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >> +else
> >> +fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
> >> min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >> @@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct
> >> drm_plane *plane,
> >> return -EINVAL;
> >> /* check src bounds */
> >> -} else if (!dpu_plane_validate_src(, _rect, min_src_size)) {
> >> +} else if (new_plane_state->fb && !dpu_plane_validate_src(,
> >> _rect, min_src_size)) {
> >> DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
> >> DRM_RECT_ARG());
> >> return -E2BIG;
> >> @@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane)
> >> if (pdpu->is_error)
> >> /* force white frame with
Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
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.
>
> 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.
>
> 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?
>
> We've recently-ish standardized a new Wayland protocol [1] which has the same
> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
> values for each channel (R, G, B, A) would be enough for almost all use-cases.
> The driver can convert these high-precision values to what the hardware
> expects.
> The only concern was about sending values outside of the [0.0, 1.0] range,
> which may have HDR use-cases.
>
> So, there are multiple ways to go about this. I can think of:
>
> - Put "RGBA32" in the name of the prop, and if we ever need a different
> color format, pick a different name.
> - Define a struct with an enum of possible fill kinds:
> #define FILL_COLOR_RGBA32 1
> #define FILL_COLOR_F32 2
> struct color_fill_blob { u32 kind; u8 data[]; };
> - Define a struct with a version and RGBA values:
> struct color_fill_blob { u32 version; u32 rgba[4]; };
> If we need to add more formats later, or new metadata:
> struct color_fill_blob2 { u32 version; /* new fields */ };
> where version must be set to 2.
> - Define a struct with a "pixel_format" prop, but force user-space to use a
> fixed format for now. Later, if we need another format, add a new prop to
> advertise supported formats.
> - More complicated solutions, e.g. advertise the list of supported formats
> from
> the start.
>
> [1]:
> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>
Agreeing with most of what you said here. However, what's the idea
behind a format anyway? The 4 values provided here are fed directly
into the color pipeline which seems to define the color channels it's
working on as RGBA (or doesn't define anything at all). The only
reason I can think of is that hardware might support only ingesting
values either in a format
