Hi Eric,
On Mon, 27 Feb 2017 14:30:13 -0800 Eric Anholt eric@anholt.net wrote:
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.
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@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;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