mbox series

[0/6] Exynos DRM: rewrite IPP subsystem and userspace API

Message ID 1505203739-29747-1-git-send-email-m.szyprowski@samsung.com
Headers show
Series Exynos DRM: rewrite IPP subsystem and userspace API | expand

Message

Marek Szyprowski Sept. 12, 2017, 8:08 a.m. UTC
Dear all,

This patchset performs complete rewrite of Exynos DRM IPP subsystem and
its userspace API.

Why such rewrite is needed? Exynos DRM IPP API is over-engineered in
general, but not really extensible on the other side. It is also buggy,
with significant design flaws:
- Userspace API covers memory-2-memory picture operations together with
  CRTC writeback and duplicating features, which belongs to video plane.
- Lack of support of the all required image formats (for example NV12
  Samsung-tiled cannot be used due to lack of pixel format modifier
  support).
- Userspace API designed only to mimic hardware behaviour, not easy to
  understand.
- Lack of proper input validation in the core, drivers also didn't do that
  correctly, so it was possible to set incorrect parameters and easil
  trigger IOMMU fault or memory trash.
- Drivers were partially disfunctional or supported only a subset of modes.

Due to the above limitations and issues the Exynos DRM IPP API was not
used by any of the open-source projects. I assume that it is safe to remove
this broken API without any damage to open-source community. All remaining
users (mainly Tizen project related) will be updated to the new version.

This patchset changes Exynos DRM IPP subsystem to something useful. The
userspace API is much simpler, state-less and easy to understand. Also
the code of the core and driver is significantly smaller and easier to
understand.

Patches were tested on Exynos4412 based Odroid U3 and Exynos5422
Odroid XU3 boards, on top of Linux next-20170911 kernel.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


My previous works in this area:

"[RFC v2 0/2] Exynos DRM: add Picture Processor extension"
https://www.spinics.net/lists/dri-devel/msg140669.html
- removed usage of DRM objects and properties - replaced them with simple
  list of parameters with predefined IDs

"[RFC 0/4] Exynos DRM: add Picture Processor extension"
https://www.spinics.net/lists/linux-samsung-soc/msg59323.html
- 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)

"[RFC 0/2] New feature: Framebuffer processors"
https://www.spinics.net/lists/linux-samsung-soc/msg54810.html
- generic approach implemented in DRM core, rejected


Patch summary:

Marek Szyprowski (6):
  drm/exynos: ipp: Remove Exynos DRM IPP subsystem
  drm/exynos: ipp: Add IPP v2 framework
  drm/exynos: rotator: Convert driver to IPP v2 core API
  drm/exynos: gsc: Convert driver to IPP v2 core API
  drm/exynos: Add generic support for devices shared with V4L2 subsystem
  drm/exynos: fimc: Convert driver to IPP v2 core API

 drivers/gpu/drm/exynos/Kconfig              |   12 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c     |   33 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |    4 +-
 drivers/gpu/drm/exynos/exynos_drm_fimc.c    |  893 +++--------
 drivers/gpu/drm/exynos/exynos_drm_fimc.h    |   23 -
 drivers/gpu/drm/exynos/exynos_drm_gsc.c     |  853 +++-------
 drivers/gpu/drm/exynos/exynos_drm_gsc.h     |   24 -
 drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 2239 ++++++++-------------------
 drivers/gpu/drm/exynos/exynos_drm_ipp.h     |  352 ++---
 drivers/gpu/drm/exynos/exynos_drm_rotator.c |  735 ++-------
 drivers/gpu/drm/exynos/exynos_drm_rotator.h |   19 -
 include/uapi/drm/exynos_drm.h               |  315 ++--
 12 files changed, 1627 insertions(+), 3875 deletions(-)
 delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_fimc.h
 delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_gsc.h
 delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_rotator.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tobias Jakobi Sept. 13, 2017, 11:16 a.m. UTC | #1
Hey Marek,

