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

[Marek Szyprowski]

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 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
>
>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland