Boris Brezillon boris.brezillon@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@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@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;
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)