Re: [PATCH v5 01/15] drm/amd/display: Handle struct drm_plane_state.ignore_damage_clips

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Javier

Am 11.06.26 um 12:10 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> Hello Thomas,
>
>> The mode-setting pipeline can disabled damage clippings for a commit
>> by setting ignore_damage_clips in struct drm_plane_state. The commit
>> will then do a full display update.
>>
>> Test the flag in DCN code and do a full update in DCN code if it has
>> been set.
>>
>> Commit 35ed38d58257 ("drm: Allow drivers to indicate the damage helpers
>> to ignore damage clips") introduced ignore_damage_clips to selectively
>> ignore damage clipping in certain framebuffer changes. This driver does
>> not do that, but DRM's damage iterator will soon rely on the flag.
>> Therefore supporting it here as well make sense for consistency.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 35ed38d58257 ("drm: Allow drivers to indicate the damage helpers to ignore damage clips")
> I don't think that a Fixes tag is correct here? Your patch series
> is changing the 'struct drm_plane_state.ignore_damage_clips' and
> the changes make sense, but definitely isn't a fix in my opinion.

But shouldn't we have added this test in amdgpu and the other drivers as 
part of commit 35ed38d58257 ? Sure, these drivers don't use 
ignore_damage_clips, but it's still an inconsistency wrt damage 
handlers. Hence the Fixes tag. Another problem is that the drivers never 
did the test for changes to the plane-state src coordinate that the 
damage iterator does. But that is only fixed later in the series.
>
> Having said that, the change look good to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for reviewing.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)