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@xxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
