Re: [PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init, merged}() helpers

[Pekka Paalanen]

On Tue, 14 Nov 2023 17:05:12 +0100
Javier Martinez Canillas <javierm@xxxxxxxxxx> wrote:

> Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
> 
> > Hi
> >
> > Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:  
> 
> [...]
> 
> >>   	struct drm_rect src;
> >>   	memset(iter, 0, sizeof(*iter));
> >> @@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> >>   	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
> >>   	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
> >>   
> >> -	if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) {
> >> +	if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src) ||
> >> +	    (buffer_damage && old_state->fb != state->fb)) {  
> >
> > I'd assume that this change effectivly disables damage handling. AFAICT 
> > user space often does a page flip with a new framebuffer plus damage 
> > data. Now, with each change of the framebuffer we ignore the damage 
> > information. It's not a blocker as that's the behavior before 6.4, but 
> > we should be aware of it.
> >  
> 
> Yes, which is the goal of this patch since page flip with a new framebuffer
> attached to a plane plus damage information can't be supported by drivers
> that do per-buffer uploads.
> 
> This was causing some weston and wlroots to have flickering artifacts, due
> the framebuffers being changed since the last plane update.
> 
> For now it was decided with Sima, Simon and Pekka that is the best we can
> do and the reason why I add a TODO in patch #6.
> 

Hi all,

this made me thinking...

The per-buffer damage accumulation that would be needed is per
upload-buffer, not per KMS FB from userspace. So it should not make any
difference whether userspace flips to another FB or not, the damage
will need to be accumulated per upload-buffer anyway if the driver is
flipping upload-buffers, in order to make the upload-buffer fully
up-to-date.

Why was that not more broken than it already was?

Is there a fixed 1:1 relationship between userspace KMS FBs and the
driver upload-buffers?

Userspace is already taking care that the KMS FB is always fully
up-to-date, FWIW, so the kernel can certainly read areas outside of
FB_DAMAGE_CLIPS if it wants to.


Thanks,
pq

Attachment: pgpLW6oq_bN1S.pgp
Description: OpenPGP digital signature