[PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Jul 11 22:42:28 UTC 2023
On 7/11/2023 3:19 PM, Dmitry Baryshkov wrote:
> On 12/07/2023 01:07, Jessica Zhang wrote:
>>
>>
>> On 7/10/2023 1:11 PM, Dmitry Baryshkov wrote:
>>> On 10/07/2023 22:51, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
>>>>> On Fri, 30 Jun 2023 03:42:28 +0300
>>>>> Dmitry Baryshkov <dmitry.baryshkov at linaro.org> wrote:
>>>>>
>>>>>> On 30/06/2023 03:25, Jessica Zhang wrote:
>>>>>>> Add support for pixel_source property to drm_plane and related
>>>>>>> documentation.
>>>>>>>
>>>>>>> This enum property will allow user to specify a pixel source for the
>>>>>>> plane. Possible pixel sources will be defined in the
>>>>>>> drm_plane_pixel_source enum.
>>>>>>>
>>>>>>> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
>>>>>>> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
>>>>>>
>>>>>> I think, this should come before the solid fill property addition.
>>>>>> First
>>>>>> you tell that there is a possibility to define other pixel
>>>>>> sources, then
>>>>>> additional sources are defined.
>>>>>
>>>>> Hi,
>>>>>
>>>>> that would be logical indeed.
>>>>
>>>> Hi Dmitry and Pekka,
>>>>
>>>> Sorry for the delay in response, was out of office last week.
>>>>
>>>> Acked.
>>>>
>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
>>>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
>>>>>>> drivers/gpu/drm/drm_blend.c | 81
>>>>>>> +++++++++++++++++++++++++++++++
>>>>>>> include/drm/drm_blend.h | 2 +
>>>>>>> include/drm/drm_plane.h | 21 ++++++++
>>>>>>> 5 files changed, 109 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>>> b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>>> index fe14be2bd2b2..86fb876efbe6 100644
>>>>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>>> @@ -252,6 +252,7 @@ void
>>>>>>> __drm_atomic_helper_plane_state_reset(struct drm_plane_state
>>>>>>> *plane_state,
>>>>>>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>>>>>>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>>>>>> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
>>>>>>> if (plane_state->solid_fill_blob) {
>>>>>>> drm_property_blob_put(plane_state->solid_fill_blob);
>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>>>>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
>>>>>>> index a28b4ee79444..6e59c21af66b 100644
>>>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>>>>> @@ -596,6 +596,8 @@ static int
>>>>>>> drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>>>> drm_property_blob_put(solid_fill);
>>>>>>> return ret;
>>>>>>> + } else if (property == plane->pixel_source_property) {
>>>>>>> + state->pixel_source = val;
>>>>>>> } else if (property == plane->alpha_property) {
>>>>>>> state->alpha = val;
>>>>>>> } else if (property == plane->blend_mode_property) {
>>>>>>
>>>>>> I think, it was pointed out in the discussion that
>>>>>> drm_mode_setplane()
>>>>>> (a pre-atomic IOCTL to turn the plane on and off) should also reset
>>>>>> pixel_source to FB.
>>>>
>>>> I don't remember drm_mode_setplane() being mentioned in the
>>>> pixel_source discussion... can you share where it was mentioned?
>>>
>>> https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/
>>>
>>> Let me quote it here:
>>> "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."
>>>
>>>
>>>>
>>>> I'd prefer to avoid having driver change the pixel_source directly
>>>> as it could cause some unexpected side effects. In general, I would
>>>> like userspace to assign the value of pixel_source without driver
>>>> doing anything "under the hood".
>>>
>>> s/driver/drm core/
>>>
>>> We have to remain compatible with old userspace, especially with the
>>> non-atomic one. If the userspace calls
>>> ioctl(DRM_IOCTL_MODE_SETPLANE), we have to display the specified FB,
>>> no matter what was the value of PIXEL_SOURCE before this ioctl.
>>
>>
>> Got it, thanks the clarification -- I see your point.
>>
>> I'm already setting plane_state->pixel_source to FB in
>> __drm_atomic_helper_plane_reset() and it seems to me that all drivers
>> are calling that within their respective plane_funcs->reset().
>>
>> Since (as far as I know) reset() is being called for both the atomic
>> and non-atomic paths, shouldn't that be enough to default pixel_source
>> to FB for old userspace?
>
> No, this will not clean up the state between userspace apps. Currently
> the rule is simple: call DRM_IOCTL_MODE_SETPLANE, get the image from FB
> displayed. We should keep it so.
>
Ok, so you are considering a use-case where we bootup with a userspace
(which is aware of pixel_source), that one uses the pixel_source to
switch the property to solid_color and then we kill this userspace and
bootup one which is unaware of this property and uses
DRM_IOCTL_MODE_SETPLANE, then we should default back to FB.
>>>>
>>>>>>
>>>>>>> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct
>>>>>>> drm_plane *plane,
>>>>>>> } else if (property == plane->solid_fill_property) {
>>>>>>> *val =state->solid_fill_blob ?
>>>>>>> state->solid_fill_blob->base.id : 0;
>>>>>>> + } else if (property == plane->pixel_source_property) {
>>>>>>> + *val = state->pixel_source;
>>>>>>> } else if (property == plane->alpha_property) {
>>>>>>> *val =state->alpha;
>>>>>>> } else if (property == plane->blend_mode_property) {
>>>>>>> diff --git a/drivers/gpu/drm/drm_blend.c
>>>>>>> b/drivers/gpu/drm/drm_blend.c
>>>>>>> index 38c3c5d6453a..8c100a957ee2 100644
>>>>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>>>>> @@ -189,6 +189,18 @@
>>>>>>> * solid_fill is set up with
>>>>>>> drm_plane_create_solid_fill_property(). It
>>>>>>> * contains pixel data that drivers can use to fill a plane.
>>>>>>> *
>>>>>>> + * pixel_source:
>>>>>>> + * pixel_source is set up with
>>>>>>> drm_plane_create_pixel_source_property().
>>>>>>> + * It is used to toggle the source of pixel data for the plane.
>>>>>
>>>>> Other sources than the selected one are ignored?
>>>>
>>>> Yep, the plane will only display the data from the set pixel_source.
>>>>
>>>> So if pixel_source == FB and solid_fill_blob is non-NULL,
>>>> solid_fill_blob will be ignored and the plane will display the FB
>>>> that is set.
>>>
>>> correct.
>>>
>>>>
>>>> Will add a note about this in the comment docs.
>>>>
>>>>>
>>>>>>> + *
>>>>>>> + * Possible values:
>>>>>
>>>>> Wouldn't hurt to explicitly mention here that this is an enum.
>>>>
>>>> Acked.
>>>>
>>>>>
>>>>>>> + *
>>>>>>> + * "FB":
>>>>>>> + * Framebuffer source
>>>>>>> + *
>>>>>>> + * "COLOR":
>>>>>>> + * solid_fill source
>>>>>
>>>>> I think these two should be more explicit. Framebuffer source is the
>>>>> usual source from the property "FB_ID". Solid fill source comes from
>>>>> the property "solid_fill".
>>>>
>>>> Acked.
>>>>
>>>>>
>>>>> Why "COLOR" and not, say, "SOLID_FILL"?
>>>>
>>>> Ah, that would make more sense :)
>>>>
>>>> I'll change this to "SOLID_FILL".
>>>>
>>>>>
>>>>>>> + *
>>>>>>> * Note that all the property extensions described here apply
>>>>>>> either to the
>>>>>>> * plane or the CRTC (e.g. for the background color, which
>>>>>>> currently is not
>>>>>>> * exposed and assumed to be black).
>>>>>>> @@ -648,3 +660,72 @@ int
>>>>>>> drm_plane_create_solid_fill_property(struct drm_plane *plane)
>>>>>>> return 0;
>>>>>>> }
>>>>>>> EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * drm_plane_create_pixel_source_property - create a new pixel
>>>>>>> source property
>>>>>>> + * @plane: drm plane
>>>>>>> + * @supported_sources: bitmask of supported pixel_sources for
>>>>>>> the driver (NOT
>>>>>>> + * including DRM_PLANE_PIXEL_SOURCE_FB, as
>>>>>>> it will be supported
>>>>>>> + * by default).
>>>>>>
>>>>>> I'd say this is too strong. I'd suggest either renaming this to
>>>>>> extra_sources (mentioning that FB is enabled for all the planes) or
>>>>>> allowing any source bitmask (mentioning that FB should be enabled
>>>>>> by the
>>>>>> caller, unless there is a good reason not to do so).
>>>>>
>>>>> Right. I don't see any problem with having planes of type OVERLAY that
>>>>> support only solid_fill and no FB. Planes of type PRIMARY and CURSOR I
>>>>> would expect to always support at least FB.
>>>>>
>>>>> Atomic userspace is prepared to have an OVERLAY plane fail for any
>>>>> arbitrary reason. Legacy userspace probably should not ever see a
>>>>> plane
>>>>> that does not support FB.
>>>>
>>>> Got it... If we allow the possibility of FB sources not being
>>>> supported, then should the default pixel_source per plane be decided
>>>> by the driver too?
>>>>
>>>> I'd forced FB support so that I could set pixel_source to FB in
>>>> __drm_atomic_helper_plane_state_reset(). If we allow more
>>>> flexibility in the default pixel_source value, I guess we can also
>>>> store a default_pixel_source value in the plane_state.
>>>
>>> I'd say, the FB is a sane default. It the driver has other needs, it
>>> can override the value in drm_plane_funcs::reset().
>>>
>>>>
>>>
>>> [skipped the rest]
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
>
More information about the dri-devel
mailing list