On Fri, 30 Jun 2023 03:42:28 +0300 Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 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 <quic_jesszhan@xxxxxxxxxxx> > > --- > > 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 suggest either renaming this to > extra_sources (mentioning that FB is enabled for all the planes) or > allowing any source bitmask (mentioning that FB should be enabled by the > caller, unless there is a good reason not to do so). Right. I don't see any problem with having planes of type OVERLAY that support only solid_fill and no FB. Planes of type PRIMARY and CURSOR I would expect to always support at least FB. Atomic userspace is prepared to have an OVERLAY plane fail for any arbitrary reason. Legacy userspace probably should not ever see a plane that does not support FB. > > + * > > + * 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 > > + * > > + * Returns: > > + * Zero on success, negative errno on failure. > > + */ > > +int drm_plane_create_pixel_source_property(struct drm_plane *plane, > > + unsigned int supported_sources) > > +{ > > + struct drm_device *dev = plane->dev; > > + struct drm_property *prop; > > + const struct drm_prop_enum_list enum_list[] = { > > + { DRM_PLANE_PIXEL_SOURCE_FB, "FB" }, > > + { DRM_PLANE_PIXEL_SOURCE_COLOR, "COLOR" }, > > + }; > > + unsigned int valid_source_mask = BIT(DRM_PLANE_PIXEL_SOURCE_FB) | > > + BIT(DRM_PLANE_PIXEL_SOURCE_COLOR); > > > static const? > > > + int i; > > + > > + /* FB is supported by default */ > > + supported_sources |= BIT(DRM_PLANE_PIXEL_SOURCE_FB); > > + > > + if (WARN_ON(supported_sources & ~valid_source_mask)) > > + return -EINVAL; > > + > > + prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, "pixel_source", Shouldn't this be an atomic prop? > > + hweight32(supported_sources)); > > + > > + if (!prop) > > + return -ENOMEM; > > + > > + for (i = 0; i < ARRAY_SIZE(enum_list); i++) { > > + int ret; > > + > > + if (!(BIT(enum_list[i].type) & supported_sources)) > > test_bit? > > > + continue; > > + > > + ret = drm_property_add_enum(prop, enum_list[i].type, enum_list[i].name); > > + > > No need for an empty line in such cases. Please drop it. > > > + if (ret) { > > + drm_property_destroy(dev, prop); > > + > > + return ret; > > + } > > + } > > + > > + drm_object_attach_property(&plane->base, prop, DRM_PLANE_PIXEL_SOURCE_FB); > > + plane->pixel_source_property = prop; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(drm_plane_create_pixel_source_property); > > diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h > > index 0338a860b9c8..31af7cfa5b1b 100644 > > --- a/include/drm/drm_blend.h > > +++ b/include/drm/drm_blend.h > > @@ -59,4 +59,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > > 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); > > +int drm_plane_create_pixel_source_property(struct drm_plane *plane, > > + unsigned int supported_sources); > > #endif > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > index f6ab313cb83e..73fb6cf8a5d9 100644 > > --- a/include/drm/drm_plane.h > > +++ b/include/drm/drm_plane.h > > @@ -59,6 +59,12 @@ struct drm_solid_fill { > > uint32_t b; > > }; > > > > +enum drm_plane_pixel_source { > > + DRM_PLANE_PIXEL_SOURCE_FB, > > + DRM_PLANE_PIXEL_SOURCE_COLOR, > > + DRM_PLANE_PIXEL_SOURCE_MAX > > +}; Just to be very clear that I'm not confusing you with my comment about UAPI headers in the previous patch, this enum is already in a good place. It does not belong in a UAPI header, because userspace recognises enum values by the name string. Thanks, pq > > + > > /** > > * struct drm_plane_state - mutable plane state > > * > > @@ -152,6 +158,14 @@ struct drm_plane_state { > > */ > > struct drm_solid_fill solid_fill; > > > > + /* > > + * @pixel_source: > > + * > > + * Source of pixel information for the plane. See > > + * drm_plane_create_pixel_source_property() for more details. > > + */ > > + enum drm_plane_pixel_source pixel_source; > > + > > /** > > * @alpha: > > * Opacity of the plane with 0 as completely transparent and 0xffff as > > @@ -742,6 +756,13 @@ struct drm_plane { > > */ > > struct drm_property *solid_fill_property; > > > > + /* > > + * @pixel_source_property: > > + * Optional pixel_source property for this plane. See > > + * drm_plane_create_pixel_source_property(). > > + */ > > + struct drm_property *pixel_source_property; > > + > > /** > > * @alpha_property: > > * Optional alpha property for this plane. See > > >
Attachment:
pgpaLaJniHEYC.pgp
Description: OpenPGP digital signature