Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxx>
> 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)
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
