Re: [PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3046 bytes --]

Hi

Am 05.10.23 um 15:18 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> Hold temporary memory for format conversion in an instance of struct
>> drm_format_conv_state. Update internal helpers of DRM's format-conversion
>> code accordingly. Drivers will later be able to maintain this cache by
>> themselves.
>>
>> Besides caching, struct drm_format_conv_state will be useful to hold
>> additional information for format conversion, such as palette data or
>> foreground/background colors. This will enable conversion from indexed
>> color formats to component-based formats.
>>
>> v3:
>> 	* rename struct drm_xfrm_buf to struct drm_format_conv_state
>> 	  (Javier)
>> 	* remove managed cleanup
>> 	* add drm_format_conv_state_copy() for shadow-plane support
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +/**
>> + * drm_format_conv_state_init - Initialize format-conversion state
>> + * @state: The state to initialize
>> + *
>> + * Clears all fields in struct drm_format_conv_state and installs a DRM
>> + * release action for the buffer. The buffer will be empty with no
>> + * preallocated resources.
>> + */
>> +void drm_format_conv_state_init(struct drm_format_conv_state *state)
>> +{
>> +	state->tmp.mem = NULL;
>> +	state->tmp.size = 0;
>> +	state->tmp.preallocated = false;
>> +}
>> +EXPORT_SYMBOL(drm_format_conv_state_init);
>> +
>> +/**
>> + * drm_format_conv_state_copy - Copy format-conversion state
>> + * @state: Destination state
>> + * @old_state: Source state
>> + *
>> + * Copies format-conversion state from @old_state to @state; except for
>> + * temporary storage.
>> + */
>> +void drm_format_conv_state_copy(struct drm_format_conv_state *state,
>> +				const struct drm_format_conv_state *old_state)
>> +{
>> +	state->tmp.mem = NULL;
>> +	state->tmp.size = 0;
>> +	state->tmp.preallocated = false;
>> +}
>> +EXPORT_SYMBOL(drm_format_conv_state_copy);
>> +
> 
> I'm confused, the copy helper is the same than init. What's the point of
> this function ? Why not just call drm_format_conv_state_init() from the
> __drm_gem_duplicate_shadow_plane_state() function in the next patch ?

I guess that deserves a comment in the code. The reserved buffer is not 
to be copied to another state. So we just clear the fields. But in the 
future, we will likely be extra fields, such as the aforementioned 
palette data, that will be copied. It's just that these fields don't 
exist yet. Hence the copy function is different from the init.

Best regards
Thomas

> 
> Other than that the patch looks good to me. After fixing the issue that
> Noralf pointed out:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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