Re: [PATCH 09/10] drm/sun4i: Add a dedicated ioctl call for allocating tiled buffers

2018-03-27 Thread Maxime Ripard 
On Tue, Mar 27, 2018 at 10:41:38AM +0200, Paul Kocialkowski wrote:
> > > +int drm_sun4i_gem_create_tiled(struct drm_device *dev, void *data,
> > > +struct drm_file *file_priv);
> > 
> > Do you need it to be non-static, and part of the header as well?
> 
> Here as well, I just find that it looks more readable that way, below
> the drm driver structure definition instead of above it.

But it also creates a global symbol for no particular reason, while
we're doing the function-first-structure-later pattern pretty much
everywhere else in the kernel.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


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

Re: [PATCH 09/10] drm/sun4i: Add a dedicated ioctl call for allocating tiled buffers

2018-03-27 Thread Paul Kocialkowski 
On Fri, 2018-03-23 at 11:48 +0100, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Mar 21, 2018 at 04:29:03PM +0100, Paul Kocialkowski wrote:
> > This introduces a dedicated ioctl for allocating tiled buffers in
> > the
> > Allwinner MB32 format, that comes with a handful of specific
> > constraints. In particular, the stride of the buffers is expected to
> > be
> > aligned to 32 bytes.
> 
> You should have more details here. What are those constraints, what is
> the expected user? Can you use it only for the scanout, like the dumb
> buffers, or also for the other devices in the system?

Agreed.

