| Message ID | 20230404-solid-fill-v4-1-f4ec0caa742d@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for Solid Fill Planes | expand |
On Thu, 29 Jun 2023 17:25:00 -0700 Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > Document and add support for solid_fill property to drm_plane. In > addition, add support for setting and getting the values for solid_fill. > > To enable solid fill planes, userspace must assign a property blob to > the "solid_fill" plane property containing the following information: > > struct drm_solid_fill_info { > u8 version; > u32 r, g, b; > }; > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> Hi Jessica, I've left some general UAPI related comments here. > --- > drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ > drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ > include/drm/drm_blend.h | 1 + > include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ > 5 files changed, 141 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > index 784e63d70a42..fe14be2bd2b2 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, > plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; > plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > > + if (plane_state->solid_fill_blob) { > + drm_property_blob_put(plane_state->solid_fill_blob); > + plane_state->solid_fill_blob = NULL; > + } > + > if (plane->color_encoding_property) { > if (!drm_object_property_get_default_value(&plane->base, > plane->color_encoding_property, > @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > if (state->fb) > drm_framebuffer_get(state->fb); > > + if (state->solid_fill_blob) > + drm_property_blob_get(state->solid_fill_blob); > + > state->fence = NULL; > state->commit = NULL; > state->fb_damage_clips = NULL; > @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > drm_crtc_commit_put(state->commit); > > drm_property_blob_put(state->fb_damage_clips); > + drm_property_blob_put(state->solid_fill_blob); > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index d867e7f9f2cd..a28b4ee79444 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, > } > EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); > > +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state, > + struct drm_property_blob *blob) > +{ > + int ret = 0; > + int blob_version; > + > + if (blob == state->solid_fill_blob) > + return 0; > + > + drm_property_blob_put(state->solid_fill_blob); > + state->solid_fill_blob = NULL; Is it ok to destroy old state before it is guaranteed that the new state is accepted? > + > + memset(&state->solid_fill, 0, sizeof(state->solid_fill)); > + > + if (blob) { > + struct drm_solid_fill_info *user_info = (struct drm_solid_fill_info *)blob->data; > + > + if (blob->length != sizeof(struct drm_solid_fill_info)) { > + drm_dbg_atomic(state->plane->dev, > + "[PLANE:%d:%s] bad solid fill blob length: %zu\n", > + state->plane->base.id, state->plane->name, > + blob->length); > + return -EINVAL; > + } > + > + blob_version = user_info->version; > + > + /* Add more versions if necessary */ > + if (blob_version == 1) { > + state->solid_fill.r = user_info->r; > + state->solid_fill.g = user_info->g; > + state->solid_fill.b = user_info->b; > + } else { > + drm_dbg_atomic(state->plane->dev, > + "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n", > + state->plane->base.id, state->plane->name, > + ret); > + return -EINVAL; Btw. how does the atomic check machinery work here? I expect that a TEST_ONLY atomic commit will do all the above checks and return failure if anything is not right. Right? > + } > + state->solid_fill_blob = drm_property_blob_get(blob); > + } > + > + return ret; > +} > + > static void set_out_fence_for_crtc(struct drm_atomic_state *state, > struct drm_crtc *crtc, s32 __user *fence_ptr) > { > @@ -544,6 +589,13 @@ 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->solid_fill_property) { > + struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val); > + > + ret = drm_atomic_set_solid_fill_prop(state, solid_fill); > + drm_property_blob_put(solid_fill); > + > + return ret; > } else if (property == plane->alpha_property) { > state->alpha = val; > } else if (property == plane->blend_mode_property) { > @@ -616,6 +668,9 @@ 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->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 6e74de833466..38c3c5d6453a 100644 > --- a/drivers/gpu/drm/drm_blend.c > +++ b/drivers/gpu/drm/drm_blend.c > @@ -185,6 +185,10 @@ > * plane does not expose the "alpha" property, then this is > * assumed to be 1.0 > * > + * solid_fill: > + * solid_fill is set up with drm_plane_create_solid_fill_property(). It > + * contains pixel data that drivers can use to fill a plane. This is a nice start, but I feel it needs to explain much more about how userspace should go about making use of this. Yeah, the pixel_source property comes in the next patch, but I feel that it is harder to review if the doc is built over multiple patches. My personal approach would be to write the doc in full and referring to pixel_source property already, and explain in the commit message that the next patch will add pixel_source so people don't wonder about referring to a non-existing property. I mean just a reference to pixel_source, and leave the actual pixel_source doc for the patch adding the property like it already is. Dmitry's suggestion of swapping the patch order is good too. > + * > * 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 +619,32 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane, > return 0; > } > EXPORT_SYMBOL(drm_plane_create_blend_mode_property); > + > +/** > + * drm_plane_create_solid_fill_property - create a new solid_fill property > + * @plane: drm plane > + * > + * This creates a new property that holds pixel data for solid fill planes. This > + * property is exposed to userspace as a property blob called "solid_fill". > + * > + * For information on what the blob contains, see `drm_solid_fill_info`. I think you should be more explicit here. For example: the blob must contain exactly one struct drm_solid_fill_info. It's better to put this content spec with the UAPI doc rather than in this kerner-internal function doc that userspace programmers won't know to look at. > + * > + * Returns: > + * Zero on success, negative errno on failure. > + */ > +int drm_plane_create_solid_fill_property(struct drm_plane *plane) > +{ > + struct drm_property *prop; > + > + prop = drm_property_create(plane->dev, > + DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB, > + "solid_fill", 0); > + if (!prop) > + return -ENOMEM; > + > + drm_object_attach_property(&plane->base, prop, 0); > + plane->solid_fill_property = prop; > + > + return 0; > +} > +EXPORT_SYMBOL(drm_plane_create_solid_fill_property); > diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h > index 88bdfec3bd88..0338a860b9c8 100644 > --- a/include/drm/drm_blend.h > +++ b/include/drm/drm_blend.h > @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > struct drm_atomic_state *state); > int drm_plane_create_blend_mode_property(struct drm_plane *plane, > unsigned int supported_modes); > +int drm_plane_create_solid_fill_property(struct drm_plane *plane); > #endif > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 51291983ea44..f6ab313cb83e 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -40,6 +40,25 @@ enum drm_scaling_filter { > DRM_SCALING_FILTER_NEAREST_NEIGHBOR, > }; > > +/** > + * struct drm_solid_fill_info - User info for solid fill planes > + */ > +struct drm_solid_fill_info { > + __u8 version; > + __u32 r, g, b; > +}; Shouldn't UAPI structs be in UAPI headers? Shouldn't UAPI structs use explicit padding to not leave any gaps when it's unavoidable? And the kernel to check that the gaps are indeed zeroed? It also needs more UAPI doc, like a link to the property doc that uses this and an explanation of what the values mean. Thanks, pq > + > +/** > + * struct solid_fill_property - RGB values for solid fill plane > + * > + * Note: This is the V1 for this feature > + */ > +struct drm_solid_fill { > + uint32_t r; > + uint32_t g; > + uint32_t b; > +}; > + > /** > * struct drm_plane_state - mutable plane state > * > @@ -116,6 +135,23 @@ struct drm_plane_state { > /** @src_h: height of visible portion of plane (in 16.16) */ > uint32_t src_h, src_w; > > + /** > + * @solid_fill_blob: > + * > + * Blob containing relevant information for a solid fill plane > + * including pixel format and data. See > + * drm_plane_create_solid_fill_property() for more details. > + */ > + struct drm_property_blob *solid_fill_blob; > + > + /** > + * @solid_fill: > + * > + * Pixel data for solid fill planes. See > + * drm_plane_create_solid_fill_property() for more details. > + */ > + struct drm_solid_fill solid_fill; > + > /** > * @alpha: > * Opacity of the plane with 0 as completely transparent and 0xffff as > @@ -699,6 +735,13 @@ struct drm_plane { > */ > struct drm_plane_state *state; > > + /* > + * @solid_fill_property: > + * Optional solid_fill property for this plane. See > + * drm_plane_create_solid_fill_property(). > + */ > + struct drm_property *solid_fill_property; > + > /** > * @alpha_property: > * Optional alpha property for this plane. See >
On 30/06/2023 03:25, Jessica Zhang wrote: > Document and add support for solid_fill property to drm_plane. In > addition, add support for setting and getting the values for solid_fill. > > To enable solid fill planes, userspace must assign a property blob to > the "solid_fill" plane property containing the following information: > > struct drm_solid_fill_info { > u8 version; > u32 r, g, b; > }; > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ > drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ > include/drm/drm_blend.h | 1 + > include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ > 5 files changed, 141 insertions(+) Also, I think the point which we missed up to now. Could you please add both new properties to dri/N/state debugfs?
On 6/30/2023 3:33 AM, Dmitry Baryshkov wrote: > On 30/06/2023 03:25, Jessica Zhang wrote: >> Document and add support for solid_fill property to drm_plane. In >> addition, add support for setting and getting the values for solid_fill. >> >> To enable solid fill planes, userspace must assign a property blob to >> the "solid_fill" plane property containing the following information: >> >> struct drm_solid_fill_info { >> u8 version; >> u32 r, g, b; >> }; >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ >> drivers/gpu/drm/drm_atomic_uapi.c | 55 >> +++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ >> include/drm/drm_blend.h | 1 + >> include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ >> 5 files changed, 141 insertions(+) > > Also, I think the point which we missed up to now. Could you please add > both new properties to dri/N/state debugfs? Hi Dmitry, Good catch -- acked. Thanks, Jessica Zhang > > -- > With best wishes > Dmitry >
On 6/30/2023 1:27 AM, Pekka Paalanen wrote: > On Thu, 29 Jun 2023 17:25:00 -0700 > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> Document and add support for solid_fill property to drm_plane. In >> addition, add support for setting and getting the values for solid_fill. >> >> To enable solid fill planes, userspace must assign a property blob to >> the "solid_fill" plane property containing the following information: >> >> struct drm_solid_fill_info { >> u8 version; >> u32 r, g, b; >> }; >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > > Hi Jessica, > > I've left some general UAPI related comments here. > >> --- >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ >> drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ >> include/drm/drm_blend.h | 1 + >> include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ >> 5 files changed, 141 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c >> index 784e63d70a42..fe14be2bd2b2 100644 >> --- a/drivers/gpu/drm/drm_atomic_state_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c >> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, >> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; >> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; >> >> + if (plane_state->solid_fill_blob) { >> + drm_property_blob_put(plane_state->solid_fill_blob); >> + plane_state->solid_fill_blob = NULL; >> + } >> + >> if (plane->color_encoding_property) { >> if (!drm_object_property_get_default_value(&plane->base, >> plane->color_encoding_property, >> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >> if (state->fb) >> drm_framebuffer_get(state->fb); >> >> + if (state->solid_fill_blob) >> + drm_property_blob_get(state->solid_fill_blob); >> + >> state->fence = NULL; >> state->commit = NULL; >> state->fb_damage_clips = NULL; >> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >> drm_crtc_commit_put(state->commit); >> >> drm_property_blob_put(state->fb_damage_clips); >> + drm_property_blob_put(state->solid_fill_blob); >> } >> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); >> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >> index d867e7f9f2cd..a28b4ee79444 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, >> } >> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); >> >> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state, >> + struct drm_property_blob *blob) >> +{ >> + int ret = 0; >> + int blob_version; >> + >> + if (blob == state->solid_fill_blob) >> + return 0; >> + >> + drm_property_blob_put(state->solid_fill_blob); >> + state->solid_fill_blob = NULL; > > Is it ok to destroy old state before it is guaranteed that the new > state is accepted? Hi Pekka, Good point. I'll change this behavior so that an error case will keep the old solid_fill_blob value. > >> + >> + memset(&state->solid_fill, 0, sizeof(state->solid_fill)); >> + >> + if (blob) { >> + struct drm_solid_fill_info *user_info = (struct drm_solid_fill_info *)blob->data; >> + >> + if (blob->length != sizeof(struct drm_solid_fill_info)) { >> + drm_dbg_atomic(state->plane->dev, >> + "[PLANE:%d:%s] bad solid fill blob length: %zu\n", >> + state->plane->base.id, state->plane->name, >> + blob->length); >> + return -EINVAL; >> + } >> + >> + blob_version = user_info->version; >> + >> + /* Add more versions if necessary */ >> + if (blob_version == 1) { >> + state->solid_fill.r = user_info->r; >> + state->solid_fill.g = user_info->g; >> + state->solid_fill.b = user_info->b; >> + } else { >> + drm_dbg_atomic(state->plane->dev, >> + "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n", >> + state->plane->base.id, state->plane->name, >> + ret); >> + return -EINVAL; > > Btw. how does the atomic check machinery work here? > > I expect that a TEST_ONLY atomic commit will do all the above checks > and return failure if anything is not right. Right? Correct, drm_atomic_set_property() will still be called for a TEST_ONLY commit, so these checks will still happen and return an error (or set the property) when appropriate. > >> + } >> + state->solid_fill_blob = drm_property_blob_get(blob); >> + } >> + >> + return ret; >> +} >> + >> static void set_out_fence_for_crtc(struct drm_atomic_state *state, >> struct drm_crtc *crtc, s32 __user *fence_ptr) >> { >> @@ -544,6 +589,13 @@ 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->solid_fill_property) { >> + struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val); >> + >> + ret = drm_atomic_set_solid_fill_prop(state, solid_fill); >> + drm_property_blob_put(solid_fill); >> + >> + return ret; >> } else if (property == plane->alpha_property) { >> state->alpha = val; >> } else if (property == plane->blend_mode_property) { >> @@ -616,6 +668,9 @@ 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->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 6e74de833466..38c3c5d6453a 100644 >> --- a/drivers/gpu/drm/drm_blend.c >> +++ b/drivers/gpu/drm/drm_blend.c >> @@ -185,6 +185,10 @@ >> * plane does not expose the "alpha" property, then this is >> * assumed to be 1.0 >> * >> + * solid_fill: >> + * solid_fill is set up with drm_plane_create_solid_fill_property(). It >> + * contains pixel data that drivers can use to fill a plane. > > This is a nice start, but I feel it needs to explain much more about > how userspace should go about making use of this. > > Yeah, the pixel_source property comes in the next patch, but I feel > that it is harder to review if the doc is built over multiple patches. > My personal approach would be to write the doc in full and referring to > pixel_source property already, and explain in the commit message that > the next patch will add pixel_source so people don't wonder about > referring to a non-existing property. > > I mean just a reference to pixel_source, and leave the actual > pixel_source doc for the patch adding the property like it already is. > > Dmitry's suggestion of swapping the patch order is good too. Makes sense. I'll switch the patch order, but will keep this in mind. > >> + * >> * 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 +619,32 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane, >> return 0; >> } >> EXPORT_SYMBOL(drm_plane_create_blend_mode_property); >> + >> +/** >> + * drm_plane_create_solid_fill_property - create a new solid_fill property >> + * @plane: drm plane >> + * >> + * This creates a new property that holds pixel data for solid fill planes. This >> + * property is exposed to userspace as a property blob called "solid_fill". >> + * >> + * For information on what the blob contains, see `drm_solid_fill_info`. > > I think you should be more explicit here. For example: the blob must > contain exactly one struct drm_solid_fill_info. > > It's better to put this content spec with the UAPI doc rather than in this > kerner-internal function doc that userspace programmers won't know to > look at. Acked. > >> + * >> + * Returns: >> + * Zero on success, negative errno on failure. >> + */ >> +int drm_plane_create_solid_fill_property(struct drm_plane *plane) >> +{ >> + struct drm_property *prop; >> + >> + prop = drm_property_create(plane->dev, >> + DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB, >> + "solid_fill", 0); >> + if (!prop) >> + return -ENOMEM; >> + >> + drm_object_attach_property(&plane->base, prop, 0); >> + plane->solid_fill_property = prop; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_plane_create_solid_fill_property); >> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h >> index 88bdfec3bd88..0338a860b9c8 100644 >> --- a/include/drm/drm_blend.h >> +++ b/include/drm/drm_blend.h >> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, >> struct drm_atomic_state *state); >> int drm_plane_create_blend_mode_property(struct drm_plane *plane, >> unsigned int supported_modes); >> +int drm_plane_create_solid_fill_property(struct drm_plane *plane); >> #endif >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index 51291983ea44..f6ab313cb83e 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -40,6 +40,25 @@ enum drm_scaling_filter { >> DRM_SCALING_FILTER_NEAREST_NEIGHBOR, >> }; >> >> +/** >> + * struct drm_solid_fill_info - User info for solid fill planes >> + */ >> +struct drm_solid_fill_info { >> + __u8 version; >> + __u32 r, g, b; >> +}; > > Shouldn't UAPI structs be in UAPI headers? Acked, will move this to uapi/drm_mode.h > > Shouldn't UAPI structs use explicit padding to not leave any gaps when > it's unavoidable? And the kernel to check that the gaps are indeed > zeroed? I don't believe so... From my understanding, padding will be taken care of by the compiler by default. Looking at the drm_mode_modeinfo UAPI struct [1], it also doesn't seem to do explicit padding. And the corresponding set_property() code doesn't seem check the gaps either. That being said, it's possible that I'm missing something here, so please let me know if that's the case. [1] https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242 > > It also needs more UAPI doc, like a link to the property doc that uses > this and an explanation of what the values mean. Acked. Thanks, Jessica Zhang > > > Thanks, > pq > >> + >> +/** >> + * struct solid_fill_property - RGB values for solid fill plane >> + * >> + * Note: This is the V1 for this feature >> + */ >> +struct drm_solid_fill { >> + uint32_t r; >> + uint32_t g; >> + uint32_t b; >> +}; >> + >> /** >> * struct drm_plane_state - mutable plane state >> * >> @@ -116,6 +135,23 @@ struct drm_plane_state { >> /** @src_h: height of visible portion of plane (in 16.16) */ >> uint32_t src_h, src_w; >> >> + /** >> + * @solid_fill_blob: >> + * >> + * Blob containing relevant information for a solid fill plane >> + * including pixel format and data. See >> + * drm_plane_create_solid_fill_property() for more details. >> + */ >> + struct drm_property_blob *solid_fill_blob; >> + >> + /** >> + * @solid_fill: >> + * >> + * Pixel data for solid fill planes. See >> + * drm_plane_create_solid_fill_property() for more details. >> + */ >> + struct drm_solid_fill solid_fill; >> + >> /** >> * @alpha: >> * Opacity of the plane with 0 as completely transparent and 0xffff as >> @@ -699,6 +735,13 @@ struct drm_plane { >> */ >> struct drm_plane_state *state; >> >> + /* >> + * @solid_fill_property: >> + * Optional solid_fill property for this plane. See >> + * drm_plane_create_solid_fill_property(). >> + */ >> + struct drm_property *solid_fill_property; >> + >> /** >> * @alpha_property: >> * Optional alpha property for this plane. See >> >
On Mon, 10 Jul 2023 16:12:06 -0700 Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > On 6/30/2023 1:27 AM, Pekka Paalanen wrote: > > On Thu, 29 Jun 2023 17:25:00 -0700 > > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > >> Document and add support for solid_fill property to drm_plane. In > >> addition, add support for setting and getting the values for solid_fill. > >> > >> To enable solid fill planes, userspace must assign a property blob to > >> the "solid_fill" plane property containing the following information: > >> > >> struct drm_solid_fill_info { > >> u8 version; > >> u32 r, g, b; > >> }; > >> > >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > > > > Hi Jessica, > > > > I've left some general UAPI related comments here. > > > >> --- > >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ > >> drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ > >> include/drm/drm_blend.h | 1 + > >> include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ > >> 5 files changed, 141 insertions(+) ... > >> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h > >> index 88bdfec3bd88..0338a860b9c8 100644 > >> --- a/include/drm/drm_blend.h > >> +++ b/include/drm/drm_blend.h > >> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > >> struct drm_atomic_state *state); > >> int drm_plane_create_blend_mode_property(struct drm_plane *plane, > >> unsigned int supported_modes); > >> +int drm_plane_create_solid_fill_property(struct drm_plane *plane); > >> #endif > >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > >> index 51291983ea44..f6ab313cb83e 100644 > >> --- a/include/drm/drm_plane.h > >> +++ b/include/drm/drm_plane.h > >> @@ -40,6 +40,25 @@ enum drm_scaling_filter { > >> DRM_SCALING_FILTER_NEAREST_NEIGHBOR, > >> }; > >> > >> +/** > >> + * struct drm_solid_fill_info - User info for solid fill planes > >> + */ > >> +struct drm_solid_fill_info { > >> + __u8 version; > >> + __u32 r, g, b; > >> +}; > > > > Shouldn't UAPI structs be in UAPI headers? > > Acked, will move this to uapi/drm_mode.h > > > > > Shouldn't UAPI structs use explicit padding to not leave any gaps when > > it's unavoidable? And the kernel to check that the gaps are indeed > > zeroed? > > I don't believe so... From my understanding, padding will be taken care > of by the compiler by default. Looking at the drm_mode_modeinfo UAPI > struct [1], it also doesn't seem to do explicit padding. And the > corresponding set_property() code doesn't seem check the gaps either. > > That being said, it's possible that I'm missing something here, so > please let me know if that's the case. > > [1] > https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242 I suspect that drm_mode_modeinfo predates the lessons learnt about "botching up ioctls" by many years: https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst drm_mode_modeinfo goes all the way back to commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef Date: Fri Nov 7 14:05:41 2008 -0800 DRM: add mode setting support and commit e0c8463a8b00b467611607df0ff369d062528875 Date: Fri Dec 19 14:50:50 2008 +1000 drm: sanitise drm modesetting API + remove unused hotplug and it got the proper types later in commit 1d7f83d5ad6c30b385ba549c1c3a287cc872b7ae Date: Thu Feb 26 00:51:42 2009 +0100 make drm headers use strict integer types My personal feeling is that if you cannot avoid padding in a struct, convert it into explicit fields anyway and require them to be zero. That way if you ever need to extend or modify the struct, you already have an "unused" field that old userspace guarantees to be zero, so you can re-purpose it when it's not zero. A struct for blob contents is maybe needing slightly less forward planning than ioctl struct, because KMS properties are cheap compared to ioctl numbers, I believe. Maybe eliminating compiler induced padding in structs is not strictly necessary, but it seems like a good idea to me, because compilers are allowed to leave the padding bits undefined. If a struct was filled in by the kernel and delivered to userspace, undefined padding could even be a security leak, theoretically. Anyway, don't take my word for it. Maybe kernel devs have a different style. Thanks, pq > > > > It also needs more UAPI doc, like a link to the property doc that uses > > this and an explanation of what the values mean. > > Acked. > > Thanks, > > Jessica Zhang > > > > > > > Thanks, > > pq > > > >> + > >> +/** > >> + * struct solid_fill_property - RGB values for solid fill plane > >> + * > >> + * Note: This is the V1 for this feature > >> + */ > >> +struct drm_solid_fill { > >> + uint32_t r; > >> + uint32_t g; > >> + uint32_t b; > >> +}; > >> + > >> /** > >> * struct drm_plane_state - mutable plane state > >> * > >> @@ -116,6 +135,23 @@ struct drm_plane_state { > >> /** @src_h: height of visible portion of plane (in 16.16) */ > >> uint32_t src_h, src_w; > >> > >> + /** > >> + * @solid_fill_blob: > >> + * > >> + * Blob containing relevant information for a solid fill plane > >> + * including pixel format and data. See > >> + * drm_plane_create_solid_fill_property() for more details. > >> + */ > >> + struct drm_property_blob *solid_fill_blob; > >> + > >> + /** > >> + * @solid_fill: > >> + * > >> + * Pixel data for solid fill planes. See > >> + * drm_plane_create_solid_fill_property() for more details. > >> + */ > >> + struct drm_solid_fill solid_fill; > >> + > >> /** > >> * @alpha: > >> * Opacity of the plane with 0 as completely transparent and 0xffff as > >> @@ -699,6 +735,13 @@ struct drm_plane { > >> */ > >> struct drm_plane_state *state; > >> > >> + /* > >> + * @solid_fill_property: > >> + * Optional solid_fill property for this plane. See > >> + * drm_plane_create_solid_fill_property(). > >> + */ > >> + struct drm_property *solid_fill_property; > >> + > >> /** > >> * @alpha_property: > >> * Optional alpha property for this plane. See > >> > >
On 7/11/2023 12:42 AM, Pekka Paalanen wrote: > On Mon, 10 Jul 2023 16:12:06 -0700 > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> On 6/30/2023 1:27 AM, Pekka Paalanen wrote: >>> On Thu, 29 Jun 2023 17:25:00 -0700 >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >>> >>>> Document and add support for solid_fill property to drm_plane. In >>>> addition, add support for setting and getting the values for solid_fill. >>>> >>>> To enable solid fill planes, userspace must assign a property blob to >>>> the "solid_fill" plane property containing the following information: >>>> >>>> struct drm_solid_fill_info { >>>> u8 version; >>>> u32 r, g, b; >>>> }; >>>> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> >>> Hi Jessica, >>> >>> I've left some general UAPI related comments here. >>> >>>> --- >>>> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ >>>> drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ >>>> include/drm/drm_blend.h | 1 + >>>> include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ >>>> 5 files changed, 141 insertions(+) > > ... > >>>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h >>>> index 88bdfec3bd88..0338a860b9c8 100644 >>>> --- a/include/drm/drm_blend.h >>>> +++ b/include/drm/drm_blend.h >>>> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, >>>> struct drm_atomic_state *state); >>>> int drm_plane_create_blend_mode_property(struct drm_plane *plane, >>>> unsigned int supported_modes); >>>> +int drm_plane_create_solid_fill_property(struct drm_plane *plane); >>>> #endif >>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>>> index 51291983ea44..f6ab313cb83e 100644 >>>> --- a/include/drm/drm_plane.h >>>> +++ b/include/drm/drm_plane.h >>>> @@ -40,6 +40,25 @@ enum drm_scaling_filter { >>>> DRM_SCALING_FILTER_NEAREST_NEIGHBOR, >>>> }; >>>> >>>> +/** >>>> + * struct drm_solid_fill_info - User info for solid fill planes >>>> + */ >>>> +struct drm_solid_fill_info { >>>> + __u8 version; >>>> + __u32 r, g, b; >>>> +}; >>> >>> Shouldn't UAPI structs be in UAPI headers? >> >> Acked, will move this to uapi/drm_mode.h >> >>> >>> Shouldn't UAPI structs use explicit padding to not leave any gaps when >>> it's unavoidable? And the kernel to check that the gaps are indeed >>> zeroed? >> >> I don't believe so... From my understanding, padding will be taken care >> of by the compiler by default. Looking at the drm_mode_modeinfo UAPI >> struct [1], it also doesn't seem to do explicit padding. And the >> corresponding set_property() code doesn't seem check the gaps either. >> >> That being said, it's possible that I'm missing something here, so >> please let me know if that's the case. >> >> [1] >> https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242 > > I suspect that drm_mode_modeinfo predates the lessons learnt about > "botching up ioctls" by many years: > https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst > > drm_mode_modeinfo goes all the way back to > > commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef > Date: Fri Nov 7 14:05:41 2008 -0800 > > DRM: add mode setting support > > and > > commit e0c8463a8b00b467611607df0ff369d062528875 > Date: Fri Dec 19 14:50:50 2008 +1000 > > drm: sanitise drm modesetting API + remove unused hotplug > > and it got the proper types later in > > commit 1d7f83d5ad6c30b385ba549c1c3a287cc872b7ae > Date: Thu Feb 26 00:51:42 2009 +0100 > > make drm headers use strict integer types > > > My personal feeling is that if you cannot avoid padding in a struct, > convert it into explicit fields anyway and require them to be zero. > That way if you ever need to extend or modify the struct, you already > have an "unused" field that old userspace guarantees to be zero, so you > can re-purpose it when it's not zero. > > A struct for blob contents is maybe needing slightly less forward > planning than ioctl struct, because KMS properties are cheap compared > to ioctl numbers, I believe. > > Maybe eliminating compiler induced padding in structs is not strictly > necessary, but it seems like a good idea to me, because compilers are > allowed to leave the padding bits undefined. If a struct was filled in > by the kernel and delivered to userspace, undefined padding could even > be a security leak, theoretically. > > Anyway, don't take my word for it. Maybe kernel devs have a different > style. Ah, got it. Thanks for the info! Looking over more recent implementations of blob properties, I do see that there's a precedent for explicit padding [1]. I think I could also just make `version` a __u32 instead. Either way, that seems to be how other structs declare `version`. Thanks, Jessica Zhang [1] https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/virtgpu_drm.h#L178 > > > Thanks, > pq > >>> >>> It also needs more UAPI doc, like a link to the property doc that uses >>> this and an explanation of what the values mean. >> >> Acked. >> >> Thanks, >> >> Jessica Zhang >> >>> >>> >>> Thanks, >>> pq >>> >>>> + >>>> +/** >>>> + * struct solid_fill_property - RGB values for solid fill plane >>>> + * >>>> + * Note: This is the V1 for this feature >>>> + */ >>>> +struct drm_solid_fill { >>>> + uint32_t r; >>>> + uint32_t g; >>>> + uint32_t b; >>>> +}; >>>> + >>>> /** >>>> * struct drm_plane_state - mutable plane state >>>> * >>>> @@ -116,6 +135,23 @@ struct drm_plane_state { >>>> /** @src_h: height of visible portion of plane (in 16.16) */ >>>> uint32_t src_h, src_w; >>>> >>>> + /** >>>> + * @solid_fill_blob: >>>> + * >>>> + * Blob containing relevant information for a solid fill plane >>>> + * including pixel format and data. See >>>> + * drm_plane_create_solid_fill_property() for more details. >>>> + */ >>>> + struct drm_property_blob *solid_fill_blob; >>>> + >>>> + /** >>>> + * @solid_fill: >>>> + * >>>> + * Pixel data for solid fill planes. See >>>> + * drm_plane_create_solid_fill_property() for more details. >>>> + */ >>>> + struct drm_solid_fill solid_fill; >>>> + >>>> /** >>>> * @alpha: >>>> * Opacity of the plane with 0 as completely transparent and 0xffff as >>>> @@ -699,6 +735,13 @@ struct drm_plane { >>>> */ >>>> struct drm_plane_state *state; >>>> >>>> + /* >>>> + * @solid_fill_property: >>>> + * Optional solid_fill property for this plane. See >>>> + * drm_plane_create_solid_fill_property(). >>>> + */ >>>> + struct drm_property *solid_fill_property; >>>> + >>>> /** >>>> * @alpha_property: >>>> * Optional alpha property for this plane. See >>>> >>> >
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 784e63d70a42..fe14be2bd2b2 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; + if (plane_state->solid_fill_blob) { + drm_property_blob_put(plane_state->solid_fill_blob); + plane_state->solid_fill_blob = NULL; + } + if (plane->color_encoding_property) { if (!drm_object_property_get_default_value(&plane->base, plane->color_encoding_property, @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, if (state->fb) drm_framebuffer_get(state->fb); + if (state->solid_fill_blob) + drm_property_blob_get(state->solid_fill_blob); + state->fence = NULL; state->commit = NULL; state->fb_damage_clips = NULL; @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) drm_crtc_commit_put(state->commit); drm_property_blob_put(state->fb_damage_clips); + drm_property_blob_put(state->solid_fill_blob); } EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index d867e7f9f2cd..a28b4ee79444 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, } EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state, + struct drm_property_blob *blob) +{ + int ret = 0; + int blob_version; + + if (blob == state->solid_fill_blob) + return 0; + + drm_property_blob_put(state->solid_fill_blob); + state->solid_fill_blob = NULL; + + memset(&state->solid_fill, 0, sizeof(state->solid_fill)); + + if (blob) { + struct drm_solid_fill_info *user_info = (struct drm_solid_fill_info *)blob->data; + + if (blob->length != sizeof(struct drm_solid_fill_info)) { + drm_dbg_atomic(state->plane->dev, + "[PLANE:%d:%s] bad solid fill blob length: %zu\n", + state->plane->base.id, state->plane->name, + blob->length); + return -EINVAL; + } + + blob_version = user_info->version; + + /* Add more versions if necessary */ + if (blob_version == 1) { + state->solid_fill.r = user_info->r; + state->solid_fill.g = user_info->g; + state->solid_fill.b = user_info->b; + } else { + drm_dbg_atomic(state->plane->dev, + "[PLANE:%d:%s] failed to set solid fill (ret=%d)\n", + state->plane->base.id, state->plane->name, + ret); + return -EINVAL; + } + state->solid_fill_blob = drm_property_blob_get(blob); + } + + return ret; +} + static void set_out_fence_for_crtc(struct drm_atomic_state *state, struct drm_crtc *crtc, s32 __user *fence_ptr) { @@ -544,6 +589,13 @@ 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->solid_fill_property) { + struct drm_property_blob *solid_fill = drm_property_lookup_blob(dev, val); + + ret = drm_atomic_set_solid_fill_prop(state, solid_fill); + drm_property_blob_put(solid_fill); + + return ret; } else if (property == plane->alpha_property) { state->alpha = val; } else if (property == plane->blend_mode_property) { @@ -616,6 +668,9 @@ 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->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 6e74de833466..38c3c5d6453a 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -185,6 +185,10 @@ * plane does not expose the "alpha" property, then this is * assumed to be 1.0 * + * solid_fill: + * solid_fill is set up with drm_plane_create_solid_fill_property(). It + * contains pixel data that drivers can use to fill a plane. + * * Note that all the property extensions described here apply either to the * plane or the CRTC (e.g. for the background color, which currently is not * exposed and assumed to be black). @@ -615,3 +619,32 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_blend_mode_property); + +/** + * drm_plane_create_solid_fill_property - create a new solid_fill property + * @plane: drm plane + * + * This creates a new property that holds pixel data for solid fill planes. This + * property is exposed to userspace as a property blob called "solid_fill". + * + * For information on what the blob contains, see `drm_solid_fill_info`. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_plane_create_solid_fill_property(struct drm_plane *plane) +{ + struct drm_property *prop; + + prop = drm_property_create(plane->dev, + DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB, + "solid_fill", 0); + if (!prop) + return -ENOMEM; + + drm_object_attach_property(&plane->base, prop, 0); + plane->solid_fill_property = prop; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_solid_fill_property); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 88bdfec3bd88..0338a860b9c8 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state); int drm_plane_create_blend_mode_property(struct drm_plane *plane, unsigned int supported_modes); +int drm_plane_create_solid_fill_property(struct drm_plane *plane); #endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 51291983ea44..f6ab313cb83e 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -40,6 +40,25 @@ enum drm_scaling_filter { DRM_SCALING_FILTER_NEAREST_NEIGHBOR, }; +/** + * struct drm_solid_fill_info - User info for solid fill planes + */ +struct drm_solid_fill_info { + __u8 version; + __u32 r, g, b; +}; + +/** + * struct solid_fill_property - RGB values for solid fill plane + * + * Note: This is the V1 for this feature + */ +struct drm_solid_fill { + uint32_t r; + uint32_t g; + uint32_t b; +}; + /** * struct drm_plane_state - mutable plane state * @@ -116,6 +135,23 @@ struct drm_plane_state { /** @src_h: height of visible portion of plane (in 16.16) */ uint32_t src_h, src_w; + /** + * @solid_fill_blob: + * + * Blob containing relevant information for a solid fill plane + * including pixel format and data. See + * drm_plane_create_solid_fill_property() for more details. + */ + struct drm_property_blob *solid_fill_blob; + + /** + * @solid_fill: + * + * Pixel data for solid fill planes. See + * drm_plane_create_solid_fill_property() for more details. + */ + struct drm_solid_fill solid_fill; + /** * @alpha: * Opacity of the plane with 0 as completely transparent and 0xffff as @@ -699,6 +735,13 @@ struct drm_plane { */ struct drm_plane_state *state; + /* + * @solid_fill_property: + * Optional solid_fill property for this plane. See + * drm_plane_create_solid_fill_property(). + */ + struct drm_property *solid_fill_property; + /** * @alpha_property: * Optional alpha property for this plane. See
Document and add support for solid_fill property to drm_plane. In addition, add support for setting and getting the values for solid_fill. To enable solid fill planes, userspace must assign a property blob to the "solid_fill" plane property containing the following information: struct drm_solid_fill_info { u8 version; u32 r, g, b; }; Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/drm_atomic_state_helper.c | 9 +++++ drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_blend.c | 33 +++++++++++++++++++ include/drm/drm_blend.h | 1 + include/drm/drm_plane.h | 43 ++++++++++++++++++++++++ 5 files changed, 141 insertions(+)