[Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes

[Jessica Zhang]

On 6/26/2023 5:06 PM, Dmitry Baryshkov wrote:
> On 27/06/2023 02:02, 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.
> 
> Why? The userspace should use atomic commits and as such it should all 
> properties at the same time.

Correct, userspace will set all the properties within the same atomic 
commit. The issue happens when the driver iterates through each property 
within the MODE_ATOMIC ioctl [1].

For reference, I'm thinking of an implementation where we're setting the 
pixel_source within drm_atomic_plane_set_property().

So something like:

drm_atomic_plane_set_property( ... )
{
     if (property == config->prop_fb_id) {
         if (fb)
             state->pixel_source = FB;
         else
             state->pixel_source = NONE;
     } else if (property == config->prop_solid_fill) {
         if (solid_fill_blob)
             state->pixel_source = SOLID_FILL;
     }

     // ...
}

If userspace sets solid_fill to a valid blob and FB to NULL, it's 
possible for driver to first set the solid_fill property then set the 
fb_id property later. This would cause pixel_source to be set to NONE 
after all properties have been set.

I've also considered an implementation without the `pixel_source = NONE` 
line in the prop_fb_id case, but we would still need to find somewhere 
to set the pixel_source to NONE in order to allow userspace to disable a 
plane.

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_uapi.c#L1385

> 
>> 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.
>>
>> Would be interested in any feedback on this.
> 
> This is an interesting idea. Frankly speaking, I'd consider it 
> counter-intuitive at the first glance.
> 
> Consider it to act as a curtain. Setup the curtain (by writing the fill 
> colour property). Then one can close the curtain (by switching source to 
> colour), or open it (by switching to any other source). Bitmask wouldn't 
> complicate this.

So if I'm understanding your analogy correctly, pixel_source won't 
necessarily be set whenever the FB or solid_fill properties are set. So 
that way we can have both FB *and* solid_fill set at the same time, but 
only the source that pixel_source is set to would be displayed.

Thanks,

Jessica Zhang

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> In addition, loosen the NULL FB checks within the atomic commit 
>>>> callstack
>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>> methods where the FB was previously assumed to be non-NULL.
>>>>
>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>> drm_plane_state.color_fill_format instead of 
>>>> dpu_plane_state.color_fill,
>>>> and add extra checks in the DPU atomic commit callstack to account 
>>>> for a
>>>> NULL FB in cases where color_fill is set.
>>>>
>>>> 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.
>>>>
>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT 
>>>> to a
>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>> framebuffer to NULL.
>>>
>>> Is there some real reason for the format property? Ie. why not
>>> just do what was the plan for the crttc background color and
>>> specify the color in full 16bpc format and just pick as many
>>> msbs from that as the hw can use?
>>>
>>>>
>>>> Jessica Zhang (3):
>>>>    drm: Introduce color fill properties for drm plane
>>>>    drm: Adjust atomic checks for solid fill color
>>>>    drm/msm/dpu: Use color_fill property for DPU planes
>>>>
>>>>   drivers/gpu/drm/drm_atomic.c              | 68 
>>>> ++++++++++++-----------
>>>>   drivers/gpu/drm/drm_atomic_helper.c       | 34 +++++++-----
>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  8 +++
>>>>   drivers/gpu/drm/drm_blend.c               | 38 +++++++++++++
>>>>   drivers/gpu/drm/drm_plane.c               |  8 +--
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  7 ++-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>>>>   include/drm/drm_atomic_helper.h           |  5 +-
>>>>   include/drm/drm_blend.h                   |  2 +
>>>>   include/drm/drm_plane.h                   | 28 ++++++++++
>>>>   10 files changed, 188 insertions(+), 76 deletions(-)
>>>>
>>>> -- 
>>>> 2.38.0
>>>
>>> -- 
>>> Ville Syrjälä
>>> Intel
> 
> -- 
> With best wishes
> Dmitry
>