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
