Hi Emil,
On 2017-09-12 17:02, Emil Velikov wrote:
Hi Marek,
Just a couple of small suggestions - this is by no means a proper review.
Thanks, I really appreciate your suggestions! I've tried to make userspace API 32/64bit agnostic, but it looks that I still need to learn a bit in this area.
On 12 September 2017 at 09:08, Marek Szyprowski m.szyprowski@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
Best regards