[PATCH v2] drm/atmel-hlcdc: Simplify the HLCDC layer logic

[Eric Anholt]

Boris Brezillon <boris.brezillon at free-electrons.com> writes:

> An HLCDC layers in Atmel's nomenclature is either a DRM plane or a 'Post
> Processing Layer' which can be used to output the results of the HLCDC
> composition in a memory buffer.
>
> atmel_hlcdc_layer.c was designed to be generic enough to be re-usable in
> both cases, but we're not exposing the post-processing layer yet, and
> even if we were, I'm not sure the code would provide the necessary tools
> to manipulate this kind of layer.
>
> Moreover, the code in atmel_hlcdc_{plane,layer}.c was designed before the
> atomic modesetting API, and was trying solve the
> check-setting/commit-if-ok/rollback-otherwise problem, which is now
> entirely solved by the existing core infrastructure.
>
> And finally, the code in atmel_hlcdc_layer.c in over-complicated compared
> to what we really need. This rework is a good excuse to simplify it. Note
> that this rework solves an existing resource leak (leading to a -EBUSY
> error) which I failed to clearly identify.
>
> Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>

Deleting 20% of your driver?  Awesome.

This was tricky to review, though.  I wonder if the configuration layout
description restructuring (and some of the config update helpers) could
have been done separately from the rollback removal parts.  The merge of
atmel_hlcdc_layer.h into atmel_hlcdc_dc.h was also something that I only
reviewed pieces of by occasionally looking into a specific definition to
see if it had happened to change in the move.

I've gone through all the code, particularly with an eye to things like
copy/paste bugs, so I think with a couple of little comments below
addressed,

Reviewed-by: Eric Anholt <eric at anholt.net>

> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index 246ed1e33d8a..cb6b2d5ae50b 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c

>  static void
>  atmel_hlcdc_plane_update_general_settings(struct atmel_hlcdc_plane *plane,
>  					struct atmel_hlcdc_plane_state *state)
>  {
> -	const struct atmel_hlcdc_layer_cfg_layout *layout =
> -						&plane->layer.desc->layout;
> -	unsigned int cfg = ATMEL_HLCDC_LAYER_DMA;
> +	unsigned int cfg = ATMEL_HLCDC_LAYER_DMA_BLEN_INCR16 | state->ahb_id;
> +	const struct atmel_hlcdc_layer_desc *desc = plane->layer.desc;
> +
> +	/*
> +	 * Rotation optimization is not working on RGB888 (rotation is still
> +	 * working but without any optimization).
> +	 */
> +	if (state->base.fb->pixel_format == DRM_FORMAT_RGB888)
> +		cfg |= ATMEL_HLCDC_LAYER_DMA_ROTDIS;
> +
> +	atmel_hlcdc_layer_write_cfg(&plane->layer, ATMEL_HLCDC_LAYER_DMA_CFG,
> +				    cfg);
> +
> +	cfg = ATMEL_HLCDC_LAYER_DMA;
>  
>  	if (plane->base.type != DRM_PLANE_TYPE_PRIMARY) {
> +		u32 format = state->base.fb->pixel_format;
> +
>  		cfg |= ATMEL_HLCDC_LAYER_OVR | ATMEL_HLCDC_LAYER_ITER2BL |
>  		       ATMEL_HLCDC_LAYER_ITER;
>  
> -		if (atmel_hlcdc_format_embeds_alpha(state->base.fb->pixel_format))
> +		if (atmel_hlcdc_format_embeds_alpha(format))
>  			cfg |= ATMEL_HLCDC_LAYER_LAEN;
>  		else
>  			cfg |= ATMEL_HLCDC_LAYER_GAEN |
>  			       ATMEL_HLCDC_LAYER_GA(state->alpha);
>  	}

The format temp here seems gratuitous (unless you wanted to move it up
to the declarations at the top of the function and use it for ROTDIS).

>  
> -	atmel_hlcdc_layer_update_cfg(&plane->layer,
> -				     ATMEL_HLCDC_LAYER_DMA_CFG_ID,
> -				     ATMEL_HLCDC_LAYER_DMA_BLEN_MASK |
> -				     ATMEL_HLCDC_LAYER_DMA_SIF,
> -				     ATMEL_HLCDC_LAYER_DMA_BLEN_INCR16 |
> -				     state->ahb_id);
> -
> -	atmel_hlcdc_layer_update_cfg(&plane->layer, layout->general_config,
> -				     ATMEL_HLCDC_LAYER_ITER2BL |
> -				     ATMEL_HLCDC_LAYER_ITER |
> -				     ATMEL_HLCDC_LAYER_GAEN |
> -				     ATMEL_HLCDC_LAYER_GA_MASK |
> -				     ATMEL_HLCDC_LAYER_LAEN |
> -				     ATMEL_HLCDC_LAYER_OVR |
> -				     ATMEL_HLCDC_LAYER_DMA, cfg);
> +	if (state->disc_h * state->disc_w)
> +		cfg |= ATMEL_HLCDC_LAYER_DISCEN;

This is a weird looking pattern for what I think is

if (state->disc_h && state->disc_w)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170227/dca98484/attachment.sig>