[RFC 0/4] Exynos DRM: add Picture Processor extension

Thu Apr 20 12:17:45 UTC 2017

Hello everyone,

Marek Szyprowski wrote:
> Hi Laurent,
> 
> On 2017-04-20 12:25, Laurent Pinchart wrote:
>> Hi Marek,
>>
>> (CC'ing Sakari Ailus)
>>
>> Thank you for the patches.
>>
>> On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
>>> Dear all,
>>>
>>> This is an updated proposal for extending EXYNOS DRM API with generic
>>> support for hardware modules, which can be used for processing image
>>> data
>>> from the one memory buffer to another. Typical memory-to-memory
>>> operations
>>> are: rotation, scaling, colour space conversion or mix of them. This
>>> is a
>>> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
>>> processors", which has been rejected as "not really needed in the DRM
>>> core":
>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>>>
>>>
>>> In this proposal I moved all the code to Exynos DRM driver, so now this
>>> will be specific only to Exynos DRM. I've also changed the name from
>>> framebuffer processor (fbproc) to picture processor (pp) to avoid
>>> confusion
>>> with fbdev API.
>>>
>>> Here is a bit more information what picture processors are:
>>>
>>> Embedded SoCs are known to have a number of hardware blocks, which
>>> perform
>>> such operations. They can be used in paralel to the main GPU module to
>>> offload CPU from processing grapics or video data. One of example use of
>>> such modules is implementing video overlay, which usually requires color
>>> space conversion from NV12 (or similar) to RGB32 color space and
>>> scaling to
>>> target window size.
>>>
>>> The proposed API is heavily inspired by atomic KMS approach - it is also
>>> based on DRM objects and their properties. A new DRM object is
>>> introduced:
>>> picture processor (called pp for convenience). Such objects have a
>>> set of
>>> standard DRM properties, which describes the operation to be
>>> performed by
>>> respective hardware module. In typical case those properties are a
>>> source
>>> fb id and rectangle (x, y, width, height) and destination fb id and
>>> rectangle. Optionally a rotation property can be also specified if
>>> supported by the given hardware. To perform an operation on image data,
>>> userspace provides a set of properties and their values for given fbproc
>>> object in a similar way as object and properties are provided for
>>> performing atomic page flip / mode setting.
>>>
>>> The proposed API consists of the 3 new ioctls:
>>> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
>>>    processors,
>>> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
>>>    processor,
>>> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
>>>    property set.
>>>
>>> The proposed API is extensible. Drivers can attach their own, custom
>>> properties to add support for more advanced picture processing (for
>>> example
>>> blending).
>>>
>>> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
>>> subsystem. IPP API is over-engineered in general, but not really
>>> extensible
>>> on the other side. It is also buggy, with significant design flaws - the
>>> biggest issue is the fact that the API covers memory-2-memory picture
>>> operations together with CRTC writeback and duplicating features, which
>>> belongs to video plane. Comparing with IPP subsystem, the PP
>>> framework is
>>> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
>>> rotator driver smaller by over 200 lines).
>> This seems to be the kind of hardware that is typically supported by
>> V4L2.
>> Stupid question, why DRM ?
> 
> Let me elaborate a bit on the reasons for implementing it in Exynos DRM:
> 
> 1. we want to replace existing Exynos IPP subsystem:
>  - it is used only in some internal/vendor trees, not in open-source
>  - we want it to have sane and potentially extensible userspace API
>  - but we don't want to loose its functionality
> 
> 2. we want to have simple API for performing single image processing
> operation:
>  - typically it will be used by compositing window manager, this means that
>    some parameters of the processing might change on each vblank (like
>    destination rectangle for example). This api allows such change on each
>    operation without any additional cost. V4L2 requires to reinitialize
>    queues with new configuration on such change, what means that a bunch of
>    ioctls has to be called.
>  - validating processing parameters in V4l2 API is really complicated,
>    because the parameters (format, src&dest rectangles, rotation) are being
>    set incrementally, so we have to either allow some impossible,
> transitional
>    configurations or complicate the configuration steps even more (like
>    calling some ioctls multiple times for both input and output). In the
> end
>    all parameters have to be again validated just before performing the
>    operation.
> 
> 3. generic approach (to add it to DRM core) has been rejected:
> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
> 
> 4. this api can be considered as extended 'blit' operation, other DRM
> drivers
>    (MGA, R128, VIA) already have ioctls for such operation, so there is
> also
>    place in DRM for it
> 
I just wanted to say that I like the proposed API. I could do some
reviewing, if the API isn't discarded quickly like last time (when it
was still proposed as part of DRM core).

