Re: Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type

[icenowy-h8G6r0blFSE@public.gmane.org]

在 2017-04-05 10:27，Chen-Yu Tsai 写道：
> On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
>> 
>> 
>> 在 2017年04月05日 03:28, Sean Paul 写道:
>>> 
>>> On Thu, Mar 30, 2017 at 03:46:06AM +0800, Icenowy Zheng wrote:
>>>> 
>>>> As we are going to add support for the Allwinner DE2 Mixer in 
>>>> sun4i-drm
>>>> driver, we will finally have two types of layer.
>>>> 
>>>> Abstract the layer type to void * and a ops struct, which contains 
>>>> the
>>>> only function used by crtc -- get the drm_plane struct of the layer.
>>>> 
>>>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>>>> ---
>>>> Refactored patch in v3.
>>>> 
>>>>  drivers/gpu/drm/sun4i/sun4i_crtc.c  | 19 +++++++++++--------
>>>>  drivers/gpu/drm/sun4i/sun4i_crtc.h  |  3 ++-
>>>>  drivers/gpu/drm/sun4i/sun4i_layer.c | 19 ++++++++++++++++++-
>>>>  drivers/gpu/drm/sun4i/sun4i_layer.h |  2 +-
>>>>  drivers/gpu/drm/sun4i/sunxi_layer.h | 17 +++++++++++++++++
>>>>  5 files changed, 49 insertions(+), 11 deletions(-)
>>>>  create mode 100644 drivers/gpu/drm/sun4i/sunxi_layer.h
>>>> 
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c
>>>> b/drivers/gpu/drm/sun4i/sun4i_crtc.c
>>>> index 3c876c3a356a..33854ee7f636 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
>>>> @@ -29,6 +29,7 @@
>>>>  #include "sun4i_crtc.h"
>>>>  #include "sun4i_drv.h"
>>>>  #include "sun4i_layer.h"
>>>> +#include "sunxi_layer.h"
>>>>  #include "sun4i_tcon.h"
>>>> 
>>>>  static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
>>>> @@ -149,7 +150,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct 
>>>> drm_device
>>>> *drm,
>>>>         scrtc->tcon = tcon;
>>>> 
>>>>         /* Create our layers */
>>>> -       scrtc->layers = sun4i_layers_init(drm, scrtc->backend);
>>>> +       scrtc->layers = (void **)sun4i_layers_init(drm, scrtc);
>>>>         if (IS_ERR(scrtc->layers)) {
>>>>                 dev_err(drm->dev, "Couldn't create the planes\n");
>>>>                 return NULL;
>>>> @@ -157,14 +158,15 @@ struct sun4i_crtc *sun4i_crtc_init(struct
>>>> drm_device *drm,
>>>> 
>>>>         /* find primary and cursor planes for 
>>>> drm_crtc_init_with_planes
>>>> */
>>>>         for (i = 0; scrtc->layers[i]; i++) {
>>>> -               struct sun4i_layer *layer = scrtc->layers[i];
>>>> +               void *layer = scrtc->layers[i];
>>>> +               struct drm_plane *plane =
>>>> scrtc->layer_ops->get_plane(layer);
>>>> 
>>>> -               switch (layer->plane.type) {
>>>> +               switch (plane->type) {
>>>>                 case DRM_PLANE_TYPE_PRIMARY:
>>>> -                       primary = &layer->plane;
>>>> +                       primary = plane;
>>>>                         break;
>>>>                 case DRM_PLANE_TYPE_CURSOR:
>>>> -                       cursor = &layer->plane;
>>>> +                       cursor = plane;
>>>>                         break;
>>>>                 default:
>>>>                         break;
>>>> @@ -190,10 +192,11 @@ struct sun4i_crtc *sun4i_crtc_init(struct
>>>> drm_device *drm,
>>>>         /* Set possible_crtcs to this crtc for overlay planes */
>>>>         for (i = 0; scrtc->layers[i]; i++) {
>>>>                 uint32_t possible_crtcs =
>>>> BIT(drm_crtc_index(&scrtc->crtc));
>>>> -               struct sun4i_layer *layer = scrtc->layers[i];
>>>> +               void *layer = scrtc->layers[i];
>>>> +               struct drm_plane *plane =
>>>> scrtc->layer_ops->get_plane(layer);
>>>> 
>>>> -               if (layer->plane.type == DRM_PLANE_TYPE_OVERLAY)
>>>> -                       layer->plane.possible_crtcs = 
>>>> possible_crtcs;
>>>> +               if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>>>> +                       plane->possible_crtcs = possible_crtcs;
>>>>         }
>>>> 
>>>>         return scrtc;
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.h
>>>> b/drivers/gpu/drm/sun4i/sun4i_crtc.h
>>>> index 230cb8f0d601..a4036ee44cf8 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.h
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.h
>>>> @@ -19,7 +19,8 @@ struct sun4i_crtc {
>>>> 
>>>>         struct sun4i_backend            *backend;
>>>>         struct sun4i_tcon               *tcon;
>>>> -       struct sun4i_layer              **layers;
>>>> +       void                            **layers;
>>>> +       const struct sunxi_layer_ops    *layer_ops;
>>> 
>>> 
>>> I think you should probably take a different approach to abstract the
>>> layer
>>> type. How about creating
>>> 
>>> struct sunxi_layer {
>>>         struct drm_plane plane;
>>> }
>>> 
>>> base and then subclassing that for sun4i and sun8i? By doing this you 
>>> can
>>> avoid
>>> the nasty casting and you can also get rid of the get_plane() hook 
>>> and
>>> layer_ops.
>> 
>> 
>> For the situation that using ** things are easily to get weird.
> 
> That code could be reworked, by initializing the layers directly within
> the crtc init code. If you look at rockchip's drm driver, you'll see
> they do this. There is a good reason to do it this way, as you need
> to first create the primary and cursor layers, pass them in when you
> create the crtc, then initialize any additional layers with the
> possible_crtcs bitmap.

I feel that it's still more proper to offload plane creation code
to *_layers_init function, as:

1. We cannot assume the cursor layer's
existance. In fact currently no code in sun4i-drm (including this
patchset) create a cursor layer.

2. The format of planes heavily depend on the engine type (
sun4i-backend or sun8i-mixer).

3. We should create planes according to the type of engine.
Currently the *_layers_init function is part of engine code (See my
Makefile change).

4. If we do so we will have two codes for plane creating -- one for
primary in sun4i_crtc_init, another for overlays in *_layers_init.

> 
> In our driver we are currently initializing all layers, then going
> back and filling in possible_crtcs for the extra layers.
> 
> And as Maxime and I mentioned in the other thread, we don't really
> need to keep a reference to **layers.

It's correct, layers doesn't need to be kept.

And the struct sunxi_layer refactor also makes sense.

> 
> Regards
> ChenYu
> 
>> 
>>> 
>>> Sean
>>> 
>>> 
>>> 
>>>>  };
>>>> 
>>>>  static inline struct sun4i_crtc *drm_crtc_to_sun4i_crtc(struct 
>>>> drm_crtc
>>>> *crtc)
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
>>>> b/drivers/gpu/drm/sun4i/sun4i_layer.c
>>>> index f26bde5b9117..bc4a70d6968b 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
>>>> @@ -16,7 +16,9 @@
>>>>  #include <drm/drmP.h>
>>>> 
>>>>  #include "sun4i_backend.h"
>>>> +#include "sun4i_crtc.h"
>>>>  #include "sun4i_layer.h"
>>>> +#include "sunxi_layer.h"
>>>> 
>>>>  struct sun4i_plane_desc {
>>>>                enum drm_plane_type     type;
>>>> @@ -100,6 +102,17 @@ static const struct sun4i_plane_desc
>>>> sun4i_backend_planes[] = {
>>>>         },
>>>>  };
>>>> 
>>>> +static struct drm_plane *sun4i_layer_get_plane(void *layer)
>>>> +{
>>>> +       struct sun4i_layer *sun4i_layer = layer;
>>>> +
>>>> +       return &sun4i_layer->plane;
>>>> +}
>>>> +
>>>> +static const struct sunxi_layer_ops layer_ops = {
>>>> +       .get_plane = sun4i_layer_get_plane,
>>>> +};
>>>> +
>>>>  static struct sun4i_layer *sun4i_layer_init_one(struct drm_device 
>>>> *drm,
>>>>                                                 struct sun4i_backend
>>>> *backend,
>>>>                                                 const struct
>>>> sun4i_plane_desc *plane)
>>>> @@ -129,9 +142,10 @@ static struct sun4i_layer
>>>> *sun4i_layer_init_one(struct drm_device *drm,
>>>>  }
>>>> 
>>>>  struct sun4i_layer **sun4i_layers_init(struct drm_device *drm,
>>>> -                                      struct sun4i_backend 
>>>> *backend)
>>>> +                                      struct sun4i_crtc *crtc)
>>>>  {
>>>>         struct sun4i_layer **layers;
>>>> +       struct sun4i_backend *backend = crtc->backend;
>>>>         int i;
>>>> 
>>>>         layers = devm_kcalloc(drm->dev, 
>>>> ARRAY_SIZE(sun4i_backend_planes)
>>>> + 1,
>>>> @@ -181,5 +195,8 @@ struct sun4i_layer **sun4i_layers_init(struct
>>>> drm_device *drm,
>>>>                 layers[i] = layer;
>>>>         };
>>>> 
>>>> +       /* Assign layer ops to the CRTC */
>>>> +       crtc->layer_ops = &layer_ops;
>>>> +
>>>>         return layers;
>>>>  }
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.h
>>>> b/drivers/gpu/drm/sun4i/sun4i_layer.h
>>>> index 4be1f0919df2..425eea7b9e3b 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_layer.h
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.h
>>>> @@ -27,6 +27,6 @@ plane_to_sun4i_layer(struct drm_plane *plane)
>>>>  }
>>>> 
>>>>  struct sun4i_layer **sun4i_layers_init(struct drm_device *drm,
>>>> -                                      struct sun4i_backend 
>>>> *backend);
>>>> +                                      struct sun4i_crtc *crtc);
>>>> 
>>>>  #endif /* _SUN4I_LAYER_H_ */
>>>> diff --git a/drivers/gpu/drm/sun4i/sunxi_layer.h
>>>> b/drivers/gpu/drm/sun4i/sunxi_layer.h
>>>> new file mode 100644
>>>> index 000000000000..d8838ec39299
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/sun4i/sunxi_layer.h
>>>> @@ -0,0 +1,17 @@
>>>> +/*
>>>> + * Copyright (C) 2017 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License as
>>>> + * published by the Free Software Foundation; either version 2 of
>>>> + * the License, or (at your option) any later version.
>>>> + */
>>>> +
>>>> +#ifndef _SUNXI_LAYER_H_
>>>> +#define _SUNXI_LAYER_H_
>>>> +
>>>> +struct sunxi_layer_ops {
>>>> +       struct drm_plane *(*get_plane)(void *layer);
>>>> +};
>>>> +
>>>> +#endif /* _SUNXI_LAYER_H_ */
>>>> --
>>>> 2.12.0
>>>> 
>>>> 
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> 
>>> 
>> 
>> --
>> You received this message because you are subscribed to the Google 
>> Groups
>> "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send 
>> an
>> email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
>> For more options, visit https://groups.google.com/d/optout.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.