Hi

Am 21.09.22 um 12:18 schrieb Ville Syrjälä:
[...]
>>
>>>
>>> Though I don't really know if a there is software relying on
>>> that behaviuor. I suppose one idea could be to keep that
>>> behaviour for the legacy ioctls but not for atomic. Ee. any
>>> fb directly specified in a legacy setcrtc/setplane/pageflip
>>> is always considered fully damaged. But including an the same
>>
>> Assuming the specified fb to be damaged seems like a non-issue to me.
>>
>> The problem is with the other framebuffers: if userspace sends us a
>> CURSOR_MOVE ioctl, we can safely assume the cursor fb to be damaged. But
>> we don't want the primary plane's fb to be damaged. Or else we'd redraw
>> the full primary plane.
>>
>>
>>> fb in the atomic ioctl does not imply damage. That would
>>> mean atomic has to rely on specifying damage explicitly, and
>>> any userspace that doesn't do that will be broken.
>>
>> The problem again is not in the damage information on planes that
>> legitimately need a redraw. It's all the other planes that are being
>> redrawn as well. Or is there some scenario that I don't see?
> 
> I thought we're talking about eg. a cursor update that also
> includes the primary plane because apparently userspace is lazy.
> 
> I think what you're saying is that currently there is no
> damage information for the primary plane so you're attempting
> to infer it based on whether the fb property changed or not.

Correct.

> 
> And what I was saying is that IIRC historically specifying
> the same fb again has still implied full damage. Changing
> that behaviour may or may not break exising userspace.

I cannot answer the question. The three cases I've seen at least worked 
with the new semantics. I think Daniel once mentioned that we already 
expect userspace to call the DIRTYFB ioctl after changing a 
framebuffer's content.

> 
>>
>>>
>>> Or we could introduce a client cap for it I guess, but that
>>> would require (minimal) userspace changes.
>>>
>>>>
>>>> I know that we don't give performance guarantees to userspace. But using
>>>> cursor/overlay planes should be faster then not using them. I think
>>>> that's a reasonable assumption for userspace to make.
>>>>
>>>>>
>>>>> Another thing the ioctls have always done is actually perform
>>>>> the commit whether anything changed or nor. That is, they
>>>>> still do all the same the same vblanky stuff and the commit
>>>>> takes the same amount of time. Not sure if your idea is
>>>>> to potentially short circuit the entire thing and make it
>>>>> take no time at all?
>>>>
>>>> The patches don't change the overall commit logics. All they do is to
>>>> set damage updates to a size of 0 if a plane reliably does not need an
>>>> update.
>>>
>>> What I gathered is that you seemed to only consider the fb
>>> contents as something that needs damage handling. That is not
>>> the case for stuff like eDP PSR and DSI command mode displays
>>> where we need to upload a new full frame whenever also the
>>> non-damaged fb contents would get transformed by the hardware
>>> on the way to the remote frambuffer. That would mean any change
>>> in eg. scanout coordinates, color management, etc.
>>
>> There is support for changing scanout coordinates in
>> drm_atomic_helper_damage_iter_init() in patch 2. In general, maybe the
>> heuristic needs to be stricter to only handle updates that only change
>> damage.
>>
>> For now, the problem only happens after converting ast to SHMEM. All the
>> other SHMEM-based drivers use a single plane where the problem doesn't
>> happen; or only reprogram the scanout address, which is fast. If the
>> damage-handling logic imposes problems on other drivers, some of it
>> could possibly be moved into ast itself.
> 
> Maybe we have two clearly separate classes of uses case:
> 1. devices where only damage to the fb contents matter (eg. some kind of
>     shadow fb that is the same size as the real fb).
> 2. devices where everything about the scanout process matters (eg. PSR)
> ?
> 
> Maybe there should be helpers to deal with just the first case,
> and then some more helpers (or just driver code) to pile the rest
> on top as well when needed?

Makes sense.

I know that these plane-state are not good style, but with them in 
place, much of the heuristic could be moved from 
drm_atomic_helper_check_plane_damage() into the driver if necessary.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev