Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8080 bytes --]

On Tue, 11 Jul 2023 15:42:28 -0700
Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:

> 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@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@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.

Not even that complex. There is no need to reboot and no need to kill
anything to hit this. A simple VT-switch can switch between two
different KMS clients, one could be using atomic with solid_fill, and
the other is an old legacy UAPI user. If the atomic client leaves stuff
up in KMS state, it would be nice if the legacy app still worked.

Or, maybe it's not a VT-switch but switching from, say, a graphical
login manager to a desktop or back. The same thing, different KMS
clients. But in this case it is more likely that userspace follows
common play rules, so it's less likely to have problematic left-over
KMS state.

Or, maybe you simply quit the atomic KMS client and expect fbcon to
successfully take over. I don't know what APIs fbcon uses inside the
kernel, but it definitely should clean up any mess left over by an
atomic (or legacy) KMS client to make sure it shows itself.
Traditionally it has failed to do that though, I don't know if things
are better nowadays (GAMMA_LUT, HDR_OUTPUT_METADATA, probably more).

Switching between two KMS clients is fundamentally problematic. If both
old and new KMS client are atomic, the kernel cannot simply go
resetting any KMS state automatically, because the kernel does not
know which part of state is good to reset and which would just cause
unwanted flicker and delays. That means that it is up to the two atomic
KMS clients to agree on common play rules and not leave funny state up.
IOW, not the kernel's problem, by what I have understood from kernel
developer opinions.

However, legacy KMS clients are different. As legacy clients, they may
not even have access to the properties they might need to clean up
after a previous atomic KMS client. The legacy UAPI is expected to
be slow and glitchy, but also Just Work(TM), so the kernel can reset
everything a legacy KMS client would not have access to when a legacy
KMS client issues drmModeSetPlane or drmModeSetCrtc (pardon for using
libdrm API functions for the ioctls whose names I never memorised).


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]