first of all thanks for keeping this series alive. Looks pretty good so far.

I have started to rewrite my IPP userspace API in libdrm and when this is done,
I'm going to adapt my mpv video backend to it.

With best wishes,
Tobias



Marek Szyprowski wrote:
> Dear all,

> 

> This patchset performs complete rewrite of Exynos DRM IPP subsystem and

> its userspace API.

> 

> Why such rewrite is needed? Exynos DRM IPP API is over-engineered in

> general, but not really extensible on the other side. It is also buggy,

> with significant design flaws:

> - Userspace API covers memory-2-memory picture operations together with

>   CRTC writeback and duplicating features, which belongs to video plane.

> - Lack of support of the all required image formats (for example NV12

>   Samsung-tiled cannot be used due to lack of pixel format modifier

>   support).

> - Userspace API designed only to mimic hardware behaviour, not easy to

>   understand.

> - Lack of proper input validation in the core, drivers also didn't do that

>   correctly, so it was possible to set incorrect parameters and easil

>   trigger IOMMU fault or memory trash.

> - Drivers were partially disfunctional or supported only a subset of modes.

> 

> Due to the above limitations and issues the Exynos DRM IPP API was not

> used by any of the open-source projects. I assume that it is safe to remove

> this broken API without any damage to open-source community. All remaining

> users (mainly Tizen project related) will be updated to the new version.

> 

> This patchset changes Exynos DRM IPP subsystem to something useful. The

> userspace API is much simpler, state-less and easy to understand. Also

> the code of the core and driver is significantly smaller and easier to

> understand.

> 

> Patches were tested on Exynos4412 based Odroid U3 and Exynos5422

> Odroid XU3 boards, on top of Linux next-20170911 kernel.

> 

> Best regards

> Marek Szyprowski

> Samsung R&D Institute Poland

> 

> 

> My previous works in this area:

> 

> "[RFC v2 0/2] Exynos DRM: add Picture Processor extension"

> https://www.spinics.net/lists/dri-devel/msg140669.html

> - removed usage of DRM objects and properties - replaced them with simple

>   list of parameters with predefined IDs

> 

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

> https://www.spinics.net/lists/linux-samsung-soc/msg59323.html

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

> 

> "[RFC 0/2] New feature: Framebuffer processors"

> https://www.spinics.net/lists/linux-samsung-soc/msg54810.html

> - generic approach implemented in DRM core, rejected

> 

> 

> Patch summary:

> 

> Marek Szyprowski (6):

>   drm/exynos: ipp: Remove Exynos DRM IPP subsystem

>   drm/exynos: ipp: Add IPP v2 framework

>   drm/exynos: rotator: Convert driver to IPP v2 core API

>   drm/exynos: gsc: Convert driver to IPP v2 core API

>   drm/exynos: Add generic support for devices shared with V4L2 subsystem

>   drm/exynos: fimc: Convert driver to IPP v2 core API

> 

>  drivers/gpu/drm/exynos/Kconfig              |   12 +-

>  drivers/gpu/drm/exynos/exynos_drm_drv.c     |   33 +-

>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |    4 +-

>  drivers/gpu/drm/exynos/exynos_drm_fimc.c    |  893 +++--------

>  drivers/gpu/drm/exynos/exynos_drm_fimc.h    |   23 -

>  drivers/gpu/drm/exynos/exynos_drm_gsc.c     |  853 +++-------

>  drivers/gpu/drm/exynos/exynos_drm_gsc.h     |   24 -

>  drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 2239 ++++++++-------------------

>  drivers/gpu/drm/exynos/exynos_drm_ipp.h     |  352 ++---

>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  735 ++-------

>  drivers/gpu/drm/exynos/exynos_drm_rotator.h |   19 -

>  include/uapi/drm/exynos_drm.h               |  315 ++--

>  12 files changed, 1627 insertions(+), 3875 deletions(-)

>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_fimc.h

>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_gsc.h

