From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Jessica Zhang <quic_jesszhan@quicinc.com>,
Pekka Paalanen <ppaalanen@gmail.com>
Cc: daniel.vetter@ffwll.ch, quic_abhinavk@quicinc.com,
dri-devel@lists.freedesktop.org, swboyd@chromium.org,
seanpaul@chromium.org, laurent.pinchart@ideasonboard.com,
linux-arm-msm@vger.kernel.org,
wayland-devel@lists.freedesktop.org,
freedreno@lists.freedesktop.org
Subject: Re: [RFC PATCH 0/3] Support for Solid Fill Planes
Date: Wed, 28 Jun 2023 00:59:30 +0300 [thread overview]
Message-ID: <54f194fe-ab7b-247d-600b-6da8f5c57dbf@linaro.org> (raw)
In-Reply-To: <5e60fe99-76d5-c242-608e-b74bf6f0e7bd@quicinc.com>
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 <quic_jesszhan@quicinc.com> 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.
> +-----------------+-------------------------+-------------------------+
> | NULL commit | pixel_source = FB | pixel_source = FB |
> | | FB_ID = NULL | FB_ID = NULL |
> +-----------------+-------------------------+-------------------------+
| or:
| pixel_source = COLOR
| solid_fill = NULL
>
> Is there anything I'm missing or needs to be clarified?
>
LGTM otherwise
> Thanks,
>
> Jessica Zhang
>
>>
>>
>> Thanks,
>> pq
>>
>>
>>>
>>> Would be interested in any feedback on this.
>>>
>>> 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
next prev parent reply other threads:[~2023-06-27 21:59 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-28 22:59 [RFC PATCH 0/3] Support for Solid Fill Planes Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
2022-10-29 11:23 ` Dmitry Baryshkov
2022-10-31 21:58 ` Jessica Zhang
2022-11-08 18:25 ` Simon Ser
2022-11-09 13:52 ` Daniel Vetter
2022-11-09 13:53 ` Dmitry Baryshkov
2022-11-09 13:59 ` Daniel Vetter
2022-11-10 1:44 ` Jessica Zhang
2022-11-11 9:45 ` Daniel Vetter
2022-11-18 19:15 ` Jessica Zhang
2022-10-29 12:08 ` Dmitry Baryshkov
2022-10-31 22:24 ` Jessica Zhang
2022-11-01 0:11 ` Dmitry Baryshkov
2022-11-01 17:35 ` Jessica Zhang
2022-11-08 18:50 ` Simon Ser
2022-11-08 22:01 ` Sebastian Wick
2022-11-09 9:18 ` Pekka Paalanen
2022-11-23 23:27 ` Jessica Zhang
2022-11-24 8:50 ` Pekka Paalanen
2022-11-29 18:53 ` [Freedreno] " Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color Jessica Zhang
2022-10-29 11:38 ` Dmitry Baryshkov
2022-10-31 20:41 ` Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes Jessica Zhang
2022-10-29 11:40 ` Dmitry Baryshkov
2022-10-31 22:14 ` Jessica Zhang
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
2022-11-07 21:32 ` Jessica Zhang
2022-11-07 22:09 ` [Freedreno] " Rob Clark
2022-11-08 0:22 ` Jessica Zhang
2022-11-08 3:34 ` Rob Clark
2022-11-08 8:52 ` Ville Syrjälä
2022-11-22 23:20 ` Jessica Zhang
2022-11-07 23:06 ` Laurent Pinchart
2023-06-26 23:02 ` Jessica Zhang
2023-06-27 0:06 ` Dmitry Baryshkov
2023-06-27 0:45 ` Jessica Zhang
2023-06-27 1:46 ` Dmitry Baryshkov
2023-06-27 7:58 ` Pekka Paalanen
2023-06-27 21:27 ` Jessica Zhang
2023-06-27 21:59 ` Dmitry Baryshkov [this message]
2023-06-27 22:10 ` Abhinav Kumar
2023-06-28 7:34 ` Pekka Paalanen
2023-06-28 16:40 ` Jessica Zhang
2023-06-29 7:29 ` Pekka Paalanen
2023-06-29 18:53 ` [Freedreno] " Jessica Zhang
2023-06-29 18:52 ` Jessica Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54f194fe-ab7b-247d-600b-6da8f5c57dbf@linaro.org \
--to=dmitry.baryshkov@linaro.org \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=ppaalanen@gmail.com \
--cc=quic_abhinavk@quicinc.com \
--cc=quic_jesszhan@quicinc.com \
--cc=seanpaul@chromium.org \
--cc=swboyd@chromium.org \
--cc=wayland-devel@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).