> > Signed-off-by: Paul Kocialkowski 
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_drv.c | 96
> > +++
> >  drivers/gpu/drm/sun4i/sun4i_drv.h |  2 +
> >  include/uapi/drm/sun4i_drm.h  | 42 +
> >  3 files changed, 140 insertions(+)
> >  create mode 100644 include/uapi/drm/sun4i_drm.h
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > index d374bb61c565..e9cb03d34b44 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > @@ -21,11 +21,18 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include "sun4i_drv.h"
> >  #include "sun4i_frontend.h"
> >  #include "sun4i_framebuffer.h"
> >  #include "sun4i_tcon.h"
> > +#include "sun4i_format.h"
> > +
> > +static const struct drm_ioctl_desc sun4i_drv_ioctls[] = {
> > +   DRM_IOCTL_DEF_DRV(SUN4I_GEM_CREATE_TILED,
> > drm_sun4i_gem_create_tiled,
> > + DRM_AUTH | DRM_RENDER_ALLOW),
> 
> Why do you need DRM_RENDER_ALLOW? as far as I know, this is only
> useful for render-nodes.

It's probably undeeded indeed.

> > +};
> >  
> >  DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
> >  
> > @@ -34,6 +41,8 @@ static struct drm_driver sun4i_drv_driver = {
> >  
> > /* Generic Operations */
> > .lastclose  = drm_fb_helper_lastclose,
> > +   .ioctls = sun4i_drv_ioctls,
> > +   .num_ioctls = ARRAY_SIZE(sun4i_drv_ioctls),
> > .fops   = _drv_fops,
> > .name   = "sun4i-drm",
> > .desc   = "Allwinner sun4i Display
> > Engine",
> > @@ -69,6 +78,93 @@ int drm_sun4i_gem_dumb_create(struct drm_file
> > *file_priv,
> > return drm_gem_cma_dumb_create_internal(file_priv, drm,
> > args);
> >  }
> >  
> > +int drm_sun4i_gem_create_tiled(struct drm_device *drm, void *data,
> > +  struct drm_file *file_priv)
> > +{
> > +   struct drm_sun4i_gem_create_tiled *args = data;
> > +   struct drm_gem_cma_object *cma_obj;
> > +   struct drm_gem_object *gem_obj;
> > +   uint32_t luma_stride, chroma_stride;
> > +   uint32_t luma_height, chroma_height;
> > +   int ret;
> > +
> > +   if (!sun4i_format_supports_tiling(args->format))
> > +   return -EINVAL;
> > +
> > +   memset(args->pitches, 0, sizeof(args->pitches));
> > +   memset(args->offsets, 0, sizeof(args->offsets));
> > +
> > +   /* Stride and height are aligned to 32 bytes for MB32 tiled
> > format. */
> > +   luma_stride = ALIGN(args->width, 32);
> > +   luma_height = ALIGN(args->height, 32);
> > +
> > +   if (sun4i_format_is_semiplanar(args->format)) {
> > +   chroma_stride = luma_stride;
> > +
> > +   if (sun4i_format_is_yuv420(args->format))
> > +   chroma_height = ALIGN(DIV_ROUND_UP(args-
> > >height, 2), 32);
> > +   else if (sun4i_format_is_yuv422(args->format))
> > +   chroma_height = luma_height;
> > +   else
> > +   return -EINVAL;
> > +
> > +   args->pitches[0] = luma_stride;
> > +   args->pitches[1] = chroma_stride;
> > +
> > +   args->offsets[0] = 0;
> > +   args->offsets[1] = luma_stride * luma_height;
> > +
> > +   args->size = luma_stride * luma_height +
> > +chroma_stride * chroma_height;
> > +   } else if (sun4i_format_is_planar(args->format)) {
> > +   if (sun4i_format_is_yuv411(args->format)) {
> > +   chroma_stride = ALIGN(DIV_ROUND_UP(args-
> > >width, 4), 32);
> > +   chroma_height = luma_height;
> > +   } if (sun4i_format_is_yuv420(args->format)) {
> > +   chroma_stride = ALIGN(DIV_ROUND_UP(args-
> > >width, 2), 32);
> > +   chroma_height = ALIGN(DIV_ROUND_UP(args-
> > >height, 2), 32);
> > +   } else if (sun4i_format_is_yuv422(args->format)) {
> > +   chroma_stride = ALIGN(DIV_ROUND_UP(args-
> > >width, 2), 32);
> > +   chroma_height = luma_height;
> > +   } else {
> > +   return -EINVAL;
> > +   }
> > +
> > +   args->pitches[0] = luma_stride;
> > +   args->pitches[1] = chroma_stride;
> > +   args->pitches[2] = chroma_stride;
> > +
> > +   args->offsets[0] = 0;
> > +

Re: [PATCH 09/10] drm/sun4i: Add a dedicated ioctl call for allocating tiled buffers

2018-03-23 Thread Maxime Ripard 
Hi,

On Wed, Mar 21, 2018 at 04:29:03PM +0100, Paul Kocialkowski wrote:
> This introduces a dedicated ioctl for allocating tiled buffers in the
> Allwinner MB32 format, that comes with a handful of specific
> constraints. In particular, the stride of the buffers is expected to be
> aligned to 32 bytes.

You should have more details here. What are those constraints, what is
the expected user? Can you use it only for the scanout, like the dumb
buffers, or also for the other devices in the system?

> Signed-off-by: Paul Kocialkowski 
> ---
>  drivers/gpu/drm/sun4i/sun4i_drv.c | 96 
> +++
>  drivers/gpu/drm/sun4i/sun4i_drv.h |  2 +
>  include/uapi/drm/sun4i_drm.h  | 42 +
>  3 files changed, 140 insertions(+)
>  create mode 100644 include/uapi/drm/sun4i_drm.h
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
> b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index d374bb61c565..e9cb03d34b44 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -21,11 +21,18 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "sun4i_drv.h"
>  #include "sun4i_frontend.h"
>  #include "sun4i_framebuffer.h"
>  #include "sun4i_tcon.h"
> +#include "sun4i_format.h"
> +
> +static const struct drm_ioctl_desc sun4i_drv_ioctls[] = {
> + DRM_IOCTL_DEF_DRV(SUN4I_GEM_CREATE_TILED, drm_sun4i_gem_create_tiled,
> +   DRM_AUTH | DRM_RENDER_ALLOW),

Why do you need DRM_RENDER_ALLOW? as far as I know, this is only
useful for render-nodes.

> +};
>  
>  DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
>  
> @@ -34,6 +41,8 @@ static struct drm_driver sun4i_drv_driver = {
>  
>   /* Generic Operations */
>   .lastclose  = drm_fb_helper_lastclose,
> + .ioctls = sun4i_drv_ioctls,
> + .num_ioctls = ARRAY_SIZE(sun4i_drv_ioctls),
>   .fops   = _drv_fops,
>   .name   = "sun4i-drm",
>   .desc   = "Allwinner sun4i Display Engine",
> @@ -69,6 +78,93 @@ int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
>   return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
>  }
>  
> +int drm_sun4i_gem_create_tiled(struct drm_device *drm, void *data,
> +struct drm_file *file_priv)
> +{
> + struct drm_sun4i_gem_create_tiled *args = data;
> + struct drm_gem_cma_object *cma_obj;
> + struct drm_gem_object *gem_obj;
> + uint32_t luma_stride, chroma_stride;
> + uint32_t luma_height, chroma_height;
> + int ret;
> +
> + if (!sun4i_format_supports_tiling(args->format))
> + return -EINVAL;
> +
> + memset(args->pitches, 0, sizeof(args->pitches));
> + memset(args->offsets, 0, sizeof(args->offsets));
> +
> + /* Stride and height are aligned to 32 bytes for MB32 tiled format. */
> + luma_stride = ALIGN(args->width, 32);
> + luma_height = ALIGN(args->height, 32);
> +
> + if (sun4i_format_is_semiplanar(args->format)) {
> + chroma_stride = luma_stride;
> +
> + if (sun4i_format_is_yuv420(args->format))
> + chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 
> 32);
> + else if (sun4i_format_is_yuv422(args->format))
> + chroma_height = luma_height;
> + else
> + return -EINVAL;
> +
> + args->pitches[0] = luma_stride;
> + args->pitches[1] = chroma_stride;
> +
> + args->offsets[0] = 0;
> + args->offsets[1] = luma_stride * luma_height;
> +
> + args->size = luma_stride * luma_height +
> +  chroma_stride * chroma_height;
> + } else if (sun4i_format_is_planar(args->format)) {
> + if (sun4i_format_is_yuv411(args->format)) {
> + chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 4), 32);
> + chroma_height = luma_height;
> + } if (sun4i_format_is_yuv420(args->format)) {
> + chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
> + chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 
> 32);
> + } else if (sun4i_format_is_yuv422(args->format)) {
> + chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
> + chroma_height = luma_height;
> + } else {
> + return -EINVAL;
> + }
> +
> + args->pitches[0] = luma_stride;
> + args->pitches[1] = chroma_stride;
> + args->pitches[2] = chroma_stride;
> +
> + args->offsets[0] = 0;
> + args->offsets[1] = luma_stride * luma_height;
> + args->offsets[2] = luma_stride * luma_height +
> +chroma_stride * chroma_height;
> +
> + args->size = luma_stride * luma_height +
> +

[PATCH 09/10] drm/sun4i: Add a dedicated ioctl call for allocating tiled buffers

2018-03-22 Thread Paul Kocialkowski 
This introduces a dedicated ioctl for allocating tiled buffers in the
Allwinner MB32 format, that comes with a handful of specific
constraints. In particular, the stride of the buffers is expected to be
aligned to 32 bytes.

Signed-off-by: Paul Kocialkowski 
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 96 +++
 drivers/gpu/drm/sun4i/sun4i_drv.h |  2 +
 include/uapi/drm/sun4i_drm.h  | 42 +
 3 files changed, 140 insertions(+)
 create mode 100644 include/uapi/drm/sun4i_drm.h

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
b/drivers/gpu/drm/sun4i/sun4i_drv.c
index d374bb61c565..e9cb03d34b44 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -21,11 +21,18 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "sun4i_drv.h"
 #include "sun4i_frontend.h"
 #include "sun4i_framebuffer.h"
 #include "sun4i_tcon.h"
+#include "sun4i_format.h"
+
+static const struct drm_ioctl_desc sun4i_drv_ioctls[] = {
+   DRM_IOCTL_DEF_DRV(SUN4I_GEM_CREATE_TILED, drm_sun4i_gem_create_tiled,
+ DRM_AUTH | DRM_RENDER_ALLOW),
+};
 
 DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
 
@@ -34,6 +41,8 @@ static struct drm_driver sun4i_drv_driver = {
 
/* Generic Operations */
.lastclose  = drm_fb_helper_lastclose,
+   .ioctls = sun4i_drv_ioctls,
+   .num_ioctls = ARRAY_SIZE(sun4i_drv_ioctls),
.fops   = _drv_fops,
.name   = "sun4i-drm",
.desc   = "Allwinner sun4i Display Engine",
@@ -69,6 +78,93 @@ int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
 }
 
+int drm_sun4i_gem_create_tiled(struct drm_device *drm, void *data,
+  struct drm_file *file_priv)
+{
+   struct drm_sun4i_gem_create_tiled *args = data;
+   struct drm_gem_cma_object *cma_obj;
+   struct drm_gem_object *gem_obj;
+   uint32_t luma_stride, chroma_stride;
+   uint32_t luma_height, chroma_height;
+   int ret;
+
+   if (!sun4i_format_supports_tiling(args->format))
+   return -EINVAL;
+
+   memset(args->pitches, 0, sizeof(args->pitches));
+   memset(args->offsets, 0, sizeof(args->offsets));
+
+   /* Stride and height are aligned to 32 bytes for MB32 tiled format. */
+   luma_stride = ALIGN(args->width, 32);
+   luma_height = ALIGN(args->height, 32);
+
+   if (sun4i_format_is_semiplanar(args->format)) {
+   chroma_stride = luma_stride;
+
+   if (sun4i_format_is_yuv420(args->format))
+   chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 
32);
+   else if (sun4i_format_is_yuv422(args->format))
+   chroma_height = luma_height;
+   else
+   return -EINVAL;
+
+   args->pitches[0] = luma_stride;
+   args->pitches[1] = chroma_stride;
+
+   args->offsets[0] = 0;
+   args->offsets[1] = luma_stride * luma_height;
+
+   args->size = luma_stride * luma_height +
+chroma_stride * chroma_height;
+   } else if (sun4i_format_is_planar(args->format)) {
+   if (sun4i_format_is_yuv411(args->format)) {
+   chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 4), 32);
+   chroma_height = luma_height;
+   } if (sun4i_format_is_yuv420(args->format)) {
+   chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
+   chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 
32);
+   } else if (sun4i_format_is_yuv422(args->format)) {
+   chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
+   chroma_height = luma_height;
+   } else {
+   return -EINVAL;
+   }
+
+   args->pitches[0] = luma_stride;
+   args->pitches[1] = chroma_stride;
+   args->pitches[2] = chroma_stride;
+
+   args->offsets[0] = 0;
+   args->offsets[1] = luma_stride * luma_height;
+   args->offsets[2] = luma_stride * luma_height +
+  chroma_stride * chroma_height;
+
+   args->size = luma_stride * luma_height +
+chroma_stride * chroma_height * 2;
+   } else {
+   /* No support for packed formats in tiled mode. */
+   return -EINVAL;
+   }
+
+   cma_obj = drm_gem_cma_create(drm, args->size);
+   if (IS_ERR(cma_obj))
+   return PTR_ERR(cma_obj);
+
+   gem_obj = _obj->base;
+
+   /*
+* allocate a id of idr table where the obj is registered
+* and handle has the id what user can see.
+*/
+