>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_rotator.h

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tobias Jakobi Sept. 15, 2017, 6:33 p.m. UTC | #2
Hello everyone,

just finished a first draft of the libdrm API and some test application:
https://github.com/tobiasjakobi/libdrm/blob/ippv2/exynos/exynos_ipp.h
https://github.com/tobiasjakobi/libdrm/blob/ippv2/tests/exynos/exynos_ipp_test.c

Needs a small patch to the kernel to prevent an Oops when a format has no limits.

Does not a lot atm. Just pretty-printing the information from the kernel driver
and testing a simple CSC queue.

Now looking to using this for the mpv vo backend.

With best wishes,
Tobias


P.S.: Output from my system below.

> exynos/ipp: 3 IPP modules found.

> IPP module number 0:

> 	id = 0

> 	supports crop

> 	supports rotate

> 	supports scale

> 	supports convert

> a total of 15 formats are supported:

> 	fourcc: XR24, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: RG24, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: RG16, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV12, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV16, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV21, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV61, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: UYVY, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: VYUY, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YUYV, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YVYU, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YU12, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YV12, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YU16, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YU24, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> IPP module number 1:

> 	id = 1

> 	supports crop

> 	supports rotate

> 	supports scale

> 	supports convert

> a total of 15 formats are supported:

> 	fourcc: XR24, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: RG24, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: RG16, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV12, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV16, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV21, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: NV61, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: UYVY, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: VYUY, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YUYV, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YVYU, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YU12, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YV12, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YU16, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> 	fourcc: YU24, type: source+destination, modifier: 0x0

> 		format has no source limits

> 		format has no destination limits

> IPP module number 2:

> 	id = 2

> 	supports crop

> 	supports rotate

> a total of 2 formats are supported:

> 	fourcc: XR24, type: source+destination, modifier: 0x0

> 		source limit: type = size (horizontal/vertical) in pixels, size = image buffer area

> 		h_min = 8, h_max = 8192, h_align = 0

> 		v_min = 8, v_max = 8192, v_align = 0

> 		source limit: type = size (horizontal/vertical) in pixels, size = rectangle area

> 		h_min = 0, h_max = 0, h_align = 4

> 		v_min = 0, v_max = 0, v_align = 4

> 		destination limit: type = size (horizontal/vertical) in pixels, size = image buffer area

> 		h_min = 8, h_max = 8192, h_align = 0

> 		v_min = 8, v_max = 8192, v_align = 0

> 		destination limit: type = size (horizontal/vertical) in pixels, size = rectangle area

> 		h_min = 0, h_max = 0, h_align = 4

> 		v_min = 0, v_max = 0, v_align = 4

> 	fourcc: NV12, type: source+destination, modifier: 0x0

> 		source limit: type = size (horizontal/vertical) in pixels, size = image buffer area

> 		h_min = 32, h_max = 32768, h_align = 0

> 		v_min = 32, v_max = 32768, v_align = 0

> 		source limit: type = size (horizontal/vertical) in pixels, size = rectangle area

> 		h_min = 0, h_max = 0, h_align = 8

> 		v_min = 0, v_max = 0, v_align = 8

> 		destination limit: type = size (horizontal/vertical) in pixels, size = image buffer area

> 		h_min = 32, h_max = 32768, h_align = 0

> 		v_min = 32, v_max = 32768, v_align = 0

> 		destination limit: type = size (horizontal/vertical) in pixels, size = rectangle area

> 		h_min = 0, h_max = 0, h_align = 8

> 		v_min = 0, v_max = 0, v_align = 8

> IPP handler:

> 	fd = 3, ipp_id = 0, seq = 29, tv_sec = 2659, tv_usec = 709341

> 	user_data = 0x0xc47a30

> IPP handler:

> 	fd = 3, ipp_id = 0, seq = 30, tv_sec = 2659, tv_usec = 711837

> 	user_data = 0x0xc47a40

