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 <paul.kocialkowski@xxxxxxxxxxx>
> > ---
> > 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 <drm/drm_gem_cma_helper.h>
> > #include <drm/drm_fb_helper.h>
> > #include <drm/drm_of.h>
> > +#include <drm/sun4i_drm.h>
> >
> > #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 = &sun4i_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 = &cma_obj->base;
> > +
> > + /*
> > + * allocate a id of idr table where the obj is registered
> > + * and handle has the id what user can see.
> > + */
> > + ret = drm_gem_handle_create(file_priv, gem_obj, &args-
> > >handle);
> > + /* drop reference from allocate - handle holds it now. */
> > + drm_gem_object_put_unlocked(gem_obj);
> > + if (ret)
> > + return ret;
> > +
> > + return PTR_ERR_OR_ZERO(cma_obj);
> > +}
> > +
> > static void sun4i_remove_framebuffers(void)
> > {
> > struct apertures_struct *ap;
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h
> > b/drivers/gpu/drm/sun4i/sun4i_drv.h
> > index 47969711a889..308ff4bfcdd5 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> > @@ -26,5 +26,7 @@ struct sun4i_drv {
> > int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> > struct drm_device *drm,
> > struct drm_mode_create_dumb *args);
> > +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.
> I'll let the DRM-misc maintainers comment on the validity of the new
> ioctl.
Cheers,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.comAttachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
