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

2017-03-16 Thread Boris Brezillon
On Fri, 10 Feb 2017 19:07:45 +0100
Boris Brezillon  wrote:

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

Just forgot to mention that this patch has been applied to
drm-misc-next a few weeks ago.

> 
> Signed-off-by: Boris Brezillon 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2017-02-28 Thread Boris Brezillon
Hi Eric,

On Mon, 27 Feb 2017 14:30:13 -0800
Eric Anholt  wrote:

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

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 =
> > -   >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(>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(>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(>layer, layout->general_config,
> > -ATMEL_HLCDC_LAYER_ITER2BL |
> > -ATMEL_HLCDC_LAYER_ITER |
> > -ATMEL_HLCDC_LAYER_GAEN |
> > -ATMEL_HLCDC_LAYER_GA_MASK |
> > -  

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

2017-02-27 Thread Eric Anholt
Boris Brezillon  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 

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 

> 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 =
> - >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(>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(>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(>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)


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2017-02-14 Thread Daniel Vetter
On Fri, Feb 10, 2017 at 07:07:45PM +0100, Boris Brezillon wrote:
> 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 
> ---
> Hi Daniel,
> 
> I intentionally dropped your ack, since inheriting from atmel_hlcdc_layer
> is implying a lot of changes.

Well I acked the idea, that still kinda holds. But if you want to
kickstart the drm-misc driver ack economy, Eric has 1-2 vc4 patches that
still need an ack, you could trade r-bs :-)

Cheers, Daniel

> 
> Regards,
> 
> Boris
> 
> Changes in v2:
> - make atmel_hlcdc_plane inherit from atmel_hlcdc_layer
> - provide read/write_reg/cfg() helpers to access layer regs
> - move all layer related definitions into atmel_hlcdc_dc.h and remove
>   atmel_hlcdc_layer.h
> - fix a bug in atmel_hlcdc_plane_atomic_duplicate_state()
> ---
>  drivers/gpu/drm/atmel-hlcdc/Makefile|   1 -
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  |  39 +-
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c|  82 +--
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h| 364 +++--
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c | 666 
> 
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h | 399 --
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 637 +++---
>  7 files changed, 695 insertions(+), 1493 deletions(-)
>  delete mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
>  delete mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/Makefile 
> b/drivers/gpu/drm/atmel-hlcdc/Makefile
> index 10ae426e60bd..bb5f8507a8ce 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/Makefile
> +++ b/drivers/gpu/drm/atmel-hlcdc/Makefile
> @@ -1,6 +1,5 @@
>  atmel-hlcdc-dc-y := atmel_hlcdc_crtc.o \
>   atmel_hlcdc_dc.o \
> - atmel_hlcdc_layer.o \
>   atmel_hlcdc_output.o \
>   atmel_hlcdc_plane.o
>  
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 9b17a66cf0e1..2fcec0a72567 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -445,8 +445,8 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs 
> = {
>  
>  int atmel_hlcdc_crtc_create(struct drm_device *dev)
>  {
> + struct atmel_hlcdc_plane *primary = NULL, *cursor = NULL;
>   struct atmel_hlcdc_dc *dc = dev->dev_private;
> - struct atmel_hlcdc_planes *planes = dc->planes;
>   struct atmel_hlcdc_crtc *crtc;
>   int ret;
>   int i;
> @@ -457,20 +457,41 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev)
>  
>   crtc->dc = dc;
>  
> - ret = drm_crtc_init_with_planes(dev, >base,
> - >primary->base,
> - planes->cursor ? >cursor->base : NULL,
> - _hlcdc_crtc_funcs, NULL);
> + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
> + if (!dc->layers[i])
> + continue;
> +
> + switch (dc->layers[i]->desc->type) {
> + case ATMEL_HLCDC_BASE_LAYER:
> + primary = atmel_hlcdc_layer_to_plane(dc->layers[i]);
> + break;
> +
> + case ATMEL_HLCDC_CURSOR_LAYER:
> + cursor = atmel_hlcdc_layer_to_plane(dc->layers[i]);
> + break;
> +
> + default:
> + break;
> + }
> + }
> +
> + ret = drm_crtc_init_with_planes(dev, >base, >base,
> + >base, _hlcdc_crtc_funcs,
> + NULL);
>   if (ret < 0)
>   goto fail;
>  
>   crtc->id = drm_crtc_index(>base);
>  
> - if (planes->cursor)
> - planes->cursor->base.possible_crtcs = 1 << crtc->id;
> + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
> + struct atmel_hlcdc_plane *overlay;
>  
> - for (i = 0; i < 

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

2017-02-10 Thread Boris Brezillon
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 
---
Hi Daniel,

I intentionally dropped your ack, since inheriting from atmel_hlcdc_layer
is implying a lot of changes.

Regards,

Boris

Changes in v2:
- make atmel_hlcdc_plane inherit from atmel_hlcdc_layer
- provide read/write_reg/cfg() helpers to access layer regs
- move all layer related definitions into atmel_hlcdc_dc.h and remove
  atmel_hlcdc_layer.h
- fix a bug in atmel_hlcdc_plane_atomic_duplicate_state()
---
 drivers/gpu/drm/atmel-hlcdc/Makefile|   1 -
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  |  39 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c|  82 +--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h| 364 +++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c | 666 
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h | 399 --
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 637 +++---
 7 files changed, 695 insertions(+), 1493 deletions(-)
 delete mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
 delete mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h

diff --git a/drivers/gpu/drm/atmel-hlcdc/Makefile 
b/drivers/gpu/drm/atmel-hlcdc/Makefile
index 10ae426e60bd..bb5f8507a8ce 100644
--- a/drivers/gpu/drm/atmel-hlcdc/Makefile
+++ b/drivers/gpu/drm/atmel-hlcdc/Makefile
@@ -1,6 +1,5 @@
 atmel-hlcdc-dc-y := atmel_hlcdc_crtc.o \
atmel_hlcdc_dc.o \
-   atmel_hlcdc_layer.o \
atmel_hlcdc_output.o \
atmel_hlcdc_plane.o
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 9b17a66cf0e1..2fcec0a72567 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -445,8 +445,8 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = 
{
 
 int atmel_hlcdc_crtc_create(struct drm_device *dev)
 {
+   struct atmel_hlcdc_plane *primary = NULL, *cursor = NULL;
struct atmel_hlcdc_dc *dc = dev->dev_private;
-   struct atmel_hlcdc_planes *planes = dc->planes;
struct atmel_hlcdc_crtc *crtc;
int ret;
int i;
@@ -457,20 +457,41 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev)
 
crtc->dc = dc;
 
-   ret = drm_crtc_init_with_planes(dev, >base,
-   >primary->base,
-   planes->cursor ? >cursor->base : NULL,
-   _hlcdc_crtc_funcs, NULL);
+   for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
+   if (!dc->layers[i])
+   continue;
+
+   switch (dc->layers[i]->desc->type) {
+   case ATMEL_HLCDC_BASE_LAYER:
+   primary = atmel_hlcdc_layer_to_plane(dc->layers[i]);
+   break;
+
+   case ATMEL_HLCDC_CURSOR_LAYER:
+   cursor = atmel_hlcdc_layer_to_plane(dc->layers[i]);
+   break;
+
+   default:
+   break;
+   }
+   }
+
+   ret = drm_crtc_init_with_planes(dev, >base, >base,
+   >base, _hlcdc_crtc_funcs,
+   NULL);
if (ret < 0)
goto fail;
 
crtc->id = drm_crtc_index(>base);
 
-   if (planes->cursor)
-   planes->cursor->base.possible_crtcs = 1 << crtc->id;
+   for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
+   struct atmel_hlcdc_plane *overlay;
 
-   for (i = 0; i < planes->noverlays; i++)
-   planes->overlays[i]->base.possible_crtcs = 1 << crtc->id;
+   if (dc->layers[i] &&
+   dc->layers[i]->desc->type == ATMEL_HLCDC_OVERLAY_LAYER) {
+   overlay = atmel_hlcdc_layer_to_plane(dc->layers[i]);
+   overlay->base.possible_crtcs = 1 << crtc->id;
+   }
+   }
 
drm_crtc_helper_add(>base,