> IPP handler:

> 	fd = 3, ipp_id = 0, seq = 31, tv_sec = 2659, tv_usec = 714027

> 	user_data = 0x0xc47a50





Marek Szyprowski wrote:
> Dear all,

> 

> This patchset performs complete rewrite of Exynos DRM IPP subsystem and

> its userspace API.

> 

> Why such rewrite is needed? Exynos DRM IPP API is over-engineered in

> general, but not really extensible on the other side. It is also buggy,

> with significant design flaws:

> - Userspace API covers memory-2-memory picture operations together with

>   CRTC writeback and duplicating features, which belongs to video plane.

> - Lack of support of the all required image formats (for example NV12

>   Samsung-tiled cannot be used due to lack of pixel format modifier

>   support).

> - Userspace API designed only to mimic hardware behaviour, not easy to

>   understand.

> - Lack of proper input validation in the core, drivers also didn't do that

>   correctly, so it was possible to set incorrect parameters and easil

>   trigger IOMMU fault or memory trash.

> - Drivers were partially disfunctional or supported only a subset of modes.

> 

> Due to the above limitations and issues the Exynos DRM IPP API was not

> used by any of the open-source projects. I assume that it is safe to remove

> this broken API without any damage to open-source community. All remaining

> users (mainly Tizen project related) will be updated to the new version.

> 

> This patchset changes Exynos DRM IPP subsystem to something useful. The

> userspace API is much simpler, state-less and easy to understand. Also

> the code of the core and driver is significantly smaller and easier to

> understand.

> 

> Patches were tested on Exynos4412 based Odroid U3 and Exynos5422

> Odroid XU3 boards, on top of Linux next-20170911 kernel.

> 

> Best regards

> Marek Szyprowski

> Samsung R&D Institute Poland

> 

> 

> My previous works in this area:

> 

> "[RFC v2 0/2] Exynos DRM: add Picture Processor extension"

> https://www.spinics.net/lists/dri-devel/msg140669.html

> - removed usage of DRM objects and properties - replaced them with simple

>   list of parameters with predefined IDs

> 

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

> https://www.spinics.net/lists/linux-samsung-soc/msg59323.html

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

> 

> "[RFC 0/2] New feature: Framebuffer processors"

> https://www.spinics.net/lists/linux-samsung-soc/msg54810.html

> - generic approach implemented in DRM core, rejected

> 

> 

> Patch summary:

> 

> Marek Szyprowski (6):

>   drm/exynos: ipp: Remove Exynos DRM IPP subsystem

>   drm/exynos: ipp: Add IPP v2 framework

>   drm/exynos: rotator: Convert driver to IPP v2 core API

>   drm/exynos: gsc: Convert driver to IPP v2 core API

>   drm/exynos: Add generic support for devices shared with V4L2 subsystem

>   drm/exynos: fimc: Convert driver to IPP v2 core API

> 

>  drivers/gpu/drm/exynos/Kconfig              |   12 +-

>  drivers/gpu/drm/exynos/exynos_drm_drv.c     |   33 +-

>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |    4 +-

>  drivers/gpu/drm/exynos/exynos_drm_fimc.c    |  893 +++--------

>  drivers/gpu/drm/exynos/exynos_drm_fimc.h    |   23 -

>  drivers/gpu/drm/exynos/exynos_drm_gsc.c     |  853 +++-------

>  drivers/gpu/drm/exynos/exynos_drm_gsc.h     |   24 -

>  drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 2239 ++++++++-------------------

>  drivers/gpu/drm/exynos/exynos_drm_ipp.h     |  352 ++---

>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  735 ++-------

>  drivers/gpu/drm/exynos/exynos_drm_rotator.h |   19 -

>  include/uapi/drm/exynos_drm.h               |  315 ++--

>  12 files changed, 1627 insertions(+), 3875 deletions(-)

>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_fimc.h

>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_gsc.h

>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_rotator.h

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html