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

[Boris Brezillon]

Hi Eric,

On Mon, 27 Feb 2017 14:30:13 -0800
Eric Anholt <eric at anholt.net> wrote:

> 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.

Yes, I'm not happy with the diff either (it's really hard to review),
I'll try do these changes in several steps (as you suggest) and see if
it helps.

> 
> 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>

Thanks a lot for your review.

> 
> > 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).

Actually, I did that to avoid the "over 80 char" checkpatch warning,
but you're right, I should declare this variable 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)

Indeed, I'll fix that.

Thanks,

Boris