Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On 10/26/23 21:25, Alex Goins wrote: > On Thu, 26 Oct 2023, Sebastian Wick wrote: >> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: >>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) >>> Alex Goins wrote: >>> Despite being programmable, the LUTs are updated in a manner that is less efficient as compared to e.g. the non-static "degamma" LUT. Would it be helpful if there was some way to tag operations according to their performance, for example so that clients can prefer a high performance one when they intend to do an animated transition? I recall from the XDC HDR workshop that this is also an issue with AMD's 3DLUT, where updates can be too slow to animate. >>> >>> I can certainly see such information being useful, but then we need to >>> somehow quantize the performance. > > Right, which wouldn't even necessarily be universal, could depend on the given > host, GPU, etc. It could just be a relative performance indication, to give an > order of preference. That wouldn't tell you if it can or can't be animated, > but > when choosing between two LUTs to animate you could prefer the higher > performance one. > >>> >>> What I was left puzzled about after the XDC workshop is that is it >>> possible to pre-load configurations in the background (slow), and then >>> quickly switch between them? Hardware-wise I mean. > > This works fine for our "fast" LUTs, you just point them to a surface in video > memory and they flip to it. You could keep multiple surfaces around and flip > between them without having to reprogram them in software. We can easily do > that > with enumerated curves, populating them when the driver initializes instead of > waiting for the client to request them. You can even point multiple hardware > LUTs to the same video memory surface, if they need the same curve. > >> >> We could define that pipelines with a lower ID are to be preferred over >> higher IDs. > > Sure, but this isn't just an issue with a pipeline as a whole, but the > individual elements within it and how to use them in a given context. > >> >> The issue is that if programming a pipeline becomes too slow to be >> useful it probably should just not be made available to user space. > > It's not that programming the pipeline is overall too slow. The LUTs we have > that are relatively slow to program are meant to be set infrequently, or even > just once, to allow the scaler and tone mapping operator to operate in fixed > point PQ space. You might still want the tone mapper, so you would choose a > pipeline that includes them, but when it comes to e.g. animating a night > light, > you would want to choose a different LUT for that purpose. > >> >> The prepare-commit idea for blob properties would help to make the >> pipelines usable again, but until then it's probably a good idea to just >> not expose those pipelines. > > The prepare-commit idea actually wouldn't work for these LUTs, because they > are > programmed using methods instead of pointing them to a surface. I'm actually > not > sure how slow it actually is, would need to benchmark it. I think not exposing > them at all would be overkill, since it would mean you can't use the > preblending > scaler or tonemapper, and animation isn't necessary for that. > > The AMD 3DLUT is another example of a LUT that is slow to update, and it would > obviously be a major loss if that wasn't exposed. There just needs to be some > way for clients to know if they are going to kill performance by trying to > change it every frame. Might a first step be to require the ALLOW_MODESET flag to be set when changing the values for a colorop which is too slow to be updated per refresh cycle? This would tell the compositor: You can use this colorop, but you can't change its values on the fly. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Fri, Oct 27, 2023 at 10:59:25AM +0200, Michel Dänzer wrote: > On 10/26/23 21:25, Alex Goins wrote: > > On Thu, 26 Oct 2023, Sebastian Wick wrote: > >> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: > >>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) > >>> Alex Goins wrote: > >>> > Despite being programmable, the LUTs are updated in a manner that is less > efficient as compared to e.g. the non-static "degamma" LUT. Would it be > helpful > if there was some way to tag operations according to their performance, > for example so that clients can prefer a high performance one when they > intend to do an animated transition? I recall from the XDC HDR workshop > that this is also an issue with AMD's 3DLUT, where updates can be too > slow to animate. > >>> > >>> I can certainly see such information being useful, but then we need to > >>> somehow quantize the performance. > > > > Right, which wouldn't even necessarily be universal, could depend on the > > given > > host, GPU, etc. It could just be a relative performance indication, to give > > an > > order of preference. That wouldn't tell you if it can or can't be animated, > > but > > when choosing between two LUTs to animate you could prefer the higher > > performance one. > > > >>> > >>> What I was left puzzled about after the XDC workshop is that is it > >>> possible to pre-load configurations in the background (slow), and then > >>> quickly switch between them? Hardware-wise I mean. > > > > This works fine for our "fast" LUTs, you just point them to a surface in > > video > > memory and they flip to it. You could keep multiple surfaces around and flip > > between them without having to reprogram them in software. We can easily do > > that > > with enumerated curves, populating them when the driver initializes instead > > of > > waiting for the client to request them. You can even point multiple hardware > > LUTs to the same video memory surface, if they need the same curve. > > > >> > >> We could define that pipelines with a lower ID are to be preferred over > >> higher IDs. > > > > Sure, but this isn't just an issue with a pipeline as a whole, but the > > individual elements within it and how to use them in a given context. > > > >> > >> The issue is that if programming a pipeline becomes too slow to be > >> useful it probably should just not be made available to user space. > > > > It's not that programming the pipeline is overall too slow. The LUTs we have > > that are relatively slow to program are meant to be set infrequently, or > > even > > just once, to allow the scaler and tone mapping operator to operate in fixed > > point PQ space. You might still want the tone mapper, so you would choose a > > pipeline that includes them, but when it comes to e.g. animating a night > > light, > > you would want to choose a different LUT for that purpose. > > > >> > >> The prepare-commit idea for blob properties would help to make the > >> pipelines usable again, but until then it's probably a good idea to just > >> not expose those pipelines. > > > > The prepare-commit idea actually wouldn't work for these LUTs, because they > > are > > programmed using methods instead of pointing them to a surface. I'm > > actually not > > sure how slow it actually is, would need to benchmark it. I think not > > exposing > > them at all would be overkill, since it would mean you can't use the > > preblending > > scaler or tonemapper, and animation isn't necessary for that. > > > > The AMD 3DLUT is another example of a LUT that is slow to update, and it > > would > > obviously be a major loss if that wasn't exposed. There just needs to be > > some > > way for clients to know if they are going to kill performance by trying to > > change it every frame. > > Might a first step be to require the ALLOW_MODESET flag to be set when > changing the values for a colorop which is too slow to be updated per refresh > cycle? > > This would tell the compositor: You can use this colorop, but you can't > change its values on the fly. I argued before that changing any color op to passthrough should never require ALLOW_MODESET and while this is really hard to guarantee from a driver perspective I still believe that it's better to not expose any feature requiring ALLOW_MODESET or taking too long to program to be useful for per-frame changes. When user space has ways to figure out if going back to a specific state (in this case setting everything to bypass) without ALLOW_MODESET we can revisit this decision, but until then, let's keep things simple and only expose things that work reliably without ALLOW_MODESET and fast enough to work for per-frame changes. Harry, Pekka: Should we document this? It obviously restricts what can be exposed but exposing things that can't be used by user space isn't useful. > > -- > Earthling Michel Dänzer| https://redhat.com > Libre software enthusias
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
Am Fr., 27. Okt. 2023 um 12:01 Uhr schrieb Sebastian Wick < sebastian.w...@redhat.com>: > On Fri, Oct 27, 2023 at 10:59:25AM +0200, Michel Dänzer wrote: > > On 10/26/23 21:25, Alex Goins wrote: > > > On Thu, 26 Oct 2023, Sebastian Wick wrote: > > >> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: > > >>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) > > >>> Alex Goins wrote: > > >>> > > Despite being programmable, the LUTs are updated in a manner that > is less > > efficient as compared to e.g. the non-static "degamma" LUT. Would > it be helpful > > if there was some way to tag operations according to their > performance, > > for example so that clients can prefer a high performance one when > they > > intend to do an animated transition? I recall from the XDC HDR > workshop > > that this is also an issue with AMD's 3DLUT, where updates can be > too > > slow to animate. > > >>> > > >>> I can certainly see such information being useful, but then we need > to > > >>> somehow quantize the performance. > > > > > > Right, which wouldn't even necessarily be universal, could depend on > the given > > > host, GPU, etc. It could just be a relative performance indication, to > give an > > > order of preference. That wouldn't tell you if it can or can't be > animated, but > > > when choosing between two LUTs to animate you could prefer the higher > > > performance one. > > > > > >>> > > >>> What I was left puzzled about after the XDC workshop is that is it > > >>> possible to pre-load configurations in the background (slow), and > then > > >>> quickly switch between them? Hardware-wise I mean. > > > > > > This works fine for our "fast" LUTs, you just point them to a surface > in video > > > memory and they flip to it. You could keep multiple surfaces around > and flip > > > between them without having to reprogram them in software. We can > easily do that > > > with enumerated curves, populating them when the driver initializes > instead of > > > waiting for the client to request them. You can even point multiple > hardware > > > LUTs to the same video memory surface, if they need the same curve. > > > > > >> > > >> We could define that pipelines with a lower ID are to be preferred > over > > >> higher IDs. > > > > > > Sure, but this isn't just an issue with a pipeline as a whole, but the > > > individual elements within it and how to use them in a given context. > > > > > >> > > >> The issue is that if programming a pipeline becomes too slow to be > > >> useful it probably should just not be made available to user space. > > > > > > It's not that programming the pipeline is overall too slow. The LUTs > we have > > > that are relatively slow to program are meant to be set infrequently, > or even > > > just once, to allow the scaler and tone mapping operator to operate in > fixed > > > point PQ space. You might still want the tone mapper, so you would > choose a > > > pipeline that includes them, but when it comes to e.g. animating a > night light, > > > you would want to choose a different LUT for that purpose. > > > > > >> > > >> The prepare-commit idea for blob properties would help to make the > > >> pipelines usable again, but until then it's probably a good idea to > just > > >> not expose those pipelines. > > > > > > The prepare-commit idea actually wouldn't work for these LUTs, because > they are > > > programmed using methods instead of pointing them to a surface. I'm > actually not > > > sure how slow it actually is, would need to benchmark it. I think not > exposing > > > them at all would be overkill, since it would mean you can't use the > preblending > > > scaler or tonemapper, and animation isn't necessary for that. > > > > > > The AMD 3DLUT is another example of a LUT that is slow to update, and > it would > > > obviously be a major loss if that wasn't exposed. There just needs to > be some > > > way for clients to know if they are going to kill performance by > trying to > > > change it every frame. > > > > Might a first step be to require the ALLOW_MODESET flag to be set when > changing the values for a colorop which is too slow to be updated per > refresh cycle? > > > > This would tell the compositor: You can use this colorop, but you can't > change its values on the fly. > > I argued before that changing any color op to passthrough should never > require ALLOW_MODESET and while this is really hard to guarantee from a > driver perspective I still believe that it's better to not expose any > feature requiring ALLOW_MODESET or taking too long to program to be > useful for per-frame changes. > > When user space has ways to figure out if going back to a specific state > (in this case setting everything to bypass) without ALLOW_MODESET we can > revisit this decision, but until then, let's keep things simple and only > expose things that work reliably without ALLOW_MODESET and fast enough > to work for per-frame changes. > Knowing an operation is fast enoug
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Fri, 27 Oct 2023 12:01:32 +0200 Sebastian Wick wrote: > On Fri, Oct 27, 2023 at 10:59:25AM +0200, Michel Dänzer wrote: > > On 10/26/23 21:25, Alex Goins wrote: > > > On Thu, 26 Oct 2023, Sebastian Wick wrote: > > >> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: > > >>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) > > >>> Alex Goins wrote: > > >>> > > Despite being programmable, the LUTs are updated in a manner that is > > less > > efficient as compared to e.g. the non-static "degamma" LUT. Would it > > be helpful > > if there was some way to tag operations according to their performance, > > for example so that clients can prefer a high performance one when they > > intend to do an animated transition? I recall from the XDC HDR workshop > > that this is also an issue with AMD's 3DLUT, where updates can be too > > slow to animate. > > >>> > > >>> I can certainly see such information being useful, but then we need to > > >>> somehow quantize the performance. > > > > > > Right, which wouldn't even necessarily be universal, could depend on the > > > given > > > host, GPU, etc. It could just be a relative performance indication, to > > > give an > > > order of preference. That wouldn't tell you if it can or can't be > > > animated, but > > > when choosing between two LUTs to animate you could prefer the higher > > > performance one. > > > > > >>> > > >>> What I was left puzzled about after the XDC workshop is that is it > > >>> possible to pre-load configurations in the background (slow), and then > > >>> quickly switch between them? Hardware-wise I mean. > > > > > > This works fine for our "fast" LUTs, you just point them to a surface in > > > video > > > memory and they flip to it. You could keep multiple surfaces around and > > > flip > > > between them without having to reprogram them in software. We can easily > > > do that > > > with enumerated curves, populating them when the driver initializes > > > instead of > > > waiting for the client to request them. You can even point multiple > > > hardware > > > LUTs to the same video memory surface, if they need the same curve. > > > > > >> > > >> We could define that pipelines with a lower ID are to be preferred over > > >> higher IDs. > > > > > > Sure, but this isn't just an issue with a pipeline as a whole, but the > > > individual elements within it and how to use them in a given context. > > > > > >> > > >> The issue is that if programming a pipeline becomes too slow to be > > >> useful it probably should just not be made available to user space. > > > > > > It's not that programming the pipeline is overall too slow. The LUTs we > > > have > > > that are relatively slow to program are meant to be set infrequently, or > > > even > > > just once, to allow the scaler and tone mapping operator to operate in > > > fixed > > > point PQ space. You might still want the tone mapper, so you would choose > > > a > > > pipeline that includes them, but when it comes to e.g. animating a night > > > light, > > > you would want to choose a different LUT for that purpose. > > > > > >> > > >> The prepare-commit idea for blob properties would help to make the > > >> pipelines usable again, but until then it's probably a good idea to just > > >> not expose those pipelines. > > > > > > The prepare-commit idea actually wouldn't work for these LUTs, because > > > they are > > > programmed using methods instead of pointing them to a surface. I'm > > > actually not > > > sure how slow it actually is, would need to benchmark it. I think not > > > exposing > > > them at all would be overkill, since it would mean you can't use the > > > preblending > > > scaler or tonemapper, and animation isn't necessary for that. > > > > > > The AMD 3DLUT is another example of a LUT that is slow to update, and it > > > would > > > obviously be a major loss if that wasn't exposed. There just needs to be > > > some > > > way for clients to know if they are going to kill performance by trying to > > > change it every frame. > > > > Might a first step be to require the ALLOW_MODESET flag to be set when > > changing the values for a colorop which is too slow to be updated per > > refresh cycle? > > > > This would tell the compositor: You can use this colorop, but you can't > > change its values on the fly. > > I argued before that changing any color op to passthrough should never > require ALLOW_MODESET and while this is really hard to guarantee from a > driver perspective I still believe that it's better to not expose any > feature requiring ALLOW_MODESET or taking too long to program to be > useful for per-frame changes. > > When user space has ways to figure out if going back to a specific state > (in this case setting everything to bypass) without ALLOW_MODESET we can > revisit this decision, but until then, let's keep things simple and only > expose things that work
Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
I'm afraid that would not be very useful. It indeed depends on the refresh rate, but also on how close to vblank the compositor does its commits / on what the latency requirements for the currently shown content are. When the compositor presents a fullscreen video with frames that are queued up in advance, needing a full frame to program the atomic commit could be acceptable, but when the user moves the cursor or plays a game, the compositor needs to do the commits as close to vblank as possible. Without a known upper bound on the time that it takes to program the hardware that's not doable. Am Fr., 27. Okt. 2023 um 14:01 Uhr schrieb Pekka Paalanen < ppaala...@gmail.com>: > On Fri, 27 Oct 2023 12:01:32 +0200 > Sebastian Wick wrote: > > > On Fri, Oct 27, 2023 at 10:59:25AM +0200, Michel Dänzer wrote: > > > On 10/26/23 21:25, Alex Goins wrote: > > > > On Thu, 26 Oct 2023, Sebastian Wick wrote: > > > >> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote: > > > >>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT) > > > >>> Alex Goins wrote: > > > >>> > > > Despite being programmable, the LUTs are updated in a manner that > is less > > > efficient as compared to e.g. the non-static "degamma" LUT. Would > it be helpful > > > if there was some way to tag operations according to their > performance, > > > for example so that clients can prefer a high performance one > when they > > > intend to do an animated transition? I recall from the XDC HDR > workshop > > > that this is also an issue with AMD's 3DLUT, where updates can be > too > > > slow to animate. > > > >>> > > > >>> I can certainly see such information being useful, but then we > need to > > > >>> somehow quantize the performance. > > > > > > > > Right, which wouldn't even necessarily be universal, could depend on > the given > > > > host, GPU, etc. It could just be a relative performance indication, > to give an > > > > order of preference. That wouldn't tell you if it can or can't be > animated, but > > > > when choosing between two LUTs to animate you could prefer the higher > > > > performance one. > > > > > > > >>> > > > >>> What I was left puzzled about after the XDC workshop is that is it > > > >>> possible to pre-load configurations in the background (slow), and > then > > > >>> quickly switch between them? Hardware-wise I mean. > > > > > > > > This works fine for our "fast" LUTs, you just point them to a > surface in video > > > > memory and they flip to it. You could keep multiple surfaces around > and flip > > > > between them without having to reprogram them in software. We can > easily do that > > > > with enumerated curves, populating them when the driver initializes > instead of > > > > waiting for the client to request them. You can even point multiple > hardware > > > > LUTs to the same video memory surface, if they need the same curve. > > > > > > > >> > > > >> We could define that pipelines with a lower ID are to be preferred > over > > > >> higher IDs. > > > > > > > > Sure, but this isn't just an issue with a pipeline as a whole, but > the > > > > individual elements within it and how to use them in a given context. > > > > > > > >> > > > >> The issue is that if programming a pipeline becomes too slow to be > > > >> useful it probably should just not be made available to user > space. > > > > > > > > It's not that programming the pipeline is overall too slow. The LUTs > we have > > > > that are relatively slow to program are meant to be set > infrequently, or even > > > > just once, to allow the scaler and tone mapping operator to operate > in fixed > > > > point PQ space. You might still want the tone mapper, so you would > choose a > > > > pipeline that includes them, but when it comes to e.g. animating a > night light, > > > > you would want to choose a different LUT for that purpose. > > > > > > > >> > > > >> The prepare-commit idea for blob properties would help to make the > > > >> pipelines usable again, but until then it's probably a good idea to > just > > > >> not expose those pipelines. > > > > > > > > The prepare-commit idea actually wouldn't work for these LUTs, > because they are > > > > programmed using methods instead of pointing them to a surface. I'm > actually not > > > > sure how slow it actually is, would need to benchmark it. I think > not exposing > > > > them at all would be overkill, since it would mean you can't use the > preblending > > > > scaler or tonemapper, and animation isn't necessary for that. > > > > > > > > The AMD 3DLUT is another example of a LUT that is slow to update, > and it would > > > > obviously be a major loss if that wasn't exposed. There just needs > to be some > > > > way for clients to know if they are going to kill performance by > trying to > > > > change it every frame. > > > > > > Might a first step be to require the ALLOW_MODESET flag to be set when > changing the values for a colorop which is too slow to be updated per > refresh cycle? >
Re: [RFC PATCH v2 02/17] drm: Don't treat 0 as -1 in drm_fixp2int_ceil
Reviewed-by: Simon Ser
Re: [RFC PATCH v2 01/17] drm/atomic: Allow get_value for immutable properties on atomic drivers
Have you seen the comment on top? * Atomic drivers should never call this function directly, the core will read * out property values through the various ->atomic_get_property callbacks. It seems like atomic drivers shouldn't call drm_object_property_get_value() at all?
Re: [RFC PATCH v2 03/17] drm/vkms: Create separate Kconfig file for VKMS
On Thursday, October 19th, 2023 at 23:21, Harry Wentland wrote: > +++ b/drivers/gpu/drm/vkms/Kconfig > @@ -0,0 +1,15 @@ > +# SPDX-License-Identifier: GPL-2.0+ It seems like the original Kconfig uses GPL-2.0-only. I think it'd be safer to just re-use the exact same license here? With that fixed: Reviewed-by: Simon Ser
[PATCH RFC v7 04/10] drm/atomic: Add pixel source to plane state dump
Add pixel source to the atomic plane state dump
Reviewed-by: Dmitry Baryshkov
Acked-by: Harry Wentland
Acked-by: Sebastian Wick
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c| 1 +
drivers/gpu/drm/drm_blend.c | 1 +
drivers/gpu/drm/drm_crtc_internal.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c277b198fa3f..9f9abbe76369 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -722,6 +722,7 @@ static void drm_atomic_plane_print_state(struct drm_printer
*p,
drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name);
drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
"(null)");
+ drm_printf(p, "\tpixel-source=%s\n",
drm_get_pixel_source_name(state->pixel_source));
drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
if (state->fb)
drm_framebuffer_print_info(p, 2, state->fb);
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 37b31b7e5ce5..9c1608f7c1df 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -647,6 +647,7 @@ static const struct drm_prop_enum_list
drm_pixel_source_enum_list[] = {
{ DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
{ DRM_PLANE_PIXEL_SOURCE_SOLID_FILL, "SOLID_FILL" },
};
+DRM_ENUM_NAME_FN(drm_get_pixel_source_name, drm_pixel_source_enum_list);
/**
* drm_plane_create_pixel_source_property - create a new pixel source property
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index 501a10edd0e1..7bc93ba449d5 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -267,6 +267,7 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
u32 format, u64 modifier);
struct drm_mode_rect *
__drm_plane_get_damage_clips(const struct drm_plane_state *state);
+const char *drm_get_pixel_source_name(int val);
/* drm_bridge.c */
void drm_bridge_detach(struct drm_bridge *bridge);
--
2.42.0
[PATCH RFC v7 10/10] drm/msm/dpu: Add solid fill and pixel source properties
Add solid_fill and pixel_source properties to DPU plane
Reviewed-by: Dmitry Baryshkov
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 832747080daf..8e9fda541211 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1493,6 +1493,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_SOLID_FILL));
drm_plane_create_blend_mode_property(plane,
BIT(DRM_MODE_BLEND_PIXEL_NONE) |
BIT(DRM_MODE_BLEND_PREMULTI) |
--
2.42.0
[PATCH RFC v7 08/10] 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.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 ---
2 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 3c475f8042b0..3b9648c679ad 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 (drm_plane_solid_fill_enabled(state))
+ fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
+ DRM_FORMAT_ABGR, 0);
+ else
+ fmt = msm_framebuffer_format(pstate->base.fb);
+
+ 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 3eef5e025e12..9615653db787 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -55,6 +55,8 @@
#define DEFAULT_REFRESH_RATE 60
+#define DPU_SOLID_FILL_FORMAT DRM_FORMAT_ABGR
+
static const uint32_t qcom_compressed_supported_formats[] = {
DRM_FORMAT_ABGR,
DRM_FORMAT_ARGB,
@@ -658,7 +660,7 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
* select fill format to match user property expectation,
* h/w only supports RGB variants
*/
- fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
+ fmt = dpu_get_dpu_format(DPU_SOLID_FILL_FORMAT);
/* should not happen ever */
if (!fmt)
return;
@@ -877,18 +879,23 @@ 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 ||
- drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) {
- DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n",
- DRM_RECT_ARG(&fb_rect));
- return -E2BIG;
+ /* Ensure fb size is supported */
+ if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH ||
+ drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) {
+ DPU_DEBUG_PLANE(pdpu, "invalid framebuffer "
DRM_RECT_FMT "\n",
+ DRM_RECT_ARG(&fb_rect));
+ return -E2BIG;
+ }
}
- fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ if (drm_plane_solid_fill_enabled(new_plane_state))
+ fmt = dpu_get_dpu_format(DPU_SOLID_FILL_FORMAT);
+ else
+ fmt =
to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
max_linewidth = pdpu->catalog->caps->max_linewidth;
@@ -1123,8 +1130,7 @@ 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);
@@ -1133,6 +1139,11 @@ static void dpu_plane_sspp_atomic_update(struct
drm_plane *plane)
bool layout_valid = false;
int ret;
+ if (drm_plane_solid_fill_enabled(state))
+ return;
+
+ fmt = to_dpu_format(msm_framebuffer_format(fb));
+
ret = dpu_format_populate_layout(aspace, fb, &layout);
if (ret)
[PATCH RFC v7 00/10] 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 test 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 solid fill color (in RGB323232 format) and and setting the pixel_source property to DRM_PLANE_PIXEL_SOURCE_SOLID_FILL. This will disable memory fetch and the resulting plane will display the color specified by the solid_fill blob. In order to disable a solid fill plane, the user must set the pixel_source to NONE. A plane with a pixel_source of "solid_fill" and a NULL solid_fill blob will cause an error on commit. 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 extending the pixel_source enum and adding another solid fill blob property. 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 v7: - Updated cover letter (Sebastian) - Updated the uAPI documentation (Sebastian, Pekka) - Specify that padding must be set to zero in drm_mode_solid_fill documentation (Pekka) - Use %08x format when printing solid fill info in plane state dump (Pekka) - Use new_plane_state->fb directly in drm_atomic_plane_check() (Dmitry) - Dropped documentation for alpha for _dpu_plane_color_fill() (Dmitry) - Defined DPU_SOLID_FILL_FORMAT macro (Dmitry) - Fixed some necessary checks being skipped in the DPU atomic commit path (Dmitry) - Rebased to tip of msm-next - Picked up Acked-by and Reviewed-by tags Changes in v6: - Have _dpu_plane_color_fill() take in a single ABGR color instead of having separate alpha and BGR color parameters (Dmitry) - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check in SetPlane ioctl (Dmitry) - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian) - Dropped versioning from solid fill property blob (Dmitry) - Use DRM_ENUM_NAME_FN (Dmitry) - Use drm_atomic_replace_property_blob_from_id() (Dmitry) - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry) - Group redundant NULL FB checks (Dmitry) - Squashed drm_plane_needs_disable() implementation with DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian) - Add comment to support RGBA solid fill color in the future (Dmitry) - Link to v5: https://lore.kernel.org/r/20230728-solid-fill-v5-0-053dbefa9...@quicinc.com Changes in v5: - Added support for PIXEL_SOURCE_NONE (Sebastian) - Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't set (Dmitry) - Added debugfs support for both properties (Dmitry) - Corrected u32 to u8 conversion (Pekka) - Moved drm_solid_fill_info struct and related documentation to include/uapi (Pekka) - Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka) - Added more detailed UAPI and kernel documentation (Pekka) - Reordered patch series so that the pixel_source property is introduced before solid_fill (Dmitry) - Fixed inconsistent ABGR/RGBA format declaration (Pekka) - Reset pixel_source to FB in drm_mode_setplane() (Dmitry) - Rename supported_sources to extra_sources (Dmitry) - Only destroy old solid_fill blob state if new state is valid (Pekka) - Link to v4: https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa7...@quicinc.com 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) - Introduce
[PATCH RFC v7 09/10] 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.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 38 ---
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 9615653db787..832747080daf 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
/*
@@ -84,7 +83,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;
@@ -640,19 +638,34 @@ 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_abgr_fill_color(struct drm_plane_state *state)
+{
+ struct drm_solid_fill solid_fill = state->solid_fill;
+
+ uint32_t ret = 0;
+ uint8_t a = state->alpha & 0xFF;
+ uint8_t b = solid_fill.b >> 24;
+ uint8_t g = solid_fill.g >> 24;
+ uint8_t r = solid_fill.r >> 24;
+
+ ret |= a << 24;
+ ret |= b << 16;
+ ret |= g << 8;
+ ret |= r;
+
+ return ret;
+}
+
/**
* _dpu_plane_color_fill - enables color fill on plane
* @pdpu: Pointer to DPU plane object
* @color: RGB fill color value, [23..16] Blue, [15..8] Green, [7..0] Red
- * @alpha: 8-bit fill alpha value, 255 selects 100% alpha
*/
-static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
- uint32_t color, uint32_t alpha)
+static void _dpu_plane_color_fill(struct dpu_plane *pdpu, uint32_t color)
{
const struct dpu_format *fmt;
const struct drm_plane *plane = &pdpu->base;
struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
- u32 fill_color = (color & 0xFF) | ((alpha & 0xFF) << 24);
DPU_DEBUG_PLANE(pdpu, "\n");
@@ -667,11 +680,11 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
/* update sspp */
_dpu_plane_color_fill_pipe(pstate, &pstate->pipe,
&pstate->pipe_cfg.dst_rect,
- fill_color, fmt);
+ color, fmt);
if (pstate->r_pipe.sspp)
_dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
&pstate->r_pipe_cfg.dst_rect,
- fill_color, fmt);
+ color, fmt);
}
static int dpu_plane_prepare_fb(struct drm_plane *plane,
@@ -1019,10 +1032,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);
+ _dpu_plane_color_fill(pdpu, 0x);
+ else if (drm_plane_solid_fill_enabled(plane->state))
+ _dpu_plane_color_fill(pdpu,
_dpu_plane_get_abgr_fill_color(plane->state));
else {
dpu_plane_flush_csc(pdpu, &pstate->pipe);
dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
@@ -1067,7 +1079,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.42.0
[PATCH RFC v7 05/10] drm/atomic: Add solid fill data to plane state dump
Add solid_fill property data to the atomic plane state dump.
Reviewed-by: Dmitry Baryshkov
Acked-by: Harry Wentland
Acked-by: Sebastian Wick
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c | 4
drivers/gpu/drm/drm_plane.c | 8
include/drm/drm_plane.h | 3 +++
3 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9f9abbe76369..af778d32785b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -726,6 +726,10 @@ static void drm_atomic_plane_print_state(struct
drm_printer *p,
drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
if (state->fb)
drm_framebuffer_print_info(p, 2, state->fb);
+ drm_printf(p, "\tsolid_fill=%u\n",
+ state->solid_fill_blob ?
state->solid_fill_blob->base.id : 0);
+ if (state->solid_fill_blob)
+ drm_plane_solid_fill_print_info(p, 2, state);
drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src));
drm_printf(p, "\trotation=%x\n", state->rotation);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 559d101162ba..289b3be86d52 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1495,6 +1495,14 @@ __drm_plane_get_damage_clips(const struct
drm_plane_state *state)
state->fb_damage_clips->data : NULL);
}
+void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned int
indent,
+const struct drm_plane_state *state)
+{
+ drm_printf_indent(p, indent, "r=0x%08x\n", state->solid_fill.r);
+ drm_printf_indent(p, indent, "g=0x%08x\n", state->solid_fill.g);
+ drm_printf_indent(p, indent, "b=0x%08x\n", state->solid_fill.b);
+}
+
/**
* drm_plane_get_damage_clips - Returns damage clips.
* @state: Plane state.
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index baaf737392bc..6171fb1a0b47 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -1001,6 +1001,9 @@ drm_plane_get_damage_clips_count(const struct
drm_plane_state *state);
struct drm_mode_rect *
drm_plane_get_damage_clips(const struct drm_plane_state *state);
+void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned int
indent,
+const struct drm_plane_state *state);
+
int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
unsigned int supported_filters);
--
2.42.0
[PATCH RFC v7 03/10] drm: Add solid fill pixel source
Add "SOLID_FILL" as a valid pixel source. If the pixel_source property is
set to "SOLID_FILL", it will display data from the drm_plane "solid_fill"
blob property.
Reviewed-by: Dmitry Baryshkov
Acked-by: Pekka Paalanen
Acked-by: Harry Wentland
Acked-by: Sebastian Wick
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_blend.c | 10 +-
include/drm/drm_plane.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 665c5d9b056f..37b31b7e5ce5 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -204,6 +204,9 @@
* "FB":
* Framebuffer source set by the "FB_ID" property.
*
+ * "SOLID_FILL":
+ * Solid fill color source set by the "solid_fill" property.
+ *
* 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.
@@ -642,6 +645,7 @@ EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
static const struct drm_prop_enum_list drm_pixel_source_enum_list[] = {
{ DRM_PLANE_PIXEL_SOURCE_NONE, "NONE" },
{ DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
+ { DRM_PLANE_PIXEL_SOURCE_SOLID_FILL, "SOLID_FILL" },
};
/**
@@ -666,6 +670,9 @@ static const struct drm_prop_enum_list
drm_pixel_source_enum_list[] = {
* "FB":
* Framebuffer pixel source
*
+ * "SOLID_FILL":
+ * Solid fill color pixel source
+ *
* Returns:
* Zero on success, negative errno on failure.
*/
@@ -675,7 +682,8 @@ int drm_plane_create_pixel_source_property(struct drm_plane
*plane,
struct drm_device *dev = plane->dev;
struct drm_property *prop;
static const unsigned int valid_source_mask =
BIT(DRM_PLANE_PIXEL_SOURCE_FB) |
-
BIT(DRM_PLANE_PIXEL_SOURCE_NONE);
+
BIT(DRM_PLANE_PIXEL_SOURCE_NONE) |
+
BIT(DRM_PLANE_PIXEL_SOURCE_SOLID_FILL);
int i;
/* FB is supported by default */
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index e2ae7c26cc57..baaf737392bc 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -43,6 +43,7 @@ enum drm_scaling_filter {
enum drm_plane_pixel_source {
DRM_PLANE_PIXEL_SOURCE_NONE,
DRM_PLANE_PIXEL_SOURCE_FB,
+ DRM_PLANE_PIXEL_SOURCE_SOLID_FILL,
DRM_PLANE_PIXEL_SOURCE_MAX
};
--
2.42.0
[PATCH RFC v7 07/10] drm/atomic: Loosen FB atomic checks
Loosen the requirements for atomic and legacy commit so that, in cases
where pixel_source != FB, the commit can still go through.
This includes adding framebuffer NULL checks in other areas to account for
FB being NULL when non-FB pixel sources are enabled.
To disable a plane, the pixel_source must be NONE or the FB must be NULL
if pixel_source == FB.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c| 21 ++--
drivers/gpu/drm/drm_atomic_helper.c | 39 +
include/drm/drm_atomic_helper.h | 4 ++--
include/drm/drm_plane.h | 29 +++
4 files changed, 64 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 48a2df4e3d27..66e49c06aced 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -674,17 +674,16 @@ 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;
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;
}
@@ -715,9 +714,11 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
}
- ret = drm_atomic_plane_check_fb(new_plane_state);
- if (ret)
- return ret;
+ if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB &&
new_plane_state->fb) {
+ ret = drm_atomic_plane_check_fb(new_plane_state);
+ if (ret)
+ return ret;
+ }
if (plane_switching_crtc(old_plane_state, new_plane_state)) {
drm_dbg_atomic(plane->dev,
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 292e38eb6218..27e8c8dd9324 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -852,7 +852,6 @@ int drm_atomic_helper_check_plane_state(struct
drm_plane_state *plane_state,
bool can_position,
bool can_update_disabled)
{
- struct drm_framebuffer *fb = plane_state->fb;
struct drm_rect *src = &plane_state->src;
struct drm_rect *dst = &plane_state->dst;
unsigned int rotation = plane_state->rotation;
@@ -864,7 +863,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 +880,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);
+ /* Check that selected pixel source is valid */
+ if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB &&
plane_state->fb) {
+ struct drm_framebuffer *fb = plane_state->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 (crtc_state->enabl
[PATCH RFC v7 01/10] drm: Introduce pixel_source DRM plane property
Add support for pixel_source property to drm_plane and related
documentation. In addition, force pixel_source to
DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
legacy userspace.
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.
Currently, the only pixel sources are DRM_PLANE_PIXEL_SOURCE_FB (the
default value) and DRM_PLANE_PIXEL_SOURCE_NONE.
Acked-by: Dmitry Baryshkov
Acked-by: Pekka Paalanen
Acked-by: Harry Wentland
Acked-by: Sebastian Wick
Acked-by: Simon Ser
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 | 94 +++
drivers/gpu/drm/drm_plane.c | 19 +--
include/drm/drm_blend.h | 2 +
include/drm/drm_plane.h | 21 +++
6 files changed, 137 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..01638c51ce0a 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->color_encoding_property) {
if (!drm_object_property_get_default_value(&plane->base,
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 98d3b10c08ae..46c78b87803d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -562,6 +562,8 @@ 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->pixel_source_property) {
+ state->pixel_source = val;
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
@@ -634,6 +636,8 @@ 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->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 6e74de833466..fce734cdb85b 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -185,6 +185,25 @@
* plane does not expose the "alpha" property, then this is
* assumed to be 1.0
*
+ * pixel_source:
+ * pixel_source is set up with drm_plane_create_pixel_source_property().
+ * It is used to toggle the active source of pixel data for the plane.
+ * The plane will only display data from the set pixel_source -- any
+ * data from other sources will be ignored.
+ *
+ * For non-framebuffer sources, if pixel_source is set to a non-framebuffer
+ * and non-NONE source, and the corresponding source property is NULL, then
+ * the atomic commit should return an error.
+ *
+ * Possible values:
+ *
+ * "NONE":
+ * No active pixel source.
+ * Committing with a NONE pixel source will disable the plane.
+ *
+ * "FB":
+ * Framebuffer source set by the "FB_ID" property.
+ *
* 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 +634,78 @@ int drm_plane_create_blend_mode_property(struct drm_plane
*plane,
return 0;
}
EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+static const struct drm_prop_enum_list drm_pixel_source_enum_list[] = {
+ { DRM_PLANE_PIXEL_SOURCE_NONE, "NONE" },
+ { DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
+};
+
+/**
+ * drm_plane_create_pixel_source_property - create a new pixel source property
+ * @plane: DRM plane
+ * @extra_sources: Bitmask of additional supported pixel_sources for the
driver.
+ *DRM_PLANE_PIXEL_SOURCE_FB and DRM_PLANE_PIXEL_SOURCE_NONE
will
+ *always be enabled as supported sources.
+ *
+ * This creates a new property describing the current source of pixel data for
the
+ * plane. The pixel_source will be initialized as DRM_PLANE_PIXEL_SOURCE_FB by
default.
+ *
+ * Drivers can set a custom default source by overriding the pixel_sour
[PATCH RFC v7 02/10] 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_mode_solid_fill {
u32 r, g, b, pad;
};
Acked-by: Harry Wentland
Acked-by: Sebastian Wick
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_state_helper.c | 9
drivers/gpu/drm/drm_atomic_uapi.c | 26 ++
drivers/gpu/drm/drm_blend.c | 30 ++
include/drm/drm_blend.h | 1 +
include/drm/drm_plane.h | 36 +++
include/uapi/drm/drm_mode.h | 24 +
6 files changed, 126 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 01638c51ce0a..86fb876efbe6 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
drm_plane_state *plane_state,
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);
+ plane_state->solid_fill_blob = NULL;
+ }
+
if (plane->color_encoding_property) {
if (!drm_object_property_get_default_value(&plane->base,
plane->color_encoding_property,
@@ -336,6 +341,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;
@@ -385,6 +393,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 46c78b87803d..576acb34195f 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -316,6 +316,20 @@ drm_atomic_set_crtc_for_connector(struct
drm_connector_state *conn_state,
}
EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
+static void drm_atomic_set_solid_fill_prop(struct drm_plane_state *state)
+{
+ struct drm_mode_solid_fill *user_info;
+
+ if (!state->solid_fill_blob)
+ return;
+
+ user_info = (struct drm_mode_solid_fill *)state->solid_fill_blob->data;
+
+ state->solid_fill.r = user_info->r;
+ state->solid_fill.g = user_info->g;
+ state->solid_fill.b = user_info->b;
+}
+
static void set_out_fence_for_crtc(struct drm_atomic_state *state,
struct drm_crtc *crtc, s32 __user *fence_ptr)
{
@@ -564,6 +578,15 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
state->src_h = val;
} else if (property == plane->pixel_source_property) {
state->pixel_source = val;
+ } else if (property == plane->solid_fill_property) {
+ ret = drm_atomic_replace_property_blob_from_id(dev,
+ &state->solid_fill_blob,
+ val, sizeof(struct drm_mode_solid_fill),
+ -1, &replaced);
+ if (ret)
+ return ret;
+
+ drm_atomic_set_solid_fill_prop(state);
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
@@ -638,6 +661,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_h;
} else if (property == plane->pixel_source_property) {
*val = state->pixel_source;
+ } 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 fce734cdb85b..665c5d9b056f 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -204,6 +204,10 @@
* "FB":
* Framebuffer source set by the "FB_ID" property.
*
+ * solid_fill:
+ * solid_fill is
[PATCH RFC v7 06/10] 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.
Reviewed-by: Dmitry Baryshkov
Acked-by: Harry Wentland
Acked-by: Sebastian Wick
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c | 130 ---
1 file changed, 73 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index af778d32785b..48a2df4e3d27 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -589,6 +589,76 @@ plane_switching_crtc(const struct drm_plane_state
*old_plane_state,
return true;
}
+static int drm_atomic_plane_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
@@ -605,9 +675,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 */
@@ -634,17 +701,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_pl
