On Thu, 29 Jun 2023 17:25:00 -0700
Jessica Zhang wrote:
> Document and add support for solid_fill property to drm_plane. In
> addition, add support for setting and getting the values for solid_fill.
>
> To enable solid fill planes, userspace must assign a property blob to
> the "solid_fill" plane property containing the following information:
>
> struct drm_solid_fill_info {
> u8 version;
> u32 r, g, b;
> };
>
> Signed-off-by: Jessica Zhang
Hi Jessica,
I've left some general UAPI related comments here.
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> drivers/gpu/drm/drm_atomic_uapi.c | 55
> +++
> drivers/gpu/drm/drm_blend.c | 33 +++
> include/drm/drm_blend.h | 1 +
> include/drm/drm_plane.h | 43
> 5 files changed, 141 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..fe14be2bd2b2 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>
> + if (plane_state->solid_fill_blob) {
> + drm_property_blob_put(plane_state->solid_fill_blob);
> + plane_state->solid_fill_blob = NULL;
> + }
> +
> if (plane->color_encoding_property) {
> if (!drm_object_property_get_default_value(&plane->base,
>
> plane->color_encoding_property,
> @@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct
> drm_plane *plane,
> if (state->fb)
> drm_framebuffer_get(state->fb);
>
> + if (state->solid_fill_blob)
> + drm_property_blob_get(state->solid_fill_blob);
> +
> state->fence = NULL;
> state->commit = NULL;
> state->fb_damage_clips = NULL;
> @@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct
> drm_plane_state *state)
> drm_crtc_commit_put(state->commit);
>
> drm_property_blob_put(state->fb_damage_clips);
> + drm_property_blob_put(state->solid_fill_blob);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd..a28b4ee79444 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct
> drm_connector_state *conn_state,
> }
> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>
> +static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
> + struct drm_property_blob *blob)
> +{
> + int ret = 0;
> + int blob_version;
> +
> + if (blob == state->solid_fill_blob)
> + return 0;
> +
> + drm_property_blob_put(state->solid_fill_blob);
> + state->solid_fill_blob = NULL;
Is it ok to destroy old state before it is guaranteed that the new
state is accepted?
> +
> + memset(&state->solid_fill, 0, sizeof(state->solid_fill));
> +
> + if (blob) {
> + struct drm_solid_fill_info *user_info = (struct
> drm_solid_fill_info *)blob->data;
> +
> + if (blob->length != sizeof(struct drm_solid_fill_info)) {
> + drm_dbg_atomic(state->plane->dev,
> +"[PLANE:%d:%s] bad solid fill blob
> length: %zu\n",
> +state->plane->base.id,
> state->plane->name,
> +blob->length);
> + return -EINVAL;
> + }
> +
> + blob_version = user_info->version;
> +
> + /* Add more versions if necessary */
> + if (blob_version == 1) {
> + state->solid_fill.r = user_info->r;
> + state->solid_fill.g = user_info->g;
> + state->solid_fill.b = user_info->b;
> + } else {
> + drm_dbg_atomic(state->plane->dev,
> +"[PLANE:%d:%s] failed to set solid fill
> (ret=%d)\n",
> +state->plane->base.id,
> state->plane->name,
> +ret);
> + return -EINVAL;
Btw. how does the atomic check machinery work here?
I expect that a TEST_ONLY atomic commit will do all the above checks
and return failure if anything is not right. Right?
> + }
> + state->solid_fill_blob = drm_property_blob_get(blob);
> + }
> +
> + return ret;
> +}
> +
> static void set_out_fence_for_crtc(struct drm_atomic_state *state,
>
