Re: [RFC PATCH 0/3] Support for Solid Fill Planes
On Wed, 28 Jun 2023 09:40:21 -0700 Jessica Zhang wrote: > On 6/28/2023 12:34 AM, Pekka Paalanen wrote: > > On Tue, 27 Jun 2023 15:10:19 -0700 > > Abhinav Kumar wrote: > > > >> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: > >>> On 28/06/2023 00:27, Jessica Zhang wrote: > > > On 6/27/2023 12:58 AM, Pekka Paalanen wrote: > > On Mon, 26 Jun 2023 16:02:50 -0700 > > Jessica Zhang wrote: > > > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote: > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: > Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT > properties. When the color fill value is set, and the framebuffer > is set > to NULL, memory fetch will be disabled. > >>> > >>> Thinking a bit more universally I wonder if there should be > >>> some kind of enum property: > >>> > >>> enum plane_pixel_source { > >>> FB, > >>> COLOR, > >>> LIVE_FOO, > >>> LIVE_BAR, > >>> ... > >>> } > >> > >> Reviving this thread as this was the initial comment suggesting to > >> implement pixel_source as an enum. > >> > >> I think the issue with having pixel_source as an enum is how to decide > >> what counts as a NULL commit. > >> > >> Currently, setting the FB to NULL will disable the plane. So I'm > >> guessing we will extend that logic to "if there's no pixel_source set > >> for the plane, then it will be a NULL commit and disable the plane". > >> > >> In that case, the question then becomes when to set the pixel_source to > >> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill > >> blob), it then forces userspace to set one property before the other. > > > > Right, that won't work. > > > > There is no ordering between each property being set inside a single > > atomic commit. They can all be applied to kernel-internal state > > theoretically simultaneously, or any arbitrary random order, and the > > end result must always be the same. Hence, setting one property cannot > > change the state of another mutable property. I believe that doing > > otherwise would make userspace fragile and hard to get right. > > > > I guess there might be an exception to that rule when the same property > > is set multiple times in a single atomic commit; the last setting in > > the array prevails. That's universal and not a special-case between two > > specific properties. > > > >> Because of that, I'm thinking of having pixel_source be represented > >> by a > >> bitmask instead. That way, we will simply unset the corresponding > >> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in > >> order to detect whether a commit is NULL or has a valid pixel > >> source, we > >> can just check if pixel_source == 0. > > > > Sounds fine to me at first hand, but isn't there the enum property that > > says if the kernel must look at solid_fill blob *or* FB_ID? > > > > If enum prop says "use solid_fill prop", the why would changes to FB_ID > > do anything? Is it for backwards-compatibility with KMS clients that do > > not know about the enum prop? > > > > It seems like that kind of backwards-compatiblity will cause problems > > in trying to reason about the atomic state, as explained above, leading > > to very delicate and fragile conditions where things work intuitively. > > Hence, I'm not sure backwards-compatibility is wanted. This won't be > > the first or the last KMS property where an unexpected value left over > > will make old atomic KMS clients silently malfunction up to showing no > > recognisable picture at all. *If* that problem needs solving, there > > have been ideas floating around about resetting everything to nice > > values so that userspace can ignore what it does not understand. So far > > there has been no real interest in solving that problem in the kernel > > though. > > > > 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. > > Hi Pekka and Dmitry, > > After reading through both of your comments, I think I have a better > understanding of the pixel_source implementation now. > > So to summarize, we want to expose another property called > "pixel_source" to userspace that will default to FB (as to not break > legacy userspace). > > If userspace wants to use solid fill planes, it will set both the > solid_fill *and* pixel_source properties to a valid blob and COLOR > respectively. If it wants to use FB, it will set FB_ID and > pixel_source to a valid FB and FB. > >>
Re: [RFC PATCH 0/3] Support for Solid Fill Planes
On 6/27/2023 3:10 PM, Abhinav Kumar wrote: On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: On 28/06/2023 00:27, Jessica Zhang wrote: On 6/27/2023 12:58 AM, Pekka Paalanen wrote: On Mon, 26 Jun 2023 16:02:50 -0700 Jessica Zhang wrote: On 11/7/2022 11:37 AM, Ville Syrjälä wrote: On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT properties. When the color fill value is set, and the framebuffer is set to NULL, memory fetch will be disabled. Thinking a bit more universally I wonder if there should be some kind of enum property: enum plane_pixel_source { FB, COLOR, LIVE_FOO, LIVE_BAR, ... } Reviving this thread as this was the initial comment suggesting to implement pixel_source as an enum. I think the issue with having pixel_source as an enum is how to decide what counts as a NULL commit. Currently, setting the FB to NULL will disable the plane. So I'm guessing we will extend that logic to "if there's no pixel_source set for the plane, then it will be a NULL commit and disable the plane". In that case, the question then becomes when to set the pixel_source to NONE. Because if we do that when setting a NULL FB (or NULL solid_fill blob), it then forces userspace to set one property before the other. Right, that won't work. There is no ordering between each property being set inside a single atomic commit. They can all be applied to kernel-internal state theoretically simultaneously, or any arbitrary random order, and the end result must always be the same. Hence, setting one property cannot change the state of another mutable property. I believe that doing otherwise would make userspace fragile and hard to get right. I guess there might be an exception to that rule when the same property is set multiple times in a single atomic commit; the last setting in the array prevails. That's universal and not a special-case between two specific properties. Because of that, I'm thinking of having pixel_source be represented by a bitmask instead. That way, we will simply unset the corresponding pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in order to detect whether a commit is NULL or has a valid pixel source, we can just check if pixel_source == 0. Sounds fine to me at first hand, but isn't there the enum property that says if the kernel must look at solid_fill blob *or* FB_ID? If enum prop says "use solid_fill prop", the why would changes to FB_ID do anything? Is it for backwards-compatibility with KMS clients that do not know about the enum prop? It seems like that kind of backwards-compatiblity will cause problems in trying to reason about the atomic state, as explained above, leading to very delicate and fragile conditions where things work intuitively. Hence, I'm not sure backwards-compatibility is wanted. This won't be the first or the last KMS property where an unexpected value left over will make old atomic KMS clients silently malfunction up to showing no recognisable picture at all. *If* that problem needs solving, there have been ideas floating around about resetting everything to nice values so that userspace can ignore what it does not understand. So far there has been no real interest in solving that problem in the kernel though. 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. Hi Pekka and Dmitry, After reading through both of your comments, I think I have a better understanding of the pixel_source implementation now. So to summarize, we want to expose another property called "pixel_source" to userspace that will default to FB (as to not break legacy userspace). If userspace wants to use solid fill planes, it will set both the solid_fill *and* pixel_source properties to a valid blob and COLOR respectively. If it wants to use FB, it will set FB_ID and pixel_source to a valid FB and FB. Here's a table illustrating what I've described above: +-+-+-+ | Use Case | Legacy Userspace | solid_fill-aware | | | | Userspace | +=+=+=+ | Valid FB | pixel_source = FB | pixel_source = FB | | | FB_ID = valid FB | FB_ID = valid FB | +-+-+-+ | Valid | pixel_source = COLOR | N/A | | solid_fill blob | solid_fill = valid blob | | Probably these two cells were swapped. Ack, yes they were swapped. +-+-+-+ | NULL commit | pixel_source = FB | pixel_source = FB | |
Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
On 6/29/2023 12:29 AM, Pekka Paalanen wrote: On Wed, 28 Jun 2023 09:40:21 -0700 Jessica Zhang wrote: On 6/28/2023 12:34 AM, Pekka Paalanen wrote: On Tue, 27 Jun 2023 15:10:19 -0700 Abhinav Kumar wrote: On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote: On 28/06/2023 00:27, Jessica Zhang wrote: On 6/27/2023 12:58 AM, Pekka Paalanen wrote: On Mon, 26 Jun 2023 16:02:50 -0700 Jessica Zhang wrote: On 11/7/2022 11:37 AM, Ville Syrjälä wrote: On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote: Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT properties. When the color fill value is set, and the framebuffer is set to NULL, memory fetch will be disabled. Thinking a bit more universally I wonder if there should be some kind of enum property: enum plane_pixel_source { FB, COLOR, LIVE_FOO, LIVE_BAR, ... } Reviving this thread as this was the initial comment suggesting to implement pixel_source as an enum. I think the issue with having pixel_source as an enum is how to decide what counts as a NULL commit. Currently, setting the FB to NULL will disable the plane. So I'm guessing we will extend that logic to "if there's no pixel_source set for the plane, then it will be a NULL commit and disable the plane". In that case, the question then becomes when to set the pixel_source to NONE. Because if we do that when setting a NULL FB (or NULL solid_fill blob), it then forces userspace to set one property before the other. Right, that won't work. There is no ordering between each property being set inside a single atomic commit. They can all be applied to kernel-internal state theoretically simultaneously, or any arbitrary random order, and the end result must always be the same. Hence, setting one property cannot change the state of another mutable property. I believe that doing otherwise would make userspace fragile and hard to get right. I guess there might be an exception to that rule when the same property is set multiple times in a single atomic commit; the last setting in the array prevails. That's universal and not a special-case between two specific properties. Because of that, I'm thinking of having pixel_source be represented by a bitmask instead. That way, we will simply unset the corresponding pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in order to detect whether a commit is NULL or has a valid pixel source, we can just check if pixel_source == 0. Sounds fine to me at first hand, but isn't there the enum property that says if the kernel must look at solid_fill blob *or* FB_ID? If enum prop says "use solid_fill prop", the why would changes to FB_ID do anything? Is it for backwards-compatibility with KMS clients that do not know about the enum prop? It seems like that kind of backwards-compatiblity will cause problems in trying to reason about the atomic state, as explained above, leading to very delicate and fragile conditions where things work intuitively. Hence, I'm not sure backwards-compatibility is wanted. This won't be the first or the last KMS property where an unexpected value left over will make old atomic KMS clients silently malfunction up to showing no recognisable picture at all. *If* that problem needs solving, there have been ideas floating around about resetting everything to nice values so that userspace can ignore what it does not understand. So far there has been no real interest in solving that problem in the kernel though. 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. Hi Pekka and Dmitry, After reading through both of your comments, I think I have a better understanding of the pixel_source implementation now. So to summarize, we want to expose another property called "pixel_source" to userspace that will default to FB (as to not break legacy userspace). If userspace wants to use solid fill planes, it will set both the solid_fill *and* pixel_source properties to a valid blob and COLOR respectively. If it wants to use FB, it will set FB_ID and pixel_source to a valid FB and FB. Here's a table illustrating what I've described above: +-+-+-+ | Use Case | Legacy Userspace | solid_fill-aware | | | | Userspace | +=+=+=+ | Valid FB | pixel_source = FB | pixel_source = FB | | | FB_ID = valid FB | FB_ID = valid FB | +-+-+-+ | Valid | pixel_source = COLOR | N/A | | solid_fill blob | solid_fill = valid blob | | Probably these two cells were swapped. Ack, yes th
[PATCH RFC v4 3/7] drm/atomic: Move framebuffer checks to helper
Currently framebuffer checks happen directly in drm_atomic_plane_check(). Move these checks into their own helper method. Signed-off-by: Jessica Zhang --- 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 & 0x) * 15625) >> 10, + state->src_h >> 16, + ((state->src_h & 0x) * 15625) >> 10, + state->src_x >> 16, + ((state->src_x & 0x) * 15625) >> 10, + state->src_y >> 16, + ((state->src_y & 0x) * 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
[PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
Add support for pixel_source property to drm_plane and related documentation. This enum property will allow user to specify a pixel source for the plane. Possible pixel sources will be defined in the drm_plane_pixel_source enum. The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/drm_atomic_state_helper.c | 1 + drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_blend.c | 81 +++ include/drm/drm_blend.h | 2 + include/drm/drm_plane.h | 21 5 files changed, 109 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index fe14be2bd2b2..86fb876efbe6 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB; if (plane_state->solid_fill_blob) { drm_property_blob_put(plane_state->solid_fill_blob); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index a28b4ee79444..6e59c21af66b 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, drm_property_blob_put(solid_fill); return ret; + } else if (property == plane->pixel_source_property) { + state->pixel_source = val; } else if (property == plane->alpha_property) { state->alpha = val; } else if (property == plane->blend_mode_property) { @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, } else if (property == plane->solid_fill_property) { *val = state->solid_fill_blob ? state->solid_fill_blob->base.id : 0; + } else if (property == plane->pixel_source_property) { + *val = state->pixel_source; } else if (property == plane->alpha_property) { *val = state->alpha; } else if (property == plane->blend_mode_property) { diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index 38c3c5d6453a..8c100a957ee2 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -189,6 +189,18 @@ * solid_fill is set up with drm_plane_create_solid_fill_property(). It * contains pixel data that drivers can use to fill a plane. * + * pixel_source: + * pixel_source is set up with drm_plane_create_pixel_source_property(). + * It is used to toggle the source of pixel data for the plane. + * + * Possible values: + * + * "FB": + * Framebuffer source + * + * "COLOR": + * solid_fill source + * * Note that all the property extensions described here apply either to the * plane or the CRTC (e.g. for the background color, which currently is not * exposed and assumed to be black). @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct drm_plane *plane) return 0; } EXPORT_SYMBOL(drm_plane_create_solid_fill_property); + +/** + * drm_plane_create_pixel_source_property - create a new pixel source property + * @plane: drm plane + * @supported_sources: bitmask of supported pixel_sources for the driver (NOT + * including DRM_PLANE_PIXEL_SOURCE_FB, as it will be supported + * by default). + * + * This creates a new property describing the current source of pixel data for the + * plane. + * + * The property is exposed to userspace as an enumeration property called + * "pixel_source" and has the following enumeration values: + * + * "FB": + * Framebuffer pixel source + * + * "COLOR": + * Solid fill color pixel source + * + * 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; + +
[PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property
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 --- 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; + + 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); + d
[PATCH RFC v4 0/7] Support for Solid Fill Planes
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_jessz...@quicinc.com Changes in v3: - Fixed some logic errors in atomic checks (Dmitry) - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper methods (Dmitry) - Fixed typo in drm_solid_fill struct documentation - 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, -- Jessica Zhang
[PATCH RFC v4 7/7] drm/msm/dpu: Use DRM solid_fill property
Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to determine if the plane is solid fill. In addition drop the DPU plane color_fill field as we can now use drm_plane_state.solid_fill instead, and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to allow userspace to configure the alpha value for the solid fill color. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 4476722f03bb..11d4fb771a1f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -42,7 +42,6 @@ #define SHARP_SMOOTH_THR_DEFAULT 8 #define SHARP_NOISE_THR_DEFAULT2 -#define DPU_PLANE_COLOR_FILL_FLAG BIT(31) #define DPU_ZPOS_MAX 255 /* @@ -82,7 +81,6 @@ struct dpu_plane { enum dpu_sspp pipe; - uint32_t color_fill; bool is_error; bool is_rt_pipe; const struct dpu_mdss_cfg *catalog; @@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate, _dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation); } +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill) +{ + uint32_t ret = 0; + + ret |= ((uint8_t) solid_fill.b) << 16; + ret |= ((uint8_t) solid_fill.g) << 8; + ret |= ((uint8_t) solid_fill.r); + + return ret; +} + /** * _dpu_plane_color_fill - enables color fill on plane * @pdpu: Pointer to DPU plane object @@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane) if (pdpu->is_error) /* force white frame with 100% alpha pipe output on error */ _dpu_plane_color_fill(pdpu, 0xFF, 0xFF); - else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) - /* force 100% alpha */ - _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF); + else if (drm_plane_solid_fill_enabled(plane->state)) + _dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill), + plane->state->alpha); else { dpu_plane_flush_csc(pdpu, &pstate->pipe); dpu_plane_flush_csc(pdpu, &pstate->r_pipe); @@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane, } /* override for color fill */ - if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) { + if (drm_plane_solid_fill_enabled(plane->state)) { _dpu_plane_set_qos_ctrl(plane, pipe, false); /* skip remaining processing on color fill */ -- 2.41.0
[PATCH RFC v4 5/7] drm/msm/dpu: Add solid fill and pixel source properties
Add solid_fill and pixel_source properties to DPU plane Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 ++ 1 file changed, 2 insertions(+) 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) | -- 2.41.0
[PATCH RFC v4 6/7] drm/msm/dpu: Allow NULL FBs in atomic commit
Since solid fill planes allow for a NULL framebuffer in a valid commit, add NULL framebuffer checks to atomic commit calls within DPU. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 45 +++ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 1edf2b6b0a26..d1b37d2cc202 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -451,6 +451,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, struct drm_plane_state *state; struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); struct dpu_plane_state *pstate = NULL; + const struct msm_format *fmt; struct dpu_format *format; struct dpu_hw_ctl *ctl = mixer->lm_ctl; @@ -470,7 +471,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, pstate = to_dpu_plane_state(state); fb = state->fb; - format = to_dpu_format(msm_framebuffer_format(pstate->base.fb)); + if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) + fmt = msm_framebuffer_format(pstate->base.fb); + else + fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base, + DRM_FORMAT_RGBA, 0); + + format = to_dpu_format(fmt); if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable) bg_alpha_enable = true; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 5f0984ce62b1..4476722f03bb 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -837,8 +837,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, pipe_cfg->dst_rect = new_plane_state->dst; - fb_rect.x2 = new_plane_state->fb->width; - fb_rect.y2 = new_plane_state->fb->height; + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && new_plane_state->fb) { + fb_rect.x2 = new_plane_state->fb->width; + fb_rect.y2 = new_plane_state->fb->height; + } /* Ensure fb size is supported */ if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH || @@ -848,10 +850,13 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, return -E2BIG; } - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); - max_linewidth = pdpu->catalog->caps->max_linewidth; + if (drm_plane_solid_fill_enabled(new_plane_state)) + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR); + else + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { /* * In parallel multirect case only the half of the usual width @@ -1082,21 +1087,32 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) struct drm_crtc *crtc = state->crtc; struct drm_framebuffer *fb = state->fb; bool is_rt_pipe; - const struct dpu_format *fmt = - to_dpu_format(msm_framebuffer_format(fb)); + const struct dpu_format *fmt; struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); struct msm_gem_address_space *aspace = kms->base.aspace; struct dpu_hw_fmt_layout layout; bool layout_valid = false; - int ret; - ret = dpu_format_populate_layout(aspace, fb, &layout); - if (ret) - DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); - else - layout_valid = true; + if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + int ret; + + fmt = to_dpu_format(msm_framebuffer_format(fb)); + + ret = dpu_format_populate_layout(aspace, fb, &layout); + if (ret) + DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); + else + layout_valid = true; + + DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT + ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src), + crtc->base.id, DRM_RECT_ARG(&state->dst), + (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt)); + } else { + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR); + } pstate->pending = true; @@ -1104,11 +1120,6 @@ static void dpu_plane_sspp_atomic_up
[PATCH RFC v4 4/7] drm/atomic: Loosen FB atomic checks
Loosen the requirements for atomic and legacy commit so that, in cases where solid fill planes is enabled but no FB is set, the commit can still go through. This includes adding framebuffer NULL checks in other areas to account for FB being NULL when solid fill is enabled. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/drm_atomic.c| 14 +++--- drivers/gpu/drm/drm_atomic_helper.c | 34 -- drivers/gpu/drm/drm_plane.c | 8 include/drm/drm_atomic_helper.h | 4 ++-- include/drm/drm_plane.h | 28 5 files changed, 61 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 404b984d2d9f..ec9681c25366 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -668,14 +668,14 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, const struct drm_framebuffer *fb = new_plane_state->fb; int ret; - /* either *both* CRTC and FB must be set, or neither */ - if (crtc && !fb) { - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n", + /* either *both* CRTC and pixel source must be set, or neither */ + if (crtc && !drm_plane_has_visible_data(new_plane_state)) { + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no visible data\n", plane->base.id, plane->name); return -EINVAL; - } else if (fb && !crtc) { - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] FB set but no CRTC\n", - plane->base.id, plane->name); + } else if (drm_plane_has_visible_data(new_plane_state) && !crtc) { + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Source %d has visible data but no CRTC\n", + plane->base.id, plane->name, new_plane_state->pixel_source); return -EINVAL; } @@ -706,7 +706,7 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, } - if (fb) { + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { ret = drm_atomic_check_fb(new_plane_state); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 41b8066f61ff..d05ec9ef2b3e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, *src = drm_plane_state_src(plane_state); *dst = drm_plane_state_dest(plane_state); - if (!fb) { + if (!drm_plane_has_visible_data(plane_state)) { plane_state->visible = false; return 0; } @@ -881,25 +881,31 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, return -EINVAL; } - drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); + if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); - /* Check scaling */ - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); - if (hscale < 0 || vscale < 0) { - drm_dbg_kms(plane_state->plane->dev, - "Invalid scaling of plane\n"); - drm_rect_debug_print("src: ", &plane_state->src, true); - drm_rect_debug_print("dst: ", &plane_state->dst, false); - return -ERANGE; + /* Check scaling */ + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); + + if (hscale < 0 || vscale < 0) { + drm_dbg_kms(plane_state->plane->dev, + "Invalid scaling of plane\n"); + drm_rect_debug_print("src: ", &plane_state->src, true); + drm_rect_debug_print("dst: ", &plane_state->dst, false); + return -ERANGE; + } } if (crtc_state->enable) drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); - - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); + if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); + } else if (drm_plane_solid_fill_enabled(plane_state)) { +
Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
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. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/drm_atomic_state_helper.c | 1 + drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_blend.c | 81 +++ include/drm/drm_blend.h | 2 + include/drm/drm_plane.h | 21 5 files changed, 109 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index fe14be2bd2b2..86fb876efbe6 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE; plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB; if (plane_state->solid_fill_blob) { drm_property_blob_put(plane_state->solid_fill_blob); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index a28b4ee79444..6e59c21af66b 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, drm_property_blob_put(solid_fill); return ret; + } else if (property == plane->pixel_source_property) { + state->pixel_source = val; } else if (property == plane->alpha_property) { state->alpha = val; } else if (property == plane->blend_mode_property) { 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. + * + * 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). 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). + * + * 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, + uns
Re: [PATCH RFC v4 3/7] drm/atomic: Move framebuffer checks to helper
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 --- 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 & 0x) * 15625) >> 10, + state->src_h >> 16, + ((state->src_h & 0x) * 15625) >> 10, + state->src_x >> 16, + ((state->src_x & 0x) * 15625) >> 10, + state->src_y >> 16, + ((state->src_y & 0x) * 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 @@ s
Re: [PATCH RFC v4 4/7] drm/atomic: Loosen FB atomic checks
On 30/06/2023 03:25, Jessica Zhang wrote: Loosen the requirements for atomic and legacy commit so that, in cases where solid fill planes is enabled but no FB is set, the commit can still go through. This includes adding framebuffer NULL checks in other areas to account for FB being NULL when solid fill is enabled. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/drm_atomic.c| 14 +++--- drivers/gpu/drm/drm_atomic_helper.c | 34 -- drivers/gpu/drm/drm_plane.c | 8 include/drm/drm_atomic_helper.h | 4 ++-- include/drm/drm_plane.h | 28 5 files changed, 61 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 404b984d2d9f..ec9681c25366 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -668,14 +668,14 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, const struct drm_framebuffer *fb = new_plane_state->fb; int ret; - /* either *both* CRTC and FB must be set, or neither */ - if (crtc && !fb) { - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n", + /* either *both* CRTC and pixel source must be set, or neither */ + if (crtc && !drm_plane_has_visible_data(new_plane_state)) { + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no visible data\n", plane->base.id, plane->name); return -EINVAL; - } else if (fb && !crtc) { - drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] FB set but no CRTC\n", - plane->base.id, plane->name); + } else if (drm_plane_has_visible_data(new_plane_state) && !crtc) { + drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Source %d has visible data but no CRTC\n", + plane->base.id, plane->name, new_plane_state->pixel_source); return -EINVAL; } @@ -706,7 +706,7 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, } - if (fb) { + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { ret = drm_atomic_check_fb(new_plane_state); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 41b8066f61ff..d05ec9ef2b3e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, *src = drm_plane_state_src(plane_state); *dst = drm_plane_state_dest(plane_state); - if (!fb) { + if (!drm_plane_has_visible_data(plane_state)) { plane_state->visible = false; return 0; } @@ -881,25 +881,31 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, return -EINVAL; } - drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); + if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); - /* Check scaling */ - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); - if (hscale < 0 || vscale < 0) { - drm_dbg_kms(plane_state->plane->dev, - "Invalid scaling of plane\n"); - drm_rect_debug_print("src: ", &plane_state->src, true); - drm_rect_debug_print("dst: ", &plane_state->dst, false); - return -ERANGE; + /* Check scaling */ + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); + + if (hscale < 0 || vscale < 0) { + drm_dbg_kms(plane_state->plane->dev, + "Invalid scaling of plane\n"); + drm_rect_debug_print("src: ", &plane_state->src, true); + drm_rect_debug_print("dst: ", &plane_state->dst, false); + return -ERANGE; + } } if (crtc_state->enable) drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); - - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); + if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); + } else if (drm_plane_solid_fill_enabled(plane
Re: [PATCH RFC v4 5/7] drm/msm/dpu: Add solid fill and pixel source properties
On 30/06/2023 03:25, Jessica Zhang wrote: Add solid_fill and pixel_source properties to DPU plane Signed-off-by: Jessica Zhang --- 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 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
Re: [PATCH RFC v4 6/7] drm/msm/dpu: Allow NULL FBs in atomic commit
On 30/06/2023 03:25, Jessica Zhang wrote: Since solid fill planes allow for a NULL framebuffer in a valid commit, add NULL framebuffer checks to atomic commit calls within DPU. Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 45 +++ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 1edf2b6b0a26..d1b37d2cc202 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -451,6 +451,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, struct drm_plane_state *state; struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); struct dpu_plane_state *pstate = NULL; + const struct msm_format *fmt; struct dpu_format *format; struct dpu_hw_ctl *ctl = mixer->lm_ctl; @@ -470,7 +471,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, pstate = to_dpu_plane_state(state); fb = state->fb; - format = to_dpu_format(msm_framebuffer_format(pstate->base.fb)); + if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) + fmt = msm_framebuffer_format(pstate->base.fb); + else + fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base, + DRM_FORMAT_RGBA, 0); The DRM_FORMAT_RGBA should be defined somewhere in patch 1 as format for the solid_fill, then that define can be used in this patch. + + format = to_dpu_format(fmt); if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable) bg_alpha_enable = true; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 5f0984ce62b1..4476722f03bb 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -837,8 +837,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, pipe_cfg->dst_rect = new_plane_state->dst; - fb_rect.x2 = new_plane_state->fb->width; - fb_rect.y2 = new_plane_state->fb->height; + if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && new_plane_state->fb) { + fb_rect.x2 = new_plane_state->fb->width; + fb_rect.y2 = new_plane_state->fb->height; + } /* Ensure fb size is supported */ if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH || @@ -848,10 +850,13 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, return -E2BIG; } - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); - max_linewidth = pdpu->catalog->caps->max_linewidth; + if (drm_plane_solid_fill_enabled(new_plane_state)) + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR); + else + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { /* * In parallel multirect case only the half of the usual width @@ -1082,21 +1087,32 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) struct drm_crtc *crtc = state->crtc; struct drm_framebuffer *fb = state->fb; bool is_rt_pipe; - const struct dpu_format *fmt = - to_dpu_format(msm_framebuffer_format(fb)); + const struct dpu_format *fmt; struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); struct msm_gem_address_space *aspace = kms->base.aspace; struct dpu_hw_fmt_layout layout; bool layout_valid = false; - int ret; - ret = dpu_format_populate_layout(aspace, fb, &layout); - if (ret) - DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); - else - layout_valid = true; + if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) { + int ret; + + fmt = to_dpu_format(msm_framebuffer_format(fb)); + + ret = dpu_format_populate_layout(aspace, fb, &layout); + if (ret) + DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); + else + layout_valid = true; + + DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT + ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src), + crtc->base.id, DRM_RECT_ARG(&state->dst), + (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt)); + } else { + fmt =
Re: [PATCH RFC v4 7/7] drm/msm/dpu: Use DRM solid_fill property
On 30/06/2023 03:25, Jessica Zhang wrote: Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to determine if the plane is solid fill. In addition drop the DPU plane color_fill field as we can now use drm_plane_state.solid_fill instead, and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to allow userspace to configure the alpha value for the solid fill color. Signed-off-by: Jessica Zhang Reviewed-by: Dmitry Baryshkov Minor suggestion below. --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 4476722f03bb..11d4fb771a1f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -42,7 +42,6 @@ #define SHARP_SMOOTH_THR_DEFAULT 8 #define SHARP_NOISE_THR_DEFAULT 2 -#define DPU_PLANE_COLOR_FILL_FLAG BIT(31) #define DPU_ZPOS_MAX 255 /* @@ -82,7 +81,6 @@ struct dpu_plane { enum dpu_sspp pipe; - uint32_t color_fill; bool is_error; bool is_rt_pipe; const struct dpu_mdss_cfg *catalog; @@ -606,6 +604,17 @@ static void _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate, _dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, pstate->rotation); } +static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill) Please consider accepting drm_plane_state instead and handling alpha here. Then _dpu_color_fill can accept rgba colour instead of separate RGB and alpha values. +{ + uint32_t ret = 0; + + ret |= ((uint8_t) solid_fill.b) << 16; + ret |= ((uint8_t) solid_fill.g) << 8; + ret |= ((uint8_t) solid_fill.r); + + return ret; +} + /** * _dpu_plane_color_fill - enables color fill on plane * @pdpu: Pointer to DPU plane object @@ -977,9 +986,9 @@ void dpu_plane_flush(struct drm_plane *plane) if (pdpu->is_error) /* force white frame with 100% alpha pipe output on error */ _dpu_plane_color_fill(pdpu, 0xFF, 0xFF); - else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) - /* force 100% alpha */ - _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF); + else if (drm_plane_solid_fill_enabled(plane->state)) + _dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill), + plane->state->alpha); else { dpu_plane_flush_csc(pdpu, &pstate->pipe); dpu_plane_flush_csc(pdpu, &pstate->r_pipe); @@ -1024,7 +1033,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane, } /* override for color fill */ - if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) { + if (drm_plane_solid_fill_enabled(plane->state)) { _dpu_plane_set_qos_ctrl(plane, pipe, false); /* skip remaining processing on color fill */ -- With best wishes Dmitry