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

[icenowy]

在 2017-04-05 10:27，Chen-Yu Tsai 写道：

On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng  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 
---
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 = >plane;
+   primary = plane;
break;
case DRM_PLANE_TYPE_CURSOR:
-   cursor = >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(>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.

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

[icenowy]

在 2017-04-05 10:27，Chen-Yu Tsai 写道：

On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng  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 
---
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 = >plane;
+   primary = plane;
break;
case DRM_PLANE_TYPE_CURSOR:
-   cursor = >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(>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 

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

[Maxime Ripard]

On Wed, Apr 05, 2017 at 01:23:15PM +0800, Icenowy Zheng wrote:
> 
> 2017年4月5日 10:27于 Chen-Yu Tsai 写道：
> >
> > On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng  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  
> > >>> --- 
> > >>> 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 = >plane; 
> > >>> +   primary = plane; 
> > >>> break; 
> > >>> case DRM_PLANE_TYPE_CURSOR: 
> > >>> -   cursor = >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(>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 { 
> > >> 

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

[Maxime Ripard]

On Wed, Apr 05, 2017 at 01:23:15PM +0800, Icenowy Zheng wrote:
> 
> 2017年4月5日 10:27于 Chen-Yu Tsai 写道：
> >
> > On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng  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  
> > >>> --- 
> > >>> 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 = >plane; 
> > >>> +   primary = plane; 
> > >>> break; 
> > >>> case DRM_PLANE_TYPE_CURSOR: 
> > >>> -   cursor = >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(>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; 
> > >> } 
> > >> 
> > >> 

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

[Chen-Yu Tsai]

On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng  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 
>>> ---
>>> 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 = >plane;
>>> +   primary = plane;
>>> break;
>>> case DRM_PLANE_TYPE_CURSOR:
>>> -   cursor = >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(>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.

In our driver we are currently 

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

[Chen-Yu Tsai]

On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng  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 
>>> ---
>>> 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 = >plane;
>>> +   primary = plane;
>>> break;
>>> case DRM_PLANE_TYPE_CURSOR:
>>> -   cursor = >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(>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.

In our driver we are currently initializing all layers, then going

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

[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 
> ---
> 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 = >plane;
> + primary = plane;
>   break;
>   case DRM_PLANE_TYPE_CURSOR:
> - cursor = >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(>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.

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 
>  
>  #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 _layer->plane;
> +}
> +
> +static const struct sunxi_layer_ops layer_ops = {
> + 

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

[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 
> ---
> 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 = >plane;
> + primary = plane;
>   break;
>   case DRM_PLANE_TYPE_CURSOR:
> - cursor = >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(>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.

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 
>  
>  #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 _layer->plane;
> +}
> +
> +static const struct sunxi_layer_ops layer_ops = {
> + .get_plane = 

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

[Chen-Yu Tsai]

On Mon, Apr 3, 2017 at 4:14 PM, Maxime Ripard
 wrote:
> Hi,
>
> 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 
>> ---
>> 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 = >plane;
>> + primary = plane;
>>   break;
>>   case DRM_PLANE_TYPE_CURSOR:
>> - cursor = >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(>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;
>
> I think the logic should be reversed here, the CRTC shouldn't care
> (much) about the layers at all.

Correct. It shouldn't. However since the layers are tied to a specific
crtc, they get created as part of the crtc init sequence.

> We should modify sun4i_crtc_init to get the argument it needs (primary
> and cursor planes for example) through its parameters, and have the
> caller (which iirc is sun4i_drv) call it with the right parameters

The caller is (now) sun4i_crtc_init.

> depending on whether you're using DE or DE2.

Ack.

>
> If we're doing that, I don't think we even need the pointer to the
> array of layers in struct sun4i_crtc, which will make it easier to
> deal with.

Ack.

ChenYu

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

[Chen-Yu Tsai]

On Mon, Apr 3, 2017 at 4:14 PM, Maxime Ripard
 wrote:
> Hi,
>
> 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 
>> ---
>> 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 = >plane;
>> + primary = plane;
>>   break;
>>   case DRM_PLANE_TYPE_CURSOR:
>> - cursor = >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(>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;
>
> I think the logic should be reversed here, the CRTC shouldn't care
> (much) about the layers at all.

Correct. It shouldn't. However since the layers are tied to a specific
crtc, they get created as part of the crtc init sequence.

> We should modify sun4i_crtc_init to get the argument it needs (primary
> and cursor planes for example) through its parameters, and have the
> caller (which iirc is sun4i_drv) call it with the right parameters

The caller is (now) sun4i_crtc_init.

> depending on whether you're using DE or DE2.

Ack.

>
> If we're doing that, I don't think we even need the pointer to the
> array of layers in struct sun4i_crtc, which will make it easier to
> deal with.

Ack.

ChenYu

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

[Maxime Ripard]

Hi,

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 
> ---
> 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 = >plane;
> + primary = plane;
>   break;
>   case DRM_PLANE_TYPE_CURSOR:
> - cursor = >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(>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;

I think the logic should be reversed here, the CRTC shouldn't care
(much) about the layers at all.

We should modify sun4i_crtc_init to get the argument it needs (primary
and cursor planes for example) through its parameters, and have the
caller (which iirc is sun4i_drv) call it with the right parameters
depending on whether you're using DE or DE2.

If we're doing that, I don't think we even need the pointer to the
array of layers in struct sun4i_crtc, which will make it easier to
deal with.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

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

[Maxime Ripard]

Hi,

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 
> ---
> 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 = >plane;
> + primary = plane;
>   break;
>   case DRM_PLANE_TYPE_CURSOR:
> - cursor = >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(>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;

I think the logic should be reversed here, the CRTC shouldn't care
(much) about the layers at all.

We should modify sun4i_crtc_init to get the argument it needs (primary
and cursor planes for example) through its parameters, and have the
caller (which iirc is sun4i_drv) call it with the right parameters
depending on whether you're using DE or DE2.

If we're doing that, I don't think we even need the pointer to the
array of layers in struct sun4i_crtc, which will make it easier to
deal with.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

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

[Icenowy Zheng]

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 
---
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 = >plane;
+   primary = plane;
break;
case DRM_PLANE_TYPE_CURSOR:
-   cursor = >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(>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;
 };
 
 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 
 
 #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 _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)
+ 

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

[Icenowy Zheng]

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 
---
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 = >plane;
+   primary = plane;
break;
case DRM_PLANE_TYPE_CURSOR:
-   cursor = >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(>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;
 };
 
 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 
 
 #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 _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