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

[Javier Martinez Canillas <javierm@redhat.com>]

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 ?

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>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat