Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane
On 1/18/23 17:53, Jessica Zhang wrote:
>
>
> On 1/18/2023 10:57 AM, Harry Wentland wrote:
>> On 1/4/23 18:40, Jessica Zhang wrote:
>>> Add support for solid_fill property to drm_plane. In addition, add
>>> support for setting and getting the values for solid_fill.
>>>
>>> solid_fill holds data for supporting solid fill planes. The property
>>> accepts an RGB323232 value and the driver data is formatted as such:
>>>
>>> struct drm_solid_fill {
>>> u32 r;
>>> u32 g;
>>> u32 b;
>>> };
>>
>> Rather than special-casing this would it make sense to define this
>> as a single pixel of a FOURCC property?
>>
>> I.e., something like this:
>>
>> struct drm_solid_fill_info {
>> u32 format; /* FOURCC value */
>> u64 value; /* FOURCC pixel value */
>> }
>>
>> That removes some ambiguity how the value should be interpreted, i.e.,
>> it can be interpreted like a single pixel of the specified FOURCC format.
>>
>> It might also make sense to let drivers advertise the supported
>> FOURCC formats for solid_fill planes.
>
> Hi Harry,
>
> The initial v1 of this RFC had support for taking in a format and such, but
> it was decided that just supporting RGB323232 would work too.
>
> Here's the original thread discussing it [1], but to summarize, the work
> needed to convert the solid fill color to RGB is trivial (as it's just a
> single pixel of data). In addition, supporting various formats for solid_fill
> would add complexity as we'd have to communicate which formats are supported.
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html
>
Make sense, and thanks for summarizing.
The only comment I would have left then, is that maybe it'd be good to add
an alpha value. I think it was suggested by someone else as well.
>>
>> Is there an implementation for this in a corresponding canonical
>> upstream userspace project, to satisfy [1]? If not, what is the plan
>> for this? If so, please point to the corresponding patches.
>
> The use case we're trying to support here is the Android HWC SOLID_FILL hint
> [1], though it can also be used to address the Wayland single pixel FB
> protocol [2]. I'm also planning to add an IGT test to show an example of end
> to end usage.
>
> [1]
> https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
>
> [2]
> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>
Makes sense.
Harry
> Thanks,
>
> Jessica Zhang
>
>>
>> [1]
>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>
>> Harry
>>
>>>
>>> To enable solid fill planes, userspace must assigned solid_fill to a
>>> property blob containing the following information:
>>>
>>> struct drm_solid_fill_info {
>>> u8 version;
>>> u32 r, g, b;
>>> };
>>>
>>> Changes in V2:
>>> - Changed solid_fill property to a property blob (Simon, Dmitry)
>>> - Added drm_solid_fill struct (Simon)
>>> - Added drm_solid_fill_info struct (Simon)
>>>
>>> Changes in V3:
>>> - Corrected typo in drm_solid_fill struct documentation
>>>
>>> Signed-off-by: Jessica Zhang
>>> ---
>>> drivers/gpu/drm/drm_atomic_state_helper.c | 9
>>> drivers/gpu/drm/drm_atomic_uapi.c | 59 +++
>>> drivers/gpu/drm/drm_blend.c | 17 +++
>>> include/drm/drm_blend.h | 1 +
>>> include/drm/drm_plane.h | 43 +
>>> 5 files changed, 129 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
>>> b/drivers/gpu/drm/drm_atomic_state_helper.c
>>> index dfb57217253b..c96fd1f2ad99 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_bl
Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane
On 1/19/2023 7:57 AM, Harry Wentland wrote:
On 1/18/23 17:53, Jessica Zhang wrote:
On 1/18/2023 10:57 AM, Harry Wentland wrote:
On 1/4/23 18:40, Jessica Zhang wrote:
Add support for solid_fill property to drm_plane. In addition, add
support for setting and getting the values for solid_fill.
solid_fill holds data for supporting solid fill planes. The property
accepts an RGB323232 value and the driver data is formatted as such:
struct drm_solid_fill {
u32 r;
u32 g;
u32 b;
};
Rather than special-casing this would it make sense to define this
as a single pixel of a FOURCC property?
I.e., something like this:
struct drm_solid_fill_info {
u32 format; /* FOURCC value */
u64 value; /* FOURCC pixel value */
}
That removes some ambiguity how the value should be interpreted, i.e.,
it can be interpreted like a single pixel of the specified FOURCC format.
It might also make sense to let drivers advertise the supported
FOURCC formats for solid_fill planes.
Hi Harry,
The initial v1 of this RFC had support for taking in a format and such, but it
was decided that just supporting RGB323232 would work too.
Here's the original thread discussing it [1], but to summarize, the work needed
to convert the solid fill color to RGB is trivial (as it's just a single pixel
of data). In addition, supporting various formats for solid_fill would add
complexity as we'd have to communicate which formats are supported.
[1] https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html
Make sense, and thanks for summarizing.
The only comment I would have left then, is that maybe it'd be good to add
an alpha value. I think it was suggested by someone else as well.
Yep it was mentioned in the v1 discussion, but we came to the conclusion
that user can set the alpha through the separate alpha plane property.
Thanks,
Jessica Zhang
Is there an implementation for this in a corresponding canonical
upstream userspace project, to satisfy [1]? If not, what is the plan
for this? If so, please point to the corresponding patches.
The use case we're trying to support here is the Android HWC SOLID_FILL hint
[1], though it can also be used to address the Wayland single pixel FB protocol
[2]. I'm also planning to add an IGT test to show an example of end to end
usage.
[1]
https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
[2]
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
Makes sense.
Harry
Thanks,
Jessica Zhang
[1]
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
Harry
To enable solid fill planes, userspace must assigned solid_fill to a
property blob containing the following information:
struct drm_solid_fill_info {
u8 version;
u32 r, g, b;
};
Changes in V2:
- Changed solid_fill property to a property blob (Simon, Dmitry)
- Added drm_solid_fill struct (Simon)
- Added drm_solid_fill_info struct (Simon)
Changes in V3:
- Corrected typo in drm_solid_fill struct documentation
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_state_helper.c | 9
drivers/gpu/drm/drm_atomic_uapi.c | 59 +++
drivers/gpu/drm/drm_blend.c | 17 +++
include/drm/drm_blend.h | 1 +
include/drm/drm_plane.h | 43 +
5 files changed, 129 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index dfb57217253b..c96fd1f2ad99 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
Re: [RFC PATCH v3 1/3] drm: Introduce solid fill property for drm plane
On 1/19/23 11:24, Jessica Zhang wrote:
>
>
> On 1/19/2023 7:57 AM, Harry Wentland wrote:
>>
>>
>> On 1/18/23 17:53, Jessica Zhang wrote:
>>>
>>>
>>> On 1/18/2023 10:57 AM, Harry Wentland wrote:
On 1/4/23 18:40, Jessica Zhang wrote:
> Add support for solid_fill property to drm_plane. In addition, add
> support for setting and getting the values for solid_fill.
>
> solid_fill holds data for supporting solid fill planes. The property
> accepts an RGB323232 value and the driver data is formatted as such:
>
> struct drm_solid_fill {
> u32 r;
> u32 g;
> u32 b;
> };
Rather than special-casing this would it make sense to define this
as a single pixel of a FOURCC property?
I.e., something like this:
struct drm_solid_fill_info {
u32 format; /* FOURCC value */
u64 value; /* FOURCC pixel value */
}
That removes some ambiguity how the value should be interpreted, i.e.,
it can be interpreted like a single pixel of the specified FOURCC format.
It might also make sense to let drivers advertise the supported
FOURCC formats for solid_fill planes.
>>>
>>> Hi Harry,
>>>
>>> The initial v1 of this RFC had support for taking in a format and such, but
>>> it was decided that just supporting RGB323232 would work too.
>>>
>>> Here's the original thread discussing it [1], but to summarize, the work
>>> needed to convert the solid fill color to RGB is trivial (as it's just a
>>> single pixel of data). In addition, supporting various formats for
>>> solid_fill would add complexity as we'd have to communicate which formats
>>> are supported.
>>>
>>> [1]
>>> https://lists.freedesktop.org/archives/dri-devel/2022-November/379148.html
>>>
>>
>> Make sense, and thanks for summarizing.
>>
>> The only comment I would have left then, is that maybe it'd be good to add
>> an alpha value. I think it was suggested by someone else as well.
>
> Yep it was mentioned in the v1 discussion, but we came to the conclusion that
> user can set the alpha through the separate alpha plane property.
>
Got it.
Harry
> Thanks,
>
> Jessica Zhang
>
>>
Is there an implementation for this in a corresponding canonical
upstream userspace project, to satisfy [1]? If not, what is the plan
for this? If so, please point to the corresponding patches.
>>>
>>> The use case we're trying to support here is the Android HWC SOLID_FILL
>>> hint [1], though it can also be used to address the Wayland single pixel FB
>>> protocol [2]. I'm also planning to add an IGT test to show an example of
>>> end to end usage.
>>>
>>> [1]
>>> https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
>>>
>>> [2]
>>> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>>>
>>
>> Makes sense.
>>
>> Harry
>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
[1]
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
Harry
>
> To enable solid fill planes, userspace must assigned solid_fill to a
> property blob containing the following information:
>
> struct drm_solid_fill_info {
> u8 version;
> u32 r, g, b;
> };
>
> Changes in V2:
> - Changed solid_fill property to a property blob (Simon, Dmitry)
> - Added drm_solid_fill struct (Simon)
> - Added drm_solid_fill_info struct (Simon)
>
> Changes in V3:
> - Corrected typo in drm_solid_fill struct documentation
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 9
> drivers/gpu/drm/drm_atomic_uapi.c | 59 +++
> drivers/gpu/drm/drm_blend.c | 17 +++
> include/drm/drm_blend.h | 1 +
> include/drm/drm_plane.h | 43 +
> 5 files changed, 129 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index dfb57217253b..c96fd1f2ad99 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_
