| Message ID | 20230404-solid-fill-v4-0-f4ec0caa742d@quicinc.com |
|---|---|
| Headers | show |
| Series | Support for Solid Fill Planes | expand |
On 30/06/2023 03:25, Jessica Zhang wrote: > Currently framebuffer checks happen directly in > drm_atomic_plane_check(). Move these checks into their own helper > method. > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/drm_atomic.c | 130 ++++++++++++++++++++++++------------------- > 1 file changed, 74 insertions(+), 56 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index b4c6ffc438da..404b984d2d9f 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state *old_plane_state, > return true; > } > > +static int drm_atomic_check_fb(const struct drm_plane_state *state) > +{ > + struct drm_plane *plane = state->plane; > + const struct drm_framebuffer *fb = state->fb; > + struct drm_mode_rect *clips; > + > + uint32_t num_clips; > + unsigned int fb_width, fb_height; > + int ret; > + > + /* Check whether this plane supports the fb pixel format. */ > + ret = drm_plane_check_pixel_format(plane, fb->format->format, > + fb->modifier); > + > + if (ret) { > + drm_dbg_atomic(plane->dev, > + "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n", > + plane->base.id, plane->name, > + &fb->format->format, fb->modifier); > + return ret; > + } > + > + fb_width = fb->width << 16; > + fb_height = fb->height << 16; > + > + /* Make sure source coordinates are inside the fb. */ > + if (state->src_w > fb_width || > + state->src_x > fb_width - state->src_w || > + state->src_h > fb_height || > + state->src_y > fb_height - state->src_h) { > + drm_dbg_atomic(plane->dev, > + "[PLANE:%d:%s] invalid source coordinates " > + "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n", > + plane->base.id, plane->name, > + state->src_w >> 16, > + ((state->src_w & 0xffff) * 15625) >> 10, > + state->src_h >> 16, > + ((state->src_h & 0xffff) * 15625) >> 10, > + state->src_x >> 16, > + ((state->src_x & 0xffff) * 15625) >> 10, > + state->src_y >> 16, > + ((state->src_y & 0xffff) * 15625) >> 10, > + fb->width, fb->height); > + return -ENOSPC; > + } > + > + clips = __drm_plane_get_damage_clips(state); > + num_clips = drm_plane_get_damage_clips_count(state); > + > + /* Make sure damage clips are valid and inside the fb. */ > + while (num_clips > 0) { > + if (clips->x1 >= clips->x2 || > + clips->y1 >= clips->y2 || > + clips->x1 < 0 || > + clips->y1 < 0 || > + clips->x2 > fb_width || > + clips->y2 > fb_height) { > + drm_dbg_atomic(plane->dev, > + "[PLANE:%d:%s] invalid damage clip %d %d %d %d\n", > + plane->base.id, plane->name, clips->x1, > + clips->y1, clips->x2, clips->y2); > + return -EINVAL; > + } > + clips++; > + num_clips--; > + } > + > + return 0; > +} > + > /** > * drm_atomic_plane_check - check plane state > * @old_plane_state: old plane state to check > @@ -596,9 +666,6 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, > struct drm_plane *plane = new_plane_state->plane; > struct drm_crtc *crtc = new_plane_state->crtc; > const struct drm_framebuffer *fb = new_plane_state->fb; > - unsigned int fb_width, fb_height; > - struct drm_mode_rect *clips; > - uint32_t num_clips; > int ret; > > /* either *both* CRTC and FB must be set, or neither */ > @@ -625,17 +692,6 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, > return -EINVAL; > } > > - /* Check whether this plane supports the fb pixel format. */ > - ret = drm_plane_check_pixel_format(plane, fb->format->format, > - fb->modifier); > - if (ret) { > - drm_dbg_atomic(plane->dev, > - "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n", > - plane->base.id, plane->name, > - &fb->format->format, fb->modifier); > - return ret; > - } > - > /* Give drivers some help against integer overflows */ > if (new_plane_state->crtc_w > INT_MAX || > new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w || > @@ -649,49 +705,11 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, > return -ERANGE; > } > > - fb_width = fb->width << 16; > - fb_height = fb->height << 16; > > - /* Make sure source coordinates are inside the fb. */ > - if (new_plane_state->src_w > fb_width || > - new_plane_state->src_x > fb_width - new_plane_state->src_w || > - new_plane_state->src_h > fb_height || > - new_plane_state->src_y > fb_height - new_plane_state->src_h) { > - drm_dbg_atomic(plane->dev, > - "[PLANE:%d:%s] invalid source coordinates " > - "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n", > - plane->base.id, plane->name, > - new_plane_state->src_w >> 16, > - ((new_plane_state->src_w & 0xffff) * 15625) >> 10, > - new_plane_state->src_h >> 16, > - ((new_plane_state->src_h & 0xffff) * 15625) >> 10, > - new_plane_state->src_x >> 16, > - ((new_plane_state->src_x & 0xffff) * 15625) >> 10, > - new_plane_state->src_y >> 16, > - ((new_plane_state->src_y & 0xffff) * 15625) >> 10, > - fb->width, fb->height); > - return -ENOSPC; > - } > - > - clips = __drm_plane_get_damage_clips(new_plane_state); > - num_clips = drm_plane_get_damage_clips_count(new_plane_state); > - > - /* Make sure damage clips are valid and inside the fb. */ > - while (num_clips > 0) { > - if (clips->x1 >= clips->x2 || > - clips->y1 >= clips->y2 || > - clips->x1 < 0 || > - clips->y1 < 0 || > - clips->x2 > fb_width || > - clips->y2 > fb_height) { > - drm_dbg_atomic(plane->dev, > - "[PLANE:%d:%s] invalid damage clip %d %d %d %d\n", > - plane->base.id, plane->name, clips->x1, > - clips->y1, clips->x2, clips->y2); > - return -EINVAL; > - } > - clips++; > - num_clips--; > + if (fb) { This doesn't only move code, but also changes semantics, making the checks optional if no FB is provided. Consider moving the condition to the next patch. Otherwise LGTM. > + ret = drm_atomic_check_fb(new_plane_state); > + if (ret) > + return ret; > } > > if (plane_switching_crtc(old_plane_state, new_plane_state)) { >
On 30/06/2023 03:25, Jessica Zhang wrote: > Add solid_fill and pixel_source properties to DPU plane > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 ++ > 1 file changed, 2 insertions(+) This should be the last commit. Otherwise: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index c2aaaded07ed..5f0984ce62b1 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -1429,6 +1429,8 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > DPU_ERROR("failed to install zpos property, rc = %d\n", ret); > > drm_plane_create_alpha_property(plane); > + drm_plane_create_solid_fill_property(plane); > + drm_plane_create_pixel_source_property(plane, BIT(DRM_PLANE_PIXEL_SOURCE_COLOR)); > drm_plane_create_blend_mode_property(plane, > BIT(DRM_MODE_BLEND_PIXEL_NONE) | > BIT(DRM_MODE_BLEND_PREMULTI) | >
On Fri, 30 Jun 2023 03:42:28 +0300 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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@quicinc.com> > > --- > > 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 > > >
On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com> > --- > 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) > +{ > + 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); > + 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", > + 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)) > + continue; > + > + ret = drm_property_add_enum(prop, enum_list[i].type, enum_list[i].name); > + > + 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 > +}; > + > /** > * 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 > > -- > 2.41.0 >
On 6/30/2023 7:43 AM, Sebastian Wick wrote: > On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com> >> --- >> 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. Hi Sebastian, Sounds good. So if pixel_source == NONE disables the planes, would that mean cases where pixel_source == COLOR && solid_fill_blob == NULL, the atomic commit should throw an error? Thanks, Jessica Zhang > >> + * >> + * 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); >> + 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", >> + 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)) >> + continue; >> + >> + ret = drm_property_add_enum(prop, enum_list[i].type, enum_list[i].name); >> + >> + 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 >> +}; >> + >> /** >> * 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 >> >> -- >> 2.41.0 >> >
On 6/29/2023 5:49 PM, Dmitry Baryshkov wrote: > On 30/06/2023 03:25, Jessica Zhang wrote: >> Add solid_fill and pixel_source properties to DPU plane >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 ++ >> 1 file changed, 2 insertions(+) > > This should be the last commit. Hi Dmitry, Acked, will move this to the end. Thanks, Jessica Zhang > Otherwise: > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> index c2aaaded07ed..5f0984ce62b1 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> @@ -1429,6 +1429,8 @@ struct drm_plane *dpu_plane_init(struct >> drm_device *dev, >> DPU_ERROR("failed to install zpos property, rc = %d\n", ret); >> drm_plane_create_alpha_property(plane); >> + drm_plane_create_solid_fill_property(plane); >> + drm_plane_create_pixel_source_property(plane, >> BIT(DRM_PLANE_PIXEL_SOURCE_COLOR)); >> drm_plane_create_blend_mode_property(plane, >> BIT(DRM_MODE_BLEND_PIXEL_NONE) | >> BIT(DRM_MODE_BLEND_PREMULTI) | >> > > -- > With best wishes > Dmitry >
On Fri, Jun 30, 2023 at 11:27 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > > On 6/30/2023 7:43 AM, Sebastian Wick wrote: > > On Fri, Jun 30, 2023 at 2:26 AM Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com> > >> --- > >> 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. > > Hi Sebastian, > > Sounds good. > > So if pixel_source == NONE disables the planes, would that mean cases > where pixel_source == COLOR && solid_fill_blob == NULL, the atomic > commit should throw an error? I would say so, yes. > Thanks, > > Jessica Zhang > > > > >> + * > >> + * 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); > >> + 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", > >> + 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)) > >> + continue; > >> + > >> + ret = drm_property_add_enum(prop, enum_list[i].type, enum_list[i].name); > >> + > >> + 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 > >> +}; > >> + > >> /** > >> * 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 > >> > >> -- > >> 2.41.0 > >> > > >
On 6/30/2023 1:27 AM, Pekka Paalanen wrote: > On Fri, 30 Jun 2023 03:42:28 +0300 > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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 <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/drm_atomic_state_helper.c | 1 + >>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ >>> drivers/gpu/drm/drm_blend.c | 81 +++++++++++++++++++++++++++++++ >>> include/drm/drm_blend.h | 2 + >>> include/drm/drm_plane.h | 21 ++++++++ >>> 5 files changed, 109 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c >>> index fe14be2bd2b2..86fb876efbe6 100644 >>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c >>> @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, >>> >>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; >>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; >>> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB; >>> >>> if (plane_state->solid_fill_blob) { >>> drm_property_blob_put(plane_state->solid_fill_blob); >>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >>> index a28b4ee79444..6e59c21af66b 100644 >>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>> drm_property_blob_put(solid_fill); >>> >>> return ret; >>> + } else if (property == plane->pixel_source_property) { >>> + state->pixel_source = val; >>> } else if (property == plane->alpha_property) { >>> state->alpha = val; >>> } else if (property == plane->blend_mode_property) { >> >> I think, it was pointed out in the discussion that drm_mode_setplane() >> (a pre-atomic IOCTL to turn the plane on and off) should also reset >> pixel_source to FB. I don't remember drm_mode_setplane() being mentioned in the pixel_source discussion... can you share where it was mentioned? I'd prefer to avoid having driver change the pixel_source directly as it could cause some unexpected side effects. In general, I would like userspace to assign the value of pixel_source without driver doing anything "under the hood". >> >>> @@ -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? Yep, the plane will only display the data from the set pixel_source. So if pixel_source == FB and solid_fill_blob is non-NULL, solid_fill_blob will be ignored and the plane will display the FB that is set. Will add a note about this in the comment docs. > >>> + * >>> + * Possible values: > > Wouldn't hurt to explicitly mention here that this is an enum. Acked. > >>> + * >>> + * "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". Acked. > > Why "COLOR" and not, say, "SOLID_FILL"? Ah, that would make more sense :) I'll change this to "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. Got it... If we allow the possibility of FB sources not being supported, then should the default pixel_source per plane be decided by the driver too? I'd forced FB support so that I could set pixel_source to FB in __drm_atomic_helper_plane_state_reset(). If we allow more flexibility in the default pixel_source value, I guess we can also store a default_pixel_source value in the plane_state. > >>> + * >>> + * 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? Acked. >> >>> + 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? Acked. > > >>> + 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? Acked. >> >>> + 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. Acked. >> >>> + 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. Got it. Thanks, Jessica Zhang > > > 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 >>> >> >
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 <dmitry.baryshkov@linaro.org> 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 <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic_state_helper.c | 1 + >>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ >>>> drivers/gpu/drm/drm_blend.c | 81 >>>> +++++++++++++++++++++++++++++++ >>>> include/drm/drm_blend.h | 2 + >>>> include/drm/drm_plane.h | 21 ++++++++ >>>> 5 files changed, 109 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c >>>> b/drivers/gpu/drm/drm_atomic_state_helper.c >>>> index fe14be2bd2b2..86fb876efbe6 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c >>>> @@ -252,6 +252,7 @@ void >>>> __drm_atomic_helper_plane_state_reset(struct drm_plane_state >>>> *plane_state, >>>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; >>>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; >>>> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB; >>>> if (plane_state->solid_fill_blob) { >>>> drm_property_blob_put(plane_state->solid_fill_blob); >>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >>>> b/drivers/gpu/drm/drm_atomic_uapi.c >>>> index a28b4ee79444..6e59c21af66b 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct >>>> drm_plane *plane, >>>> drm_property_blob_put(solid_fill); >>>> return ret; >>>> + } else if (property == plane->pixel_source_property) { >>>> + state->pixel_source = val; >>>> } else if (property == plane->alpha_property) { >>>> state->alpha = val; >>>> } else if (property == plane->blend_mode_property) { >>> >>> I think, it was pointed out in the discussion that drm_mode_setplane() >>> (a pre-atomic IOCTL to turn the plane on and off) should also reset >>> pixel_source to FB. > > I don't remember drm_mode_setplane() being mentioned in the pixel_source > discussion... can you share where it was mentioned? https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/ Let me quote it here: "Legacy non-atomic UAPI wrappers can do whatever they want, and program any (new) properties they want in order to implement the legacy expectations, so that does not seem to be a problem." > > I'd prefer to avoid having driver change the pixel_source directly as it > could cause some unexpected side effects. In general, I would like > userspace to assign the value of pixel_source without driver doing > anything "under the hood". s/driver/drm core/ We have to remain compatible with old userspace, especially with the non-atomic one. If the userspace calls ioctl(DRM_IOCTL_MODE_SETPLANE), we have to display the specified FB, no matter what was the value of PIXEL_SOURCE before this ioctl. > >>> >>>> @@ -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? > > Yep, the plane will only display the data from the set pixel_source. > > So if pixel_source == FB and solid_fill_blob is non-NULL, > solid_fill_blob will be ignored and the plane will display the FB that > is set. correct. > > Will add a note about this in the comment docs. > >> >>>> + * >>>> + * Possible values: >> >> Wouldn't hurt to explicitly mention here that this is an enum. > > Acked. > >> >>>> + * >>>> + * "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". > > Acked. > >> >> Why "COLOR" and not, say, "SOLID_FILL"? > > Ah, that would make more sense :) > > I'll change this to "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. > > Got it... If we allow the possibility of FB sources not being supported, > then should the default pixel_source per plane be decided by the driver > too? > > I'd forced FB support so that I could set pixel_source to FB in > __drm_atomic_helper_plane_state_reset(). If we allow more flexibility in > the default pixel_source value, I guess we can also store a > default_pixel_source value in the plane_state. I'd say, the FB is a sane default. It the driver has other needs, it can override the value in drm_plane_funcs::reset(). > [skipped the rest]
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 <dmitry.baryshkov@linaro.org> 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 <quic_jesszhan@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/drm_atomic_state_helper.c | 1 + >>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ >>>>>> drivers/gpu/drm/drm_blend.c | 81 >>>>>> +++++++++++++++++++++++++++++++ >>>>>> include/drm/drm_blend.h | 2 + >>>>>> include/drm/drm_plane.h | 21 ++++++++ >>>>>> 5 files changed, 109 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c >>>>>> b/drivers/gpu/drm/drm_atomic_state_helper.c >>>>>> index fe14be2bd2b2..86fb876efbe6 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c >>>>>> @@ -252,6 +252,7 @@ void >>>>>> __drm_atomic_helper_plane_state_reset(struct drm_plane_state >>>>>> *plane_state, >>>>>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; >>>>>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; >>>>>> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB; >>>>>> if (plane_state->solid_fill_blob) { >>>>>> drm_property_blob_put(plane_state->solid_fill_blob); >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> b/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> index a28b4ee79444..6e59c21af66b 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> @@ -596,6 +596,8 @@ static int >>>>>> drm_atomic_plane_set_property(struct drm_plane *plane, >>>>>> drm_property_blob_put(solid_fill); >>>>>> return ret; >>>>>> + } else if (property == plane->pixel_source_property) { >>>>>> + state->pixel_source = val; >>>>>> } else if (property == plane->alpha_property) { >>>>>> state->alpha = val; >>>>>> } else if (property == plane->blend_mode_property) { >>>>> >>>>> I think, it was pointed out in the discussion that drm_mode_setplane() >>>>> (a pre-atomic IOCTL to turn the plane on and off) should also reset >>>>> pixel_source to FB. >>> >>> I don't remember drm_mode_setplane() being mentioned in the >>> pixel_source discussion... can you share where it was mentioned? >> >> https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/ >> >> Let me quote it here: >> "Legacy non-atomic UAPI wrappers can do whatever they want, and program >> any (new) properties they want in order to implement the legacy >> expectations, so that does not seem to be a problem." >> >> >>> >>> I'd prefer to avoid having driver change the pixel_source directly as >>> it could cause some unexpected side effects. In general, I would like >>> userspace to assign the value of pixel_source without driver doing >>> anything "under the hood". >> >> s/driver/drm core/ >> >> We have to remain compatible with old userspace, especially with the >> non-atomic one. If the userspace calls ioctl(DRM_IOCTL_MODE_SETPLANE), >> we have to display the specified FB, no matter what was the value of >> PIXEL_SOURCE before this ioctl. > > > Got it, thanks the clarification -- I see your point. > > I'm already setting plane_state->pixel_source to FB in > __drm_atomic_helper_plane_reset() and it seems to me that all drivers > are calling that within their respective plane_funcs->reset(). > > Since (as far as I know) reset() is being called for both the atomic and > non-atomic paths, shouldn't that be enough to default pixel_source to FB > for old userspace? No, this will not clean up the state between userspace apps. Currently the rule is simple: call DRM_IOCTL_MODE_SETPLANE, get the image from FB displayed. We should keep it so. >>> >>>>> >>>>>> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane >>>>>> *plane, >>>>>> } else if (property == plane->solid_fill_property) { >>>>>> *val =state->solid_fill_blob ? >>>>>> state->solid_fill_blob->base.id : 0; >>>>>> + } else if (property == plane->pixel_source_property) { >>>>>> + *val = state->pixel_source; >>>>>> } else if (property == plane->alpha_property) { >>>>>> *val =state->alpha; >>>>>> } else if (property == plane->blend_mode_property) { >>>>>> diff --git a/drivers/gpu/drm/drm_blend.c >>>>>> b/drivers/gpu/drm/drm_blend.c >>>>>> index 38c3c5d6453a..8c100a957ee2 100644 >>>>>> --- a/drivers/gpu/drm/drm_blend.c >>>>>> +++ b/drivers/gpu/drm/drm_blend.c >>>>>> @@ -189,6 +189,18 @@ >>>>>> * solid_fill is set up with >>>>>> drm_plane_create_solid_fill_property(). 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? >>> >>> Yep, the plane will only display the data from the set pixel_source. >>> >>> So if pixel_source == FB and solid_fill_blob is non-NULL, >>> solid_fill_blob will be ignored and the plane will display the FB >>> that is set. >> >> correct. >> >>> >>> Will add a note about this in the comment docs. >>> >>>> >>>>>> + * >>>>>> + * Possible values: >>>> >>>> Wouldn't hurt to explicitly mention here that this is an enum. >>> >>> Acked. >>> >>>> >>>>>> + * >>>>>> + * "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". >>> >>> Acked. >>> >>>> >>>> Why "COLOR" and not, say, "SOLID_FILL"? >>> >>> Ah, that would make more sense :) >>> >>> I'll change this to "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. >>> >>> Got it... If we allow the possibility of FB sources not being >>> supported, then should the default pixel_source per plane be decided >>> by the driver too? >>> >>> I'd forced FB support so that I could set pixel_source to FB in >>> __drm_atomic_helper_plane_state_reset(). If we allow more flexibility >>> in the default pixel_source value, I guess we can also store a >>> default_pixel_source value in the plane_state. >> >> I'd say, the FB is a sane default. It the driver has other needs, it >> can override the value in drm_plane_funcs::reset(). >> >>> >> >> [skipped the rest] >> >> -- >> With best wishes >> Dmitry >>
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. --- 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_jesszhan@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 - Created drm_plane_has_visible_data() helper and corrected CRTC and FB NULL-check logic (Dmitry) - Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted them into helper method (Dmitry) - Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry) - Fixed indentation (Dmitry) Changes in v2: - Dropped SOLID_FILL_FORMAT property (Simon) - Switched to implementing solid_fill property as a blob (Simon, Dmitry) - Added drm_solid_fill and drm_solid_fill_info structs (Simon) - 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) - Changed to checks for if solid_fill_blob is set (Dmitry) - Abstracted (plane_state && !solid_fill_blob) checks to helper method (Dmitry) - Fixed dropped 'const' warning - Added helper to convert color fill to BGR888 (Rob) - Fixed indentation issue (Dmitry) - Added support for solid fill on planes of varying sizes --- Jessica Zhang (7): drm: Introduce solid fill DRM plane property drm: Introduce pixel_source DRM plane property drm/atomic: Move framebuffer checks to helper drm/atomic: Loosen FB atomic checks drm/msm/dpu: Add solid fill and pixel source properties drm/msm/dpu: Allow NULL FBs in atomic commit drm/msm/dpu: Use DRM solid_fill property drivers/gpu/drm/drm_atomic.c | 142 +++++++++++++++++------------- drivers/gpu/drm/drm_atomic_helper.c | 34 ++++--- drivers/gpu/drm/drm_atomic_state_helper.c | 10 +++ drivers/gpu/drm/drm_atomic_uapi.c | 59 +++++++++++++ drivers/gpu/drm/drm_blend.c | 114 ++++++++++++++++++++++++ drivers/gpu/drm/drm_plane.c | 8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 68 +++++++++----- include/drm/drm_atomic_helper.h | 4 +- include/drm/drm_blend.h | 3 + include/drm/drm_plane.h | 92 +++++++++++++++++++ 11 files changed, 437 insertions(+), 106 deletions(-) --- base-commit: a0364260213c96f6817f7e85cdce293cb743460f change-id: 20230404-solid-fill-05016175db36 Best regards,