[PATCH 2/6] drm/exynos: ipp: Add IPP v2 framework

[Emil Velikov]

Hi Marek,

Just a couple of small suggestions - this is by no means a proper review.

On 12 September 2017 at 09:08, Marek Szyprowski
<m.szyprowski at samsung.com> wrote:
> This patch adds Exynos IPP v2 subsystem and userspace API.
>
> New userspace API is focused ONLY on memory-to-memory image processing.
> The two remainging IPP operation modes (framebuffer writeback and
s/remainging/remaining/

> local-path output with image processing) can be implemented using
> standard DRM features: writeback connectors and additional DRM planes with
> scaling features.
>
> V2 IPP userspace API is not compatible with old IPP ioctls. This is a
> significant change, but the old IPP subsystem in mainline Linux kernel was
> partially disfunctional anyway and not used in any open-source project.
>
s/disfunctional/dysfunctional/

> V2 IPP userspace API is based on stateless approach, which much better fits
> to memory-to-memory image processing model. It also provides support for
> all image formats, which are both already defined in DRM API and supported
> by the existing IPP hardware modules.
>
> The API consists of the following ioctls:
> - DRM_IOCTL_EXYNOS_IPP_GET_RESOURCES: to enumerate all available image
>   processing modules,
> - DRM_IOCTL_EXYNOS_IPP_GET_CAPS: to query capabilities and supported image
>   formats of given IPP module,
> - DRM_IOCTL_EXYNOS_IPP_GET_LIMITS: to query hardware limitiations for
s/limitiations/limitations/


> +int exynos_drm_ipp_get_limits_ioctl(struct drm_device *dev, void *data,
> +                                   struct drm_file *file_priv)
> +{
> +       struct drm_exynos_ioctl_ipp_get_limits *resp = data;
> +       void __user *ptr = (void __user *)(unsigned long)resp->limits_ptr;
We have the u64_to_user_ptr helper for these casts. Please use it
through the codebase.


> +struct exynos_drm_param_map {
static const?

> +       unsigned int id;
> +       unsigned int size;
> +       unsigned int offset;
> +} exynos_drm_ipp_params_maps[] = {
> +       {
> +               DRM_EXYNOS_IPP_TASK_BUFFER | DRM_EXYNOS_IPP_TASK_TYPE_SOURCE,
> +               sizeof(struct drm_exynos_ipp_task_buffer),
> +               offsetof(struct exynos_drm_ipp_task, src.buf),
> +       }, {
> +               DRM_EXYNOS_IPP_TASK_BUFFER | DRM_EXYNOS_IPP_TASK_TYPE_DESTINATION,
> +               sizeof(struct drm_exynos_ipp_task_buffer),
> +               offsetof(struct exynos_drm_ipp_task, dst.buf),
> +       }, {
> +               DRM_EXYNOS_IPP_TASK_RECTANGLE | DRM_EXYNOS_IPP_TASK_TYPE_SOURCE,
> +               sizeof(struct drm_exynos_ipp_task_rect),
> +               offsetof(struct exynos_drm_ipp_task, src.rect),
> +       }, {
> +               DRM_EXYNOS_IPP_TASK_RECTANGLE | DRM_EXYNOS_IPP_TASK_TYPE_DESTINATION,
> +               sizeof(struct drm_exynos_ipp_task_rect),
> +               offsetof(struct exynos_drm_ipp_task, dst.rect),
> +       }, {
> +               DRM_EXYNOS_IPP_TASK_TRANSFORM,
> +               sizeof(struct drm_exynos_ipp_task_transform),
> +               offsetof(struct exynos_drm_ipp_task, transform),
> +       }, {
> +               DRM_EXYNOS_IPP_TASK_ALPHA,
> +               sizeof(struct drm_exynos_ipp_task_alpha),
> +               offsetof(struct exynos_drm_ipp_task, alpha),
> +       },
> +};
> +
> +static int exynos_drm_ipp_task_set(struct exynos_drm_ipp_task *task,
> +                                 struct drm_exynos_ioctl_ipp_commit *arg)
> +{
> +       unsigned int size = arg->params_size;
> +       void *p, *params;
> +       int ret = 0;
> +
> +       if (size > PAGE_SIZE)
> +               return -ERANGE;
> +
> +       p = kmalloc(size, GFP_KERNEL);
> +       if (!p)
> +               return -ENOMEM;
> +
> +       if (copy_from_user(p, (void __user *)(unsigned long)arg->params_ptr,
> +                          size)) {
> +               ret = -EFAULT;
> +               DRM_DEBUG_DRIVER("Failed to copy configuration from userspace\n");
> +               goto free;
> +       }
> +
> +       params = p;
> +       while (size) {
> +               struct exynos_drm_param_map *map = exynos_drm_ipp_params_maps;
I'd just drop this since...

> +               u32 *id = params;
> +               int i;
> +
> +               for (i = 0; i < ARRAY_SIZE(exynos_drm_ipp_params_maps); i++)
... using ARRAY_SIZE(foo) only to access bar[] is quite misleading.

> +                       if (map[i].id == *id)
> +                               break;
> +
> +               if (i < ARRAY_SIZE(exynos_drm_ipp_params_maps) &&
> +                   map[i].size <= size) {
> +                       memcpy((void *)task + map[i].offset, params,
> +                              map[i].size);
> +                       params += map[i].size;
> +                       size -= map[i].size;
> +               } else {
> +                       ret = -EINVAL;
> +                       goto free;
> +               }
Maybe flip the condition order?
if (!foo)
   ret = -EINVAL;
   goto xx;

map[i]...

> +       }
> +
> +       DRM_DEBUG_DRIVER("Got task %pK configuration from userspace\n", task);
> +free:
> +       kfree(p);
> +       return ret;
> +}
> +


> +
> +struct drm_exynos_ipp_limit_val {
> +       __u32 min, max, align;
Having these one per line, will make it harder to miss alignment issues.


> +struct drm_exynos_ipp_limit {
> +       __u32 type;
> +       struct drm_exynos_ipp_limit_val h;
> +       struct drm_exynos_ipp_limit_val v;
If you extend the struct by appending 64bit variable it will break
compat ioctls.
One could add a note/warning above the struct?

> +struct drm_exynos_ipp_task_rect {
> +       __u32   id;
> +       __u32   x, y, w, h;
Ditto - separate lines, note about future compat ioctl breakage?

HTH
Emil