To comment about why this should be in DRM land. I personally could
never get comfortable with the V4L2 API. It seems to be overengineered
for the context of a blitting op, hence what Marek already said. I
prefer to work with one API, so I don't have to worry about potential
problems when two of them interact. For V4L2 this would be exchange of
buffers, where I always have to think where I have to create some
buffer, because I might be able to export it, but not import it on the
other side. In this case DRM is way simpler, I just pass around my GEM
handle and that's it. Stuff like that.

Also, I still haven't dropped this small project of mine, using such an
API for an Exynos renderer backend for the mpv media player.

With best wishes,
Tobias

>>> Open questions:
>>> - How to expose pp capabilities and supported formats? Currently this is
>>> done with a drm_exynos_pp_get structure and DRM_IOCTL_EXYNOS_PP_GET
>>> ioctl.
>>> However one can try to use IMMUTABLE properties for capabilities and
>>> src/dst format set. Rationale: recently Rob Clark proposed to create
>>> a DRM
>>> property with supported pixelformats and modifiers:
>>>    http://www.spinics.net/lists/dri-devel/msg137380.html
>>> - Is it okay to use DRM objects and properties API
>>> (DRM_IOCTL_MODE_GETPROPERTY and DRM_IOCTL_MODE_OBJ_GETPROPERTIES ioctls)
>>> for this purpose?
>>>
>>> TODO:
>>> - convert remaining Exynos DRM IPP drivers (FIMC, GScaller)
>>> - remove Exynos DRM IPP subsystem
>>> - (optional) provide virtual V4L2 mem2mem device on top of Exynos PP
>>> framework
>>>
>>> Patches were tested on Exynos 4412-based Odroid U3 board, on top of
>>> Linux
>>> next-20170420 kernel.
>>>
>>> Best regards
>>> Marek Szyprowski
>>> Samsung R&D Institute Poland
>>>
>>>
>>> Changelog:
>>> v1:
>>> - moved this feature from DRM core to Exynos DRM driver
>>> - changed name from framebuffer processor to picture processor
>>> - simplified code to cover only things needed by Exynos drivers
>>> - implemented simple fifo task scheduler
>>> - cleaned up rotator driver conversion (removed IPP remainings)
>>>
>>>
>>> v0:
>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>>>
>>> - initial post of "[RFC 0/2] New feature: Framebuffer processors"
>>> - generic approach implemented in DRM core, rejected
>>>
>>>
>>> Patch summary:
>>>
>>> Marek Szyprowski (4):
>>>    drm: Export functions to create custom DRM objects
>>>    drm: Add support for vendor specific DRM objects with custom
>>>      properties
>>>    drm/exynos: Add Picture Processor framework
>>>    drm/exynos: Convert Exynos Rotator driver to Picture Processor
>>>      interface
>>>
>>>   drivers/gpu/drm/drm_crtc_internal.h         |   4 -
>>>   drivers/gpu/drm/drm_mode_object.c           |  11 +-
>>>   drivers/gpu/drm/drm_property.c              |   2 +-
>>>   drivers/gpu/drm/exynos/Kconfig              |   1 -
>>>   drivers/gpu/drm/exynos/Makefile             |   3 +-
>>>   drivers/gpu/drm/exynos/exynos_drm_drv.c     |   9 +
>>>   drivers/gpu/drm/exynos/exynos_drm_drv.h     |  15 +
>>>   drivers/gpu/drm/exynos/exynos_drm_pp.c      | 775
>>> +++++++++++++++++++++++++
>>>   drivers/gpu/drm/exynos/exynos_drm_pp.h      | 155 ++++++
>>>   drivers/gpu/drm/exynos/exynos_drm_rotator.c | 513 +++++-------------
>>>   drivers/gpu/drm/exynos/exynos_drm_rotator.h |  19 -
>>>   include/drm/drm_mode_object.h               |   6 +
>>>   include/drm/drm_property.h                  |   7 +
>>>   include/uapi/drm/drm_mode.h                 |   1 +
>>>   include/uapi/drm/exynos_drm.h               |  62 +++
>>>   15 files changed, 1166 insertions(+), 417 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/exynos/exynos_drm_pp.c
>>>   create mode 100644 drivers/gpu/drm/exynos/exynos_drm_pp.h
>>>   delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_rotator.h
> 
> Best regards