This introduces support for YUV formats in the sun4i DRM driver, through
the frontend. In addition to regular YUV formats, a modifier for the
Allwinner MB32 tiling format is introduced along with a dedicated ioctl
for allocating buffers (through CMA) with the appropriate constraints.
This ioctl must always be used when allocating buffers to be used with
the MB32 tiling modifier, as dumb GEM buffer allocation is reserved for
linear planes.
This series is based on (and requires) the following series:
* drm/sun4i: backend: Support interleaved YUV planes,
from Maxime Ripard: https://patchwork.freedesktop.org/series/39232/
* drm/sun4i: Support the Display Engine frontend,
from Maxime Ripard: https://patchwork.freedesktop.org/series/35292/
* drm/sun4i: Support more planes, zpos and plane-wide alpha,
from Maxime Ripard: https://patchwork.freedesktop.org/series/36183/
Paul Kocialkowski (10):
drm/sun4i: Disable frontend video channel before enabling a layer
drm/sun4i: Disable YUV channel when using the frontend and set
interlace
drm/sun4i: Don't pretend to handle ARGB8888 with the frontend
drm/sun4i: Explicitly list and check formats supported by the backend
drm/sun4i: Explicitly list and check formats supported by the frontend
drm/sun4i: Move and extend format-related helpers and tables
drm/sun4i: Add support for YUV formats through the frontend
drm/fourcc: Add definitions for Allwinner vendor and MB32 tiled format
drm/sun4i: Add a dedicated ioctl call for allocating tiled buffers
drm/sun4i: Add support for YUV-based formats in MB32 tiles
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_backend.c | 148 +++++++++-----
drivers/gpu/drm/sun4i/sun4i_backend.h | 6 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 108 +++++++++-
drivers/gpu/drm/sun4i/sun4i_drv.h | 6 +
drivers/gpu/drm/sun4i/sun4i_format.c | 193 ++++++++++++++++++
drivers/gpu/drm/sun4i/sun4i_format.h | 35 ++++
drivers/gpu/drm/sun4i/sun4i_frontend.c | 359 +++++++++++++++++++++++++++++----
drivers/gpu/drm/sun4i/sun4i_frontend.h | 50 ++++-
drivers/gpu/drm/sun4i/sun4i_layer.c | 58 ++++--
include/uapi/drm/drm_fourcc.h | 10 +
include/uapi/drm/sun4i_drm.h | 42 ++++
12 files changed, 910 insertions(+), 106 deletions(-)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.c
create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.h
create mode 100644 include/uapi/drm/sun4i_drm.h
--
2.16.2
This moves the various helpers and tables related to format detection
and conversion to a dedicated file, while adding a bunch of new helpers
(especially for YUV and tiling support) along the way.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_backend.c | 54 ++--------
drivers/gpu/drm/sun4i/sun4i_format.c | 193 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/sun4i/sun4i_format.h | 35 ++++++
4 files changed, 235 insertions(+), 48 deletions(-)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.c
create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.h
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index 582607c0c488..c89c42ff803e 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -4,6 +4,7 @@ sun4i-frontend-y += sun4i_frontend.o
sun4i-drm-y += sun4i_drv.o
sun4i-drm-y += sun4i_framebuffer.o
+sun4i-drm-y += sun4i_format.o
sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 1fad0714c70e..e8af9f3cf20b 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -29,6 +29,7 @@
#include "sun4i_drv.h"
#include "sun4i_frontend.h"
#include "sun4i_layer.h"
+#include "sun4i_format.h"
#include "sunxi_engine.h"
struct sun4i_backend_quirks {
@@ -36,50 +37,6 @@ struct sun4i_backend_quirks {
bool needs_output_muxing;
};
-static const u32 sunxi_rgb2yuv_coef[12] = {
- 0x00000107, 0x00000204, 0x00000064, 0x00000108,
- 0x00003f69, 0x00003ed6, 0x000001c1, 0x00000808,
- 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
-};
-
-static const u32 sunxi_bt601_yuv2rgb_coef[12] = {
- 0x000004a7, 0x00001e6f, 0x00001cbf, 0x00000877,
- 0x000004a7, 0x00000000, 0x00000662, 0x00003211,
- 0x000004a7, 0x00000812, 0x00000000, 0x00002eb1,
-};
-
-static inline bool sun4i_backend_format_is_planar_yuv(uint32_t format)
-{
- switch (format) {
- case DRM_FORMAT_YUV411:
- case DRM_FORMAT_YUV422:
- case DRM_FORMAT_YUV444:
- return true;
- default:
- return false;
- }
-}
-
-static inline bool sun4i_backend_format_is_packed_yuv422(uint32_t format)
-{
- switch (format) {
- case DRM_FORMAT_YUYV:
- case DRM_FORMAT_YVYU:
- case DRM_FORMAT_UYVY:
- case DRM_FORMAT_VYUY:
- return true;
-
- default:
- return false;
- }
-}
-
-static inline bool sun4i_backend_format_is_yuv(uint32_t format)
-{
- return sun4i_backend_format_is_planar_yuv(format) ||
- sun4i_backend_format_is_packed_yuv422(format);
-}
-
static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
{
int i;
@@ -259,7 +216,7 @@ static int sun4i_backend_update_yuv_format(struct sun4i_backend *backend,
SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN,
SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN);
- if (sun4i_backend_format_is_packed_yuv422(format))
+ if (sun4i_format_is_packed_yuv422(format))
val |= SUN4I_BACKEND_IYUVCTL_FBFMT_PACKED_YUV422;
else
DRM_DEBUG_DRIVER("Unknown YUV format\n");
@@ -310,7 +267,7 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
DRM_DEBUG_DRIVER("Switching display backend interlaced mode %s\n",
interlaced ? "on" : "off");
- if (sun4i_backend_format_is_yuv(fb->format->format))
+ if (sun4i_format_is_yuv(fb->format->format))
return sun4i_backend_update_yuv_format(backend, layer, plane);
ret = sun4i_backend_drm_format_to_layer(fb->format->format, &val);
@@ -404,7 +361,7 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
*/
paddr -= PHYS_OFFSET;
- if (sun4i_backend_format_is_yuv(fb->format->format))
+ if (sun4i_format_is_yuv(fb->format->format))
return sun4i_backend_update_yuv_buffer(backend, fb, paddr);
/* Write the 32 lower bits of the address (in bits) */
@@ -549,10 +506,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
DRM_DEBUG_DRIVER("Plane FB format is %s\n",
drm_get_format_name(fb->format->format,
&format_name));
+
if (fb->format->has_alpha)
num_alpha_planes++;
- if (sun4i_backend_format_is_yuv(fb->format->format)) {
+ if (sun4i_format_is_yuv(fb->format->format)) {
DRM_DEBUG_DRIVER("Plane FB format is YUV\n");
num_yuv_planes++;
}
diff --git a/drivers/gpu/drm/sun4i/sun4i_format.c b/drivers/gpu/drm/sun4i/sun4i_format.c
new file mode 100644
index 000000000000..3830767e6d5b
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_format.c
@@ -0,0 +1,193 @@
+/*
+ * Copyright (C) 2015-2018 Free Electrons/Bootlin
+ *
+ * Maxime Ripard <[email protected]>
+ * Paul Kocialkowski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/component.h>
+#include <linux/kfifo.h>
+#include <linux/of_graph.h>
+#include <linux/of_reserved_mem.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
+
+#include "sun4i_drv.h"
+#include "sun4i_format.h"
+
+const u32 sunxi_rgb2yuv_coef[12] = {
+ 0x00000107, 0x00000204, 0x00000064, 0x00000108,
+ 0x00003f69, 0x00003ed6, 0x000001c1, 0x00000808,
+ 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
+};
+
+const u32 sunxi_bt601_yuv2rgb_coef[12] = {
+ 0x000004a7, 0x00001e6f, 0x00001cbf, 0x00000877,
+ 0x000004a7, 0x00000000, 0x00000662, 0x00003211,
+ 0x000004a7, 0x00000812, 0x00000000, 0x00002eb1,
+};
+
+bool sun4i_format_is_rgb(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_ARGB4444:
+ case DRM_FORMAT_RGBA4444:
+ case DRM_FORMAT_ARGB1555:
+ case DRM_FORMAT_RGBA5551:
+ case DRM_FORMAT_RGB888:
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_yuv(uint32_t format)
+{
+ return sun4i_format_is_yuv411(format) ||
+ sun4i_format_is_yuv420(format) ||
+ sun4i_format_is_yuv422(format) ||
+ sun4i_format_is_yuv444(format);
+}
+
+bool sun4i_format_is_yuv411(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_YUV411:
+ case DRM_FORMAT_YVU411:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_yuv420(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_NV12:
+ case DRM_FORMAT_NV21:
+ case DRM_FORMAT_YUV420:
+ case DRM_FORMAT_YVU420:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_yuv422(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_YUYV:
+ case DRM_FORMAT_YVYU:
+ case DRM_FORMAT_UYVY:
+ case DRM_FORMAT_VYUY:
+ case DRM_FORMAT_NV16:
+ case DRM_FORMAT_NV61:
+ case DRM_FORMAT_YUV422:
+ case DRM_FORMAT_YVU422:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_yuv444(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_YUV444:
+ case DRM_FORMAT_YVU444:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_packed(uint32_t format)
+{
+ if (sun4i_format_is_rgb(format))
+ return true;
+
+ switch (format) {
+ case DRM_FORMAT_YUYV:
+ case DRM_FORMAT_YVYU:
+ case DRM_FORMAT_UYVY:
+ case DRM_FORMAT_VYUY:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_semiplanar(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_NV12:
+ case DRM_FORMAT_NV21:
+ case DRM_FORMAT_NV16:
+ case DRM_FORMAT_NV61:
+ case DRM_FORMAT_NV24:
+ case DRM_FORMAT_NV42:
+ return true;
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_is_planar(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_YUV410:
+ case DRM_FORMAT_YVU410:
+ case DRM_FORMAT_YUV411:
+ case DRM_FORMAT_YVU411:
+ case DRM_FORMAT_YUV420:
+ case DRM_FORMAT_YVU420:
+ case DRM_FORMAT_YUV422:
+ case DRM_FORMAT_YVU422:
+ case DRM_FORMAT_YUV444:
+ case DRM_FORMAT_YVU444:
+ return true;
+ default:
+ return false;
+ }
+}
+
+bool sun4i_format_supports_tiling(uint32_t format)
+{
+ switch (format) {
+ /* Semiplanar */
+ case DRM_FORMAT_NV12:
+ case DRM_FORMAT_NV21:
+ case DRM_FORMAT_NV16:
+ case DRM_FORMAT_NV61:
+ /* Planar */
+ case DRM_FORMAT_YUV420:
+ case DRM_FORMAT_YVU420:
+ case DRM_FORMAT_YUV422:
+ case DRM_FORMAT_YVU422:
+ case DRM_FORMAT_YUV411:
+ case DRM_FORMAT_YVU411:
+ return true;
+
+ default:
+ return false;
+ }
+}
diff --git a/drivers/gpu/drm/sun4i/sun4i_format.h b/drivers/gpu/drm/sun4i/sun4i_format.h
new file mode 100644
index 000000000000..1d6364cd3681
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_format.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2015-2018 Free Electrons/Bootlin
+ *
+ * Maxime Ripard <[email protected]>
+ * Paul Kocialkowski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#ifndef _SUN4I_FORMAT_H_
+#define _SUN4I_FORMAT_H_
+
+extern const u32 sunxi_rgb2yuv_coef[12];
+extern const u32 sunxi_bt601_yuv2rgb_coef[12];
+
+bool sun4i_format_is_rgb(uint32_t format);
+bool sun4i_format_is_yuv(uint32_t format);
+bool sun4i_format_is_yuv411(uint32_t format);
+bool sun4i_format_is_yuv420(uint32_t format);
+bool sun4i_format_is_yuv422(uint32_t format);
+bool sun4i_format_is_yuv444(uint32_t format);
+bool sun4i_format_is_packed(uint32_t format);
+bool sun4i_format_is_semiplanar(uint32_t format);
+bool sun4i_format_is_planar(uint32_t format);
+bool sun4i_format_supports_tiling(uint32_t format);
+
+static inline bool sun4i_format_is_packed_yuv422(uint32_t format)
+{
+ return sun4i_format_is_packed(format) && sun4i_format_is_yuv422(format);
+}
+
+#endif /* _SUN4I_FORMAT_H_ */
--
2.16.2
It turns out that the frontend is not capable of preserving the alpha
component (that is always set to 0xff), so only support XRGB8888
instead.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 4 ++++
drivers/gpu/drm/sun4i/sun4i_frontend.c | 3 +--
drivers/gpu/drm/sun4i/sun4i_layer.c | 4 ++--
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index b98dafda52f8..274a1db6fa8e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -440,6 +440,10 @@ static bool sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
if (IS_ERR(backend->frontend))
return false;
+ /*
+ * TODO: Don't use the frontend for x2/x4 scaling and allow RGB formats
+ * with an alpha component then.
+ */
return sun4i_backend_plane_uses_scaler(state);
}
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
index ddf6cfa6dd23..3ea925584891 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
@@ -107,7 +107,7 @@ EXPORT_SYMBOL(sun4i_frontend_update_buffer);
static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32 *val)
{
switch (fmt) {
- case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_XRGB8888:
*val = 5;
return 0;
@@ -120,7 +120,6 @@ static int sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
{
switch (fmt) {
case DRM_FORMAT_XRGB8888:
- case DRM_FORMAT_ARGB8888:
*val = 2;
return 0;
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index eb93df445a10..15238211a61a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -100,9 +100,9 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
sun4i_frontend_update_coord(frontend, plane);
sun4i_frontend_update_buffer(frontend, plane);
sun4i_frontend_update_formats(frontend, plane,
- DRM_FORMAT_ARGB8888);
+ DRM_FORMAT_XRGB8888);
sun4i_backend_update_layer_frontend(backend, layer->id, plane,
- DRM_FORMAT_ARGB8888);
+ DRM_FORMAT_XRGB8888);
sun4i_frontend_enable(frontend);
} else {
sun4i_backend_update_layer_formats(backend, layer->id, plane);
--
2.16.2
The YUV channel was only disabled in sun4i_backend_update_layer_formats,
which is not called when the frontend is selected.
Thus, creating a layer with a YUV format handled by the backend and then
switching to a format that requires the frontend would keep the YUV
channel enabled for the layer.
This explicitly disables the YUV channel for the layer when using the
frontend as well. It also sets the relevant interlace bit, which was
missing in the frontend path as well.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 17 ++++++++++++++++-
drivers/gpu/drm/sun4i/sun4i_backend.h | 3 ++-
drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index e07a33adc51d..b98dafda52f8 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -294,8 +294,10 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
}
int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend,
- int layer, uint32_t fmt)
+ int layer, struct drm_plane *plane,
+ uint32_t fmt)
{
+ bool interlaced = false;
u32 val;
int ret;
@@ -305,11 +307,24 @@ int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend,
return ret;
}
+ /* Clear the YUV mode */
+ regmap_update_bits(backend->engine.regs,
+ SUN4I_BACKEND_ATTCTL_REG0(layer),
+ SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN, 0);
+
regmap_update_bits(backend->engine.regs,
SUN4I_BACKEND_ATTCTL_REG0(layer),
SUN4I_BACKEND_ATTCTL_REG0_LAY_VDOEN,
SUN4I_BACKEND_ATTCTL_REG0_LAY_VDOEN);
+ if (plane->state->crtc)
+ interlaced = plane->state->crtc->state->adjusted_mode.flags
+ & DRM_MODE_FLAG_INTERLACE;
+
+ regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_MODCTL_REG,
+ SUN4I_BACKEND_MODCTL_ITLMOD_EN,
+ interlaced ? SUN4I_BACKEND_MODCTL_ITLMOD_EN : 0);
+
regmap_update_bits(backend->engine.regs,
SUN4I_BACKEND_ATTCTL_REG1(layer),
SUN4I_BACKEND_ATTCTL_REG1_LAY_FBFMT, val);
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index 7ae0f0ffec8c..cb6df2b690c0 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -201,7 +201,8 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
int layer, struct drm_plane *plane);
int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend,
- int layer, uint32_t in_fmt);
+ int layer, struct drm_plane *plane,
+ uint32_t fmt);
int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend,
int layer, struct drm_plane *plane);
void sun4i_backend_disable_layer_frontend(struct sun4i_backend *backend,
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 224bb1811e0a..eb93df445a10 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -101,7 +101,7 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
sun4i_frontend_update_buffer(frontend, plane);
sun4i_frontend_update_formats(frontend, plane,
DRM_FORMAT_ARGB8888);
- sun4i_backend_update_layer_frontend(backend, layer->id,
+ sun4i_backend_update_layer_frontend(backend, layer->id, plane,
DRM_FORMAT_ARGB8888);
sun4i_frontend_enable(frontend);
} else {
--
2.16.2
This adds a dedicated function for disabling the layer video channel
from the frontend to the backend. It is called automatically on an
atomic update, as to disable the frontend by default (it will be enabled
later on in the atomic update if necessary).
It fixes an issue when enabling a layer that uses the frontend (e.g. for
scaling) and then reusing the same layer without the frontend. Since the
video channel to the frontend was never disabled, the backend was still
trying to get data from there.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 8 ++++++++
drivers/gpu/drm/sun4i/sun4i_backend.h | 2 ++
drivers/gpu/drm/sun4i/sun4i_layer.c | 2 ++
3 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 7e647044cbed..e07a33adc51d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -395,6 +395,14 @@ int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend, int layer,
return 0;
}
+void sun4i_backend_disable_layer_frontend(struct sun4i_backend *backend,
+ int layer)
+{
+ regmap_update_bits(backend->engine.regs,
+ SUN4I_BACKEND_ATTCTL_REG0(layer),
+ SUN4I_BACKEND_ATTCTL_REG0_LAY_VDOEN, 0);
+}
+
static bool sun4i_backend_plane_uses_scaler(struct drm_plane_state *state)
{
u16 src_h = state->src_h >> 16;
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index 316f2179e9e1..7ae0f0ffec8c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -204,5 +204,7 @@ int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend,
int layer, uint32_t in_fmt);
int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend,
int layer, struct drm_plane *plane);
+void sun4i_backend_disable_layer_frontend(struct sun4i_backend *backend,
+ int layer);
#endif /* _SUN4I_BACKEND_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 2949a3c912c1..224bb1811e0a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -93,6 +93,8 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
struct sun4i_backend *backend = layer->backend;
struct sun4i_frontend *frontend = backend->frontend;
+ sun4i_backend_disable_layer_frontend(backend, layer->id);
+
if (layer_state->uses_frontend) {
sun4i_frontend_init(frontend);
sun4i_frontend_update_coord(frontend, plane);
--
2.16.2
In order to check whether the frontend supports a specific format, an
explicit list and a related helper are introduced.
They are then used to determine whether the frontend can actually support
the requested format when it was selected to be used.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 5 ++++
drivers/gpu/drm/sun4i/sun4i_frontend.c | 44 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/sun4i/sun4i_frontend.h | 1 +
3 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 7703ba989743..1fad0714c70e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -532,6 +532,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
struct drm_format_name_buf format_name;
if (sun4i_backend_plane_uses_frontend(plane_state)) {
+ if (!sun4i_frontend_format_is_supported(fb->format->format)) {
+ DRM_DEBUG_DRIVER("Frontend plane check failed\n");
+ return -EINVAL;
+ }
+
DRM_DEBUG_DRIVER("Using the frontend for plane %d\n",
plane->index);
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
index 3ea925584891..2dc33383be22 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
@@ -128,6 +128,50 @@ static int sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
}
}
+static const uint32_t sun4i_frontend_formats[] = {
+ /* RGB */
+ DRM_FORMAT_XRGB8888,
+ DRM_FORMAT_BGRX8888,
+ /* YUV444 */
+ DRM_FORMAT_YUV444,
+ DRM_FORMAT_YVU444,
+ /* YUV422 */
+ DRM_FORMAT_YUYV,
+ DRM_FORMAT_YVYU,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_VYUY,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV61,
+ DRM_FORMAT_YUV422,
+ DRM_FORMAT_YVU422,
+ /* YUV420 */
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV21,
+ DRM_FORMAT_YUV420,
+ DRM_FORMAT_YVU420,
+ /* YUV411 */
+ DRM_FORMAT_YUV411,
+ DRM_FORMAT_YVU411,
+};
+
+bool sun4i_frontend_format_is_supported(uint32_t fmt)
+{
+ bool found = false;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(sun4i_frontend_formats); i++) {
+ if (sun4i_frontend_formats[i] == fmt) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return false;
+
+ return true;
+}
+
int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
struct drm_plane *plane, uint32_t out_fmt)
{
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.h b/drivers/gpu/drm/sun4i/sun4i_frontend.h
index 02661ce81f3e..a9cb908ced16 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.h
@@ -95,5 +95,6 @@ void sun4i_frontend_update_coord(struct sun4i_frontend *frontend,
struct drm_plane *plane);
int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
struct drm_plane *plane, uint32_t out_fmt);
+bool sun4i_frontend_format_is_supported(uint32_t fmt);
#endif /* _SUN4I_FRONTEND_H_ */
--
2.16.2
The frontend supports many YUV formats as input and also contains a
color-space converter (CSC) block that can convert YUV input into
RGB output. It also allows scaling between the input and output for
every possible combination of supported formats.
This adds support for all the (untiled) YUV video formats supported by
the frontend, with associated changes in the backend and layer
management.
A specific dumb GEM create function translates a hardware constraint,
that the stride must be an even number, when allocating dumb (linear)
buffers.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 10 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 11 +-
drivers/gpu/drm/sun4i/sun4i_drv.h | 4 +
drivers/gpu/drm/sun4i/sun4i_frontend.c | 234 ++++++++++++++++++++++++++++-----
drivers/gpu/drm/sun4i/sun4i_frontend.h | 48 ++++++-
drivers/gpu/drm/sun4i/sun4i_layer.c | 34 +++--
6 files changed, 293 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index e8af9f3cf20b..3de7f3a427c3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -500,6 +500,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
layer_state->uses_frontend = true;
num_frontend_planes++;
} else {
+ if (sun4i_format_is_yuv(fb->format->format)) {
+ DRM_DEBUG_DRIVER("Plane FB format is YUV\n");
+ num_yuv_planes++;
+ }
+
layer_state->uses_frontend = false;
}
@@ -510,11 +515,6 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
if (fb->format->has_alpha)
num_alpha_planes++;
- if (sun4i_format_is_yuv(fb->format->format)) {
- DRM_DEBUG_DRIVER("Plane FB format is YUV\n");
- num_yuv_planes++;
- }
-
DRM_DEBUG_DRIVER("Plane zpos is %d\n",
plane_state->normalized_zpos);
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 3957c2ff6870..d374bb61c565 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -42,7 +42,7 @@ static struct drm_driver sun4i_drv_driver = {
.minor = 0,
/* GEM Operations */
- .dumb_create = drm_gem_cma_dumb_create,
+ .dumb_create = drm_sun4i_gem_dumb_create,
.gem_free_object_unlocked = drm_gem_cma_free_object,
.gem_vm_ops = &drm_gem_cma_vm_ops,
@@ -60,6 +60,15 @@ static struct drm_driver sun4i_drv_driver = {
/* Frame Buffer Operations */
};
+int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
+ struct drm_device *drm,
+ struct drm_mode_create_dumb *args)
+{
+ args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp, 8), 2);
+
+ return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
+}
+
static void sun4i_remove_framebuffers(void)
{
struct apertures_struct *ap;
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
index 5750b8ce8b31..47969711a889 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.h
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
@@ -23,4 +23,8 @@ struct sun4i_drv {
struct list_head tcon_list;
};
+int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
+ struct drm_device *drm,
+ struct drm_mode_create_dumb *args);
+
#endif /* _SUN4I_DRV_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
index 2dc33383be22..d9e58e96119c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
@@ -16,6 +16,7 @@
#include <linux/reset.h>
#include "sun4i_drv.h"
+#include "sun4i_format.h"
#include "sun4i_frontend.h"
static const u32 sun4i_frontend_vert_coef[32] = {
@@ -89,26 +90,135 @@ void sun4i_frontend_update_buffer(struct sun4i_frontend *frontend,
{
struct drm_plane_state *state = plane->state;
struct drm_framebuffer *fb = state->fb;
+ uint32_t format = fb->format->format;
+ uint32_t width, height;
+ uint32_t stride, offset;
+ bool swap;
dma_addr_t paddr;
- /* Set the line width */
- DRM_DEBUG_DRIVER("Frontend stride: %d bytes\n", fb->pitches[0]);
+ width = state->src_w >> 16;
+ height = state->src_h >> 16;
+
regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD0_REG,
fb->pitches[0]);
+ if (drm_format_num_planes(format) > 1)
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD1_REG,
+ fb->pitches[1]);
+
+ if (drm_format_num_planes(format) > 2)
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD2_REG,
+ fb->pitches[2]);
+
/* Set the physical address of the buffer in memory */
paddr = drm_fb_cma_get_gem_addr(fb, state, 0);
paddr -= PHYS_OFFSET;
- DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
+ DRM_DEBUG_DRIVER("Setting buffer #0 address to %pad\n", &paddr);
regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR0_REG, paddr);
+
+ /* Some planar formats require chroma channel swapping by hand. */
+ swap = sun4i_frontend_format_chroma_requires_swap(format);
+
+ if (drm_format_num_planes(format) > 1) {
+ paddr = drm_fb_cma_get_gem_addr(fb, state, swap ? 2 : 1);
+ paddr -= PHYS_OFFSET;
+ DRM_DEBUG_DRIVER("Setting buffer #1 address to %pad\n", &paddr);
+ regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR1_REG,
+ paddr);
+ }
+
+ if (drm_format_num_planes(format) > 2) {
+ paddr = drm_fb_cma_get_gem_addr(fb, state, swap ? 1 : 2);
+ paddr -= PHYS_OFFSET;
+ DRM_DEBUG_DRIVER("Setting buffer #2 address to %pad\n", &paddr);
+ regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR2_REG,
+ paddr);
+ }
}
EXPORT_SYMBOL(sun4i_frontend_update_buffer);
static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32 *val)
{
+ if (sun4i_format_is_rgb(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_RGB;
+ else if (sun4i_format_is_yuv411(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV411;
+ else if (sun4i_format_is_yuv420(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV420;
+ else if (sun4i_format_is_yuv422(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV422;
+ else if (sun4i_format_is_yuv444(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV444;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+
+static int sun4i_frontend_drm_format_to_input_mode(uint32_t fmt, u32 *val)
+{
+ if (sun4i_format_is_packed(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED;
+ else if (sun4i_format_is_semiplanar(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR;
+ else if (sun4i_format_is_planar(fmt))
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+
+static int sun4i_frontend_drm_format_to_input_sequence(uint32_t fmt, u32 *val)
+{
+ /* Planar formats have an explicit input sequence. */
+ if (sun4i_format_is_planar(fmt)) {
+ *val = 0;
+ return 0;
+ }
+
switch (fmt) {
+ /* RGB */
case DRM_FORMAT_XRGB8888:
- *val = 5;
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_XRGB;
+ return 0;
+
+ case DRM_FORMAT_BGRX8888:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_BGRX;
+ return 0;
+
+ /* YUV420 */
+ case DRM_FORMAT_NV12:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV;
+ return 0;
+
+ case DRM_FORMAT_NV21:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU;
+ return 0;
+
+ /* YUV422 */
+ case DRM_FORMAT_YUYV:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YUYV;
+ return 0;
+
+ case DRM_FORMAT_VYUY:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VYUY;
+ return 0;
+
+ case DRM_FORMAT_YVYU:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YVYU;
+ return 0;
+
+ case DRM_FORMAT_UYVY:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UYVY;
+ return 0;
+
+ case DRM_FORMAT_NV16:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV;
+ return 0;
+
+ case DRM_FORMAT_NV61:
+ *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU;
return 0;
default:
@@ -120,7 +230,11 @@ static int sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
{
switch (fmt) {
case DRM_FORMAT_XRGB8888:
- *val = 2;
+ *val = SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_XRGB8888;
+ return 0;
+
+ case DRM_FORMAT_BGRX8888:
+ *val = SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_BGRX8888;
return 0;
default:
@@ -172,22 +286,52 @@ bool sun4i_frontend_format_is_supported(uint32_t fmt)
return true;
}
+bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt)
+{
+ switch (fmt) {
+ case DRM_FORMAT_YVU444:
+ case DRM_FORMAT_YVU422:
+ case DRM_FORMAT_YVU420:
+ case DRM_FORMAT_YVU411:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
struct drm_plane *plane, uint32_t out_fmt)
{
struct drm_plane_state *state = plane->state;
struct drm_framebuffer *fb = state->fb;
+ uint32_t format = fb->format->format;
u32 out_fmt_val;
u32 in_fmt_val;
+ u32 in_mod_val;
+ u32 in_ps_val;
+ u32 bypass;
+ unsigned int i;
int ret;
- ret = sun4i_frontend_drm_format_to_input_fmt(fb->format->format,
- &in_fmt_val);
+ ret = sun4i_frontend_drm_format_to_input_fmt(format, &in_fmt_val);
if (ret) {
DRM_DEBUG_DRIVER("Invalid input format\n");
return ret;
}
+ ret = sun4i_frontend_drm_format_to_input_mode(format, &in_mod_val);
+ if (ret) {
+ DRM_DEBUG_DRIVER("Invalid input mode\n");
+ return ret;
+ }
+
+ ret = sun4i_frontend_drm_format_to_input_sequence(format, &in_ps_val);
+ if (ret) {
+ DRM_DEBUG_DRIVER("Invalid pixel sequence\n");
+ return ret;
+ }
+
ret = sun4i_frontend_drm_format_to_output_fmt(out_fmt, &out_fmt_val);
if (ret) {
DRM_DEBUG_DRIVER("Invalid output format\n");
@@ -205,10 +349,30 @@ int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_VERTPHASE1_REG, 0x400);
regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_VERTPHASE1_REG, 0x400);
+ if (sun4i_format_is_yuv(format) &&
+ !sun4i_format_is_yuv(out_fmt)) {
+ /* Setup the CSC engine for YUV to RGB conversion. */
+
+ for (i = 0; i < ARRAY_SIZE(sunxi_bt601_yuv2rgb_coef); i++)
+ regmap_write(frontend->regs,
+ SUN4I_FRONTEND_CSC_COEF_REG(i),
+ sunxi_bt601_yuv2rgb_coef[i]);
+
+ regmap_update_bits(frontend->regs,
+ SUN4I_FRONTEND_FRM_CTRL_REG,
+ SUN4I_FRONTEND_FRM_CTRL_COEF_RDY,
+ SUN4I_FRONTEND_FRM_CTRL_COEF_RDY);
+
+ bypass = 0;
+ } else {
+ bypass = SUN4I_FRONTEND_BYPASS_CSC_EN;
+ }
+
+ regmap_update_bits(frontend->regs, SUN4I_FRONTEND_BYPASS_REG,
+ SUN4I_FRONTEND_BYPASS_CSC_EN, bypass);
+
regmap_write(frontend->regs, SUN4I_FRONTEND_INPUT_FMT_REG,
- SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(1) |
- SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(in_fmt_val) |
- SUN4I_FRONTEND_INPUT_FMT_PS(1));
+ in_mod_val | in_fmt_val | in_ps_val);
/*
* TODO: It look like the A31 and A80 at least will need the
@@ -216,7 +380,7 @@ int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
* ARGB8888).
*/
regmap_write(frontend->regs, SUN4I_FRONTEND_OUTPUT_FMT_REG,
- SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(out_fmt_val));
+ out_fmt_val);
return 0;
}
@@ -226,31 +390,45 @@ void sun4i_frontend_update_coord(struct sun4i_frontend *frontend,
struct drm_plane *plane)
{
struct drm_plane_state *state = plane->state;
+ struct drm_framebuffer *fb = state->fb;
+ uint32_t format = fb->format->format;
+ uint32_t luma_width, luma_height;
+ uint32_t chroma_width, chroma_height;
/* Set height and width */
- DRM_DEBUG_DRIVER("Frontend size W: %u H: %u\n",
+ DRM_DEBUG_DRIVER("Frontend crtc size W: %u H: %u\n",
state->crtc_w, state->crtc_h);
- regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_INSIZE_REG,
- SUN4I_FRONTEND_INSIZE(state->src_h >> 16,
- state->src_w >> 16));
- regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_INSIZE_REG,
- SUN4I_FRONTEND_INSIZE(state->src_h >> 16,
- state->src_w >> 16));
+ luma_width = chroma_width = state->src_w >> 16;
+ luma_height = chroma_height = state->src_h >> 16;
+
+ if (sun4i_format_is_yuv411(format)) {
+ chroma_width = DIV_ROUND_UP(luma_width, 4);
+ } else if (sun4i_format_is_yuv420(format)) {
+ chroma_width = DIV_ROUND_UP(luma_width, 2);
+ chroma_height = DIV_ROUND_UP(luma_height, 2);
+ } else if (sun4i_format_is_yuv422(format)) {
+ chroma_width = DIV_ROUND_UP(luma_width, 2);
+ }
+
+ regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_INSIZE_REG,
+ SUN4I_FRONTEND_INSIZE(luma_height, luma_width));
regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_OUTSIZE_REG,
SUN4I_FRONTEND_OUTSIZE(state->crtc_h, state->crtc_w));
+ regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_HORZFACT_REG,
+ (luma_width << 16) / state->crtc_w);
+ regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_VERTFACT_REG,
+ (luma_height << 16) / state->crtc_h);
+
+ /* These also have to be specified, even for interleaved formats. */
+ regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_INSIZE_REG,
+ SUN4I_FRONTEND_INSIZE(chroma_height, chroma_width));
regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_OUTSIZE_REG,
SUN4I_FRONTEND_OUTSIZE(state->crtc_h, state->crtc_w));
-
- regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_HORZFACT_REG,
- state->src_w / state->crtc_w);
regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_HORZFACT_REG,
- state->src_w / state->crtc_w);
-
- regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_VERTFACT_REG,
- state->src_h / state->crtc_h);
+ (chroma_width << 16) / state->crtc_w);
regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_VERTFACT_REG,
- state->src_h / state->crtc_h);
+ (chroma_height << 16) / state->crtc_h);
regmap_write_bits(frontend->regs, SUN4I_FRONTEND_FRM_CTRL_REG,
SUN4I_FRONTEND_FRM_CTRL_REG_RDY,
@@ -382,10 +560,6 @@ static int sun4i_frontend_runtime_resume(struct device *dev)
SUN4I_FRONTEND_EN_EN,
SUN4I_FRONTEND_EN_EN);
- regmap_update_bits(frontend->regs, SUN4I_FRONTEND_BYPASS_REG,
- SUN4I_FRONTEND_BYPASS_CSC_EN,
- SUN4I_FRONTEND_BYPASS_CSC_EN);
-
sun4i_frontend_scaler_init(frontend);
return 0;
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.h b/drivers/gpu/drm/sun4i/sun4i_frontend.h
index a9cb908ced16..6dd1d18752f4 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.h
@@ -21,17 +21,56 @@
#define SUN4I_FRONTEND_BYPASS_REG 0x008
#define SUN4I_FRONTEND_BYPASS_CSC_EN BIT(1)
+#define SUN4I_FRONTEND_AGTH_SEL_REG 0x00C
+#define SUN4I_FRONTEND_AGTH_SEL_ORIGINAL BIT(8)
+
#define SUN4I_FRONTEND_BUF_ADDR0_REG 0x020
+#define SUN4I_FRONTEND_BUF_ADDR1_REG 0x024
+#define SUN4I_FRONTEND_BUF_ADDR2_REG 0x028
+
+#define SUN4I_FRONTEND_TB_OFF0_REG 0x030
+#define SUN4I_FRONTEND_TB_OFF1_REG 0x034
+#define SUN4I_FRONTEND_TB_OFF2_REG 0x038
+
+#define SUN4I_FRONTEND_TB_OFF_X1(x1) ((x1) << 16)
+#define SUN4I_FRONTEND_TB_OFF_Y0(y0) ((y0) << 8)
+#define SUN4I_FRONTEND_TB_OFF_X0(x0) (x0)
#define SUN4I_FRONTEND_LINESTRD0_REG 0x040
+#define SUN4I_FRONTEND_LINESTRD1_REG 0x044
+#define SUN4I_FRONTEND_LINESTRD2_REG 0x048
#define SUN4I_FRONTEND_INPUT_FMT_REG 0x04c
-#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(mod) ((mod) << 8)
-#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(fmt) ((fmt) << 4)
-#define SUN4I_FRONTEND_INPUT_FMT_PS(ps) (ps)
+
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MASK GENMASK(10, 8)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR (0 << 8)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED (1 << 8)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR (2 << 8)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_PLANAR (4 << 8)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_SEMIPLANAR (6 << 8)
+
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_MASK GENMASK(6, 4)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV444 (0 << 4)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV422 (1 << 4)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV420 (2 << 4)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV411 (3 << 4)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_RGB (5 << 4)
+
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_MASK GENMASK(1, 0)
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UYVY 0
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YUYV 1
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VYUY 2
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YVYU 3
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV 0
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU 1
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_XRGB 0
+#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_BGRX 1
#define SUN4I_FRONTEND_OUTPUT_FMT_REG 0x05c
-#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(fmt) (fmt)
+#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_BGRX8888 1
+#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_XRGB8888 2
+
+#define SUN4I_FRONTEND_CSC_COEF_REG(c) (0x070 + (0x4 * (c)))
#define SUN4I_FRONTEND_CH0_INSIZE_REG 0x100
#define SUN4I_FRONTEND_INSIZE(h, w) ((((h) - 1) << 16) | (((w) - 1)))
@@ -96,5 +135,6 @@ void sun4i_frontend_update_coord(struct sun4i_frontend *frontend,
int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
struct drm_plane *plane, uint32_t out_fmt);
bool sun4i_frontend_format_is_supported(uint32_t fmt);
+bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt);
#endif /* _SUN4I_FRONTEND_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 15238211a61a..a39eed6a0e75 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -128,19 +128,37 @@ static const struct drm_plane_funcs sun4i_backend_layer_funcs = {
.update_plane = drm_atomic_helper_update_plane,
};
-static const uint32_t sun4i_backend_layer_formats[] = {
- DRM_FORMAT_ARGB8888,
+static const uint32_t sun4i_layer_formats[] = {
+ /* RGB */
DRM_FORMAT_ARGB4444,
+ DRM_FORMAT_RGBA4444,
DRM_FORMAT_ARGB1555,
DRM_FORMAT_RGBA5551,
- DRM_FORMAT_RGBA4444,
- DRM_FORMAT_RGB888,
DRM_FORMAT_RGB565,
- DRM_FORMAT_UYVY,
- DRM_FORMAT_VYUY,
+ DRM_FORMAT_RGB888,
DRM_FORMAT_XRGB8888,
+ DRM_FORMAT_BGRX8888,
+ DRM_FORMAT_ARGB8888,
+ /* YUV444 */
+ DRM_FORMAT_YUV444,
+ DRM_FORMAT_YVU444,
+ /* YUV422 */
DRM_FORMAT_YUYV,
DRM_FORMAT_YVYU,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_VYUY,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV61,
+ DRM_FORMAT_YUV422,
+ DRM_FORMAT_YVU422,
+ /* YUV420 */
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV21,
+ DRM_FORMAT_YUV420,
+ DRM_FORMAT_YVU420,
+ /* YUV411 */
+ DRM_FORMAT_YUV411,
+ DRM_FORMAT_YVU411,
};
static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
@@ -157,8 +175,8 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
/* possible crtcs are set later */
ret = drm_universal_plane_init(drm, &layer->plane, 0,
&sun4i_backend_layer_funcs,
- sun4i_backend_layer_formats,
- ARRAY_SIZE(sun4i_backend_layer_formats),
+ sun4i_layer_formats,
+ ARRAY_SIZE(sun4i_layer_formats),
NULL, type, NULL);
if (ret) {
dev_err(drm->dev, "Couldn't initialize layer\n");
--
2.16.2
This adds all the required configuration and support for handling YUV
formats tiled with the Allwinner MB32 modifier. This newly-introduced
YUV support should be in as good a shape as RGB support.
While this implementation covers most MB32-capable formats supported
by the frontend, only NV12-based formats were actually tested.
Since most of the logic is common, it is likely that other formats will
work just as well.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 10 +++-
drivers/gpu/drm/sun4i/sun4i_backend.h | 2 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 1 +
drivers/gpu/drm/sun4i/sun4i_frontend.c | 100 ++++++++++++++++++++++++++++-----
drivers/gpu/drm/sun4i/sun4i_frontend.h | 3 +-
drivers/gpu/drm/sun4i/sun4i_layer.c | 16 +++++-
6 files changed, 113 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 3de7f3a427c3..335c444b1547 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -147,11 +147,14 @@ static const uint32_t sun4i_backend_formats[] = {
DRM_FORMAT_VYUY,
};
-bool sun4i_backend_format_is_supported(uint32_t fmt)
+bool sun4i_backend_format_is_supported(uint32_t fmt, uint64_t modifier)
{
bool found = false;
unsigned int i;
+ if (modifier == DRM_FORMAT_MOD_ALLWINNER_MB32_TILED)
+ return false;
+
for (i = 0; i < ARRAY_SIZE(sun4i_backend_formats); i++) {
if (sun4i_backend_formats[i] == fmt) {
found = true;
@@ -437,7 +440,8 @@ static bool sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
* not supported by either. There is still room to check this later in
* the atomic check process.
*/
- if (!sun4i_backend_format_is_supported(fb->format->format))
+ if (!sun4i_backend_format_is_supported(fb->format->format,
+ fb->modifier))
return true;
/*
@@ -489,7 +493,7 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
struct drm_format_name_buf format_name;
if (sun4i_backend_plane_uses_frontend(plane_state)) {
- if (!sun4i_frontend_format_is_supported(fb->format->format)) {
+ if (!sun4i_frontend_plane_check(plane_state)) {
DRM_DEBUG_DRIVER("Frontend plane check failed\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index a7bfc38f12bd..bd93808c3ee7 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -207,6 +207,6 @@ int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend,
int layer, struct drm_plane *plane);
void sun4i_backend_disable_layer_frontend(struct sun4i_backend *backend,
int layer);
-bool sun4i_backend_format_is_supported(uint32_t fmt);
+bool sun4i_backend_format_is_supported(uint32_t fmt, uint64_t modifier);
#endif /* _SUN4I_BACKEND_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index e9cb03d34b44..41888743722a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -208,6 +208,7 @@ static int sun4i_drv_bind(struct device *dev)
}
drm_mode_config_init(drm);
+ drm->mode_config.allow_fb_modifiers = true;
ret = component_bind_all(drm->dev, drm);
if (ret) {
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
index d9e58e96119c..85f75046712c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
@@ -99,16 +99,56 @@ void sun4i_frontend_update_buffer(struct sun4i_frontend *frontend,
width = state->src_w >> 16;
height = state->src_h >> 16;
- regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD0_REG,
- fb->pitches[0]);
+ if (fb->modifier == DRM_FORMAT_MOD_ALLWINNER_MB32_TILED) {
+ /*
+ * In MB32 tiled mode, the stride is defined as the distance
+ * between the start of the end line of the current tile and
+ * the start of the first line in the next vertical tile.
+ *
+ * Tiles are represented linearly in memory, thus the end line
+ * of current tile starts at: 31 * 32 (31 lines of 32 cols),
+ * the next vertical tile starts at: 32-bit-aligned-width * 32
+ * and the distance is: 32 * (32-bit-aligned-width - 31).
+ */
+
+ stride = (fb->pitches[0] - 31) * 32;
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD0_REG,
+ stride);
+
+ /* Offset of the bottom-right point in the end tile. */
+ offset = (width + (32 - 1)) & (32 - 1);
+ regmap_write(frontend->regs, SUN4I_FRONTEND_TB_OFF0_REG,
+ SUN4I_FRONTEND_TB_OFF_X1(offset));
+
+ if (drm_format_num_planes(format) > 1) {
+ stride = (fb->pitches[1] - 31) * 32;
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD1_REG,
+ stride);
+
+ regmap_write(frontend->regs, SUN4I_FRONTEND_TB_OFF1_REG,
+ SUN4I_FRONTEND_TB_OFF_X1(offset));
+ }
+
+ if (drm_format_num_planes(format) > 2) {
+ stride = (fb->pitches[2] - 31) * 32;
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD2_REG,
+ stride);
+
+ regmap_write(frontend->regs, SUN4I_FRONTEND_TB_OFF2_REG,
+ SUN4I_FRONTEND_TB_OFF_X1(offset));
+ }
+ } else {
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD0_REG,
+ fb->pitches[0]);
- if (drm_format_num_planes(format) > 1)
- regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD1_REG,
- fb->pitches[1]);
+ if (drm_format_num_planes(format) > 1)
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD1_REG,
+ fb->pitches[1]);
- if (drm_format_num_planes(format) > 2)
- regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD2_REG,
- fb->pitches[2]);
+ if (drm_format_num_planes(format) > 2)
+ regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD2_REG,
+ fb->pitches[2]);
+ }
/* Set the physical address of the buffer in memory */
paddr = drm_fb_cma_get_gem_addr(fb, state, 0);
@@ -155,14 +195,22 @@ static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32 *val)
return 0;
}
-static int sun4i_frontend_drm_format_to_input_mode(uint32_t fmt, u32 *val)
+static int sun4i_frontend_drm_format_to_input_mode(uint32_t fmt, u32 *val,
+ uint64_t modifier)
{
+ bool tiled = modifier == DRM_FORMAT_MOD_ALLWINNER_MB32_TILED;
+
+ if (tiled && !sun4i_format_supports_tiling(fmt))
+ return -EINVAL;
+
if (sun4i_format_is_packed(fmt))
*val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED;
else if (sun4i_format_is_semiplanar(fmt))
- *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR;
+ *val = tiled ? SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_SEMIPLANAR
+ : SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR;
else if (sun4i_format_is_planar(fmt))
- *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR;
+ *val = tiled ? SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_PLANAR
+ : SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR;
else
return -EINVAL;
@@ -268,7 +316,7 @@ static const uint32_t sun4i_frontend_formats[] = {
DRM_FORMAT_YVU411,
};
-bool sun4i_frontend_format_is_supported(uint32_t fmt)
+bool sun4i_frontend_format_is_supported(uint32_t fmt, uint64_t modifier)
{
bool found = false;
unsigned int i;
@@ -283,6 +331,31 @@ bool sun4i_frontend_format_is_supported(uint32_t fmt)
if (!found)
return false;
+ if (modifier == DRM_FORMAT_MOD_ALLWINNER_MB32_TILED)
+ return sun4i_format_supports_tiling(fmt);
+
+ return true;
+}
+
+bool sun4i_frontend_plane_check(struct drm_plane_state *state)
+{
+ struct drm_framebuffer *fb = state->fb;
+ uint32_t format = fb->format->format;
+ uint32_t width = state->src_w >> 16;
+ uint64_t modifier = fb->modifier;
+ bool supported;
+
+ supported = sun4i_frontend_format_is_supported(format, modifier);
+ if (!supported)
+ return false;
+
+ /* Width is required to be even for MB32 tiled format. */
+ if (modifier == DRM_FORMAT_MOD_ALLWINNER_MB32_TILED &&
+ (width % 2) != 0) {
+ DRM_DEBUG_DRIVER("MB32 tiled format requires an even width\n");
+ return false;
+ }
+
return true;
}
@@ -320,7 +393,8 @@ int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
return ret;
}
- ret = sun4i_frontend_drm_format_to_input_mode(format, &in_mod_val);
+ ret = sun4i_frontend_drm_format_to_input_mode(format, &in_mod_val,
+ fb->modifier);
if (ret) {
DRM_DEBUG_DRIVER("Invalid input mode\n");
return ret;
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.h b/drivers/gpu/drm/sun4i/sun4i_frontend.h
index 6dd1d18752f4..ab6ab4f16f74 100644
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.h
@@ -134,7 +134,8 @@ void sun4i_frontend_update_coord(struct sun4i_frontend *frontend,
struct drm_plane *plane);
int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
struct drm_plane *plane, uint32_t out_fmt);
-bool sun4i_frontend_format_is_supported(uint32_t fmt);
+bool sun4i_frontend_format_is_supported(uint32_t fmt, uint64_t modifier);
+bool sun4i_frontend_plane_check(struct drm_plane_state *plane_state);
bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt);
#endif /* _SUN4I_FRONTEND_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index a39eed6a0e75..d456148ed4b3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -114,6 +114,13 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
sun4i_backend_layer_enable(backend, layer->id, true);
}
+static bool sun4i_layer_format_mod_supported(struct drm_plane *plane,
+ uint32_t format, uint64_t modifier)
+{
+ return sun4i_backend_format_is_supported(format, modifier) ||
+ sun4i_frontend_format_is_supported(format, modifier);
+}
+
static const struct drm_plane_helper_funcs sun4i_backend_layer_helper_funcs = {
.atomic_disable = sun4i_backend_layer_atomic_disable,
.atomic_update = sun4i_backend_layer_atomic_update,
@@ -126,6 +133,7 @@ static const struct drm_plane_funcs sun4i_backend_layer_funcs = {
.disable_plane = drm_atomic_helper_disable_plane,
.reset = sun4i_backend_layer_reset,
.update_plane = drm_atomic_helper_update_plane,
+ .format_mod_supported = sun4i_layer_format_mod_supported,
};
static const uint32_t sun4i_layer_formats[] = {
@@ -161,6 +169,12 @@ static const uint32_t sun4i_layer_formats[] = {
DRM_FORMAT_YVU411,
};
+static const uint64_t sun4i_layer_modifiers[] = {
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_ALLWINNER_MB32_TILED,
+ DRM_FORMAT_MOD_INVALID
+};
+
static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
struct sun4i_backend *backend,
enum drm_plane_type type)
@@ -177,7 +191,7 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
&sun4i_backend_layer_funcs,
sun4i_layer_formats,
ARRAY_SIZE(sun4i_layer_formats),
- NULL, type, NULL);
+ sun4i_layer_modifiers, type, NULL);
if (ret) {
dev_err(drm->dev, "Couldn't initialize layer\n");
return ERR_PTR(ret);
--
2.16.2
This introduces specific definitions for vendor Allwinner and its
specific MB32 tiled format, an NV12-based format with 32x32 tiles.
This format is the default output format used by the VPU on most
Allwinner platforms.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
include/uapi/drm/drm_fourcc.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index e04613d30a13..1b7ef9102582 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -183,6 +183,7 @@ extern "C" {
#define DRM_FORMAT_MOD_VENDOR_QCOM 0x05
#define DRM_FORMAT_MOD_VENDOR_VIVANTE 0x06
#define DRM_FORMAT_MOD_VENDOR_BROADCOM 0x07
+#define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x08
/* add more to the end as needed */
#define DRM_FORMAT_RESERVED ((1ULL << 56) - 1)
@@ -405,6 +406,15 @@ extern "C" {
*/
#define DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED fourcc_mod_code(BROADCOM, 1)
+/*
+ * Allwinner "MB32" tiled format
+ *
+ * This is the primary layout coming out of the VPU, where pixels are tiled
+ * 32x32.
+ */
+#define DRM_FORMAT_MOD_ALLWINNER_MB32_TILED fourcc_mod_code(ALLWINNER, 1)
+
+
#if defined(__cplusplus)
}
#endif
--
2.16.2
In order to check whether the backend supports a specific format, an
explicit list and a related helper are introduced.
They are then used to determine whether the frontend should be used for
a layer, when the format is not supported by the backend.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 48 ++++++++++++++++++++++++++++++++++-
drivers/gpu/drm/sun4i/sun4i_backend.h | 1 +
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 274a1db6fa8e..7703ba989743 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -172,6 +172,39 @@ static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
return 0;
}
+static const uint32_t sun4i_backend_formats[] = {
+ /* RGB */
+ DRM_FORMAT_ARGB4444,
+ DRM_FORMAT_RGBA4444,
+ DRM_FORMAT_ARGB1555,
+ DRM_FORMAT_RGBA5551,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_XRGB8888,
+ DRM_FORMAT_BGRX8888,
+ DRM_FORMAT_ARGB8888,
+ /* YUV422 */
+ DRM_FORMAT_YUYV,
+ DRM_FORMAT_YVYU,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_VYUY,
+};
+
+bool sun4i_backend_format_is_supported(uint32_t fmt)
+{
+ bool found = false;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(sun4i_backend_formats); i++) {
+ if (sun4i_backend_formats[i] == fmt) {
+ found = true;
+ break;
+ }
+ }
+
+ return found;
+}
+
int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
int layer, struct drm_plane *plane)
{
@@ -436,15 +469,28 @@ static bool sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
{
struct sun4i_layer *layer = plane_to_sun4i_layer(state->plane);
struct sun4i_backend *backend = layer->backend;
+ struct drm_framebuffer *fb = state->fb;
if (IS_ERR(backend->frontend))
return false;
+ /*
+ * Let's pretend that every format is either supported by the backend or
+ * the frontend. This is not true in practice, as some tiling modes are
+ * not supported by either. There is still room to check this later in
+ * the atomic check process.
+ */
+ if (!sun4i_backend_format_is_supported(fb->format->format))
+ return true;
+
/*
* TODO: Don't use the frontend for x2/x4 scaling and allow RGB formats
* with an alpha component then.
*/
- return sun4i_backend_plane_uses_scaler(state);
+ if (sun4i_backend_plane_uses_scaler(state))
+ return true;
+
+ return false;
}
static void sun4i_backend_atomic_begin(struct sunxi_engine *engine,
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index cb6df2b690c0..a7bfc38f12bd 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -207,5 +207,6 @@ int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend,
int layer, struct drm_plane *plane);
void sun4i_backend_disable_layer_frontend(struct sun4i_backend *backend,
int layer);
+bool sun4i_backend_format_is_supported(uint32_t fmt);
#endif /* _SUN4I_BACKEND_H_ */
--
2.16.2
This introduces a dedicated ioctl for allocating tiled buffers in the
Allwinner MB32 format, that comes with a handful of specific
constraints. In particular, the stride of the buffers is expected to be
aligned to 32 bytes.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_drv.c | 96 +++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/sun4i/sun4i_drv.h | 2 +
include/uapi/drm/sun4i_drm.h | 42 +++++++++++++++++
3 files changed, 140 insertions(+)
create mode 100644 include/uapi/drm/sun4i_drm.h
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index d374bb61c565..e9cb03d34b44 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -21,11 +21,18 @@
#include <drm/drm_gem_cma_helper.h>
#include <drm/drm_fb_helper.h>
#include <drm/drm_of.h>
+#include <drm/sun4i_drm.h>
#include "sun4i_drv.h"
#include "sun4i_frontend.h"
#include "sun4i_framebuffer.h"
#include "sun4i_tcon.h"
+#include "sun4i_format.h"
+
+static const struct drm_ioctl_desc sun4i_drv_ioctls[] = {
+ DRM_IOCTL_DEF_DRV(SUN4I_GEM_CREATE_TILED, drm_sun4i_gem_create_tiled,
+ DRM_AUTH | DRM_RENDER_ALLOW),
+};
DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
@@ -34,6 +41,8 @@ static struct drm_driver sun4i_drv_driver = {
/* Generic Operations */
.lastclose = drm_fb_helper_lastclose,
+ .ioctls = sun4i_drv_ioctls,
+ .num_ioctls = ARRAY_SIZE(sun4i_drv_ioctls),
.fops = &sun4i_drv_fops,
.name = "sun4i-drm",
.desc = "Allwinner sun4i Display Engine",
@@ -69,6 +78,93 @@ int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
}
+int drm_sun4i_gem_create_tiled(struct drm_device *drm, void *data,
+ struct drm_file *file_priv)
+{
+ struct drm_sun4i_gem_create_tiled *args = data;
+ struct drm_gem_cma_object *cma_obj;
+ struct drm_gem_object *gem_obj;
+ uint32_t luma_stride, chroma_stride;
+ uint32_t luma_height, chroma_height;
+ int ret;
+
+ if (!sun4i_format_supports_tiling(args->format))
+ return -EINVAL;
+
+ memset(args->pitches, 0, sizeof(args->pitches));
+ memset(args->offsets, 0, sizeof(args->offsets));
+
+ /* Stride and height are aligned to 32 bytes for MB32 tiled format. */
+ luma_stride = ALIGN(args->width, 32);
+ luma_height = ALIGN(args->height, 32);
+
+ if (sun4i_format_is_semiplanar(args->format)) {
+ chroma_stride = luma_stride;
+
+ if (sun4i_format_is_yuv420(args->format))
+ chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 32);
+ else if (sun4i_format_is_yuv422(args->format))
+ chroma_height = luma_height;
+ else
+ return -EINVAL;
+
+ args->pitches[0] = luma_stride;
+ args->pitches[1] = chroma_stride;
+
+ args->offsets[0] = 0;
+ args->offsets[1] = luma_stride * luma_height;
+
+ args->size = luma_stride * luma_height +
+ chroma_stride * chroma_height;
+ } else if (sun4i_format_is_planar(args->format)) {
+ if (sun4i_format_is_yuv411(args->format)) {
+ chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 4), 32);
+ chroma_height = luma_height;
+ } if (sun4i_format_is_yuv420(args->format)) {
+ chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
+ chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 32);
+ } else if (sun4i_format_is_yuv422(args->format)) {
+ chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
+ chroma_height = luma_height;
+ } else {
+ return -EINVAL;
+ }
+
+ args->pitches[0] = luma_stride;
+ args->pitches[1] = chroma_stride;
+ args->pitches[2] = chroma_stride;
+
+ args->offsets[0] = 0;
+ args->offsets[1] = luma_stride * luma_height;
+ args->offsets[2] = luma_stride * luma_height +
+ chroma_stride * chroma_height;
+
+ args->size = luma_stride * luma_height +
+ chroma_stride * chroma_height * 2;
+ } else {
+ /* No support for packed formats in tiled mode. */
+ return -EINVAL;
+ }
+
+ cma_obj = drm_gem_cma_create(drm, args->size);
+ if (IS_ERR(cma_obj))
+ return PTR_ERR(cma_obj);
+
+ gem_obj = &cma_obj->base;
+
+ /*
+ * allocate a id of idr table where the obj is registered
+ * and handle has the id what user can see.
+ */
+ ret = drm_gem_handle_create(file_priv, gem_obj, &args->handle);
+ /* drop reference from allocate - handle holds it now. */
+ drm_gem_object_put_unlocked(gem_obj);
+ if (ret)
+ return ret;
+
+ return PTR_ERR_OR_ZERO(cma_obj);
+}
+
static void sun4i_remove_framebuffers(void)
{
struct apertures_struct *ap;
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
index 47969711a889..308ff4bfcdd5 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.h
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
@@ -26,5 +26,7 @@ struct sun4i_drv {
int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
struct drm_device *drm,
struct drm_mode_create_dumb *args);
+int drm_sun4i_gem_create_tiled(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
#endif /* _SUN4I_DRV_H_ */
diff --git a/include/uapi/drm/sun4i_drm.h b/include/uapi/drm/sun4i_drm.h
new file mode 100644
index 000000000000..2c77584b057b
--- /dev/null
+++ b/include/uapi/drm/sun4i_drm.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/* sun4i_drm.h
+ *
+ * Copyright (C) 2018 Paul Kocialkowski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#ifndef _UAPI_SUN4I_DRM_H_
+#define _UAPI_SUN4I_DRM_H_
+
+#include "drm.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+struct drm_sun4i_gem_create_tiled {
+ __u32 height;
+ __u32 width;
+ __u32 format;
+ /* handle, offsets, pitches, size will be returned */
+ __u32 handle;
+ __u32 pitches[4];
+ __u32 offsets[4];
+ __u64 size;
+};
+
+#define DRM_SUN4I_GEM_CREATE_TILED 0x00
+
+#define DRM_IOCTL_SUN4I_GEM_CREATE_TILED \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_SUN4I_GEM_CREATE_TILED, \
+ struct drm_sun4i_gem_create_tiled)
+
+#if defined(__cplusplus)
+}
+#endif
+
+#endif /* _UAPI_SUN4I_DRM_H_ */
--
2.16.2
Hi Paul,
On 21 March 2018 at 15:29, Paul Kocialkowski
<[email protected]> wrote:
> +/*
> + * Allwinner "MB32" tiled format
> + *
> + * This is the primary layout coming out of the VPU, where pixels are tiled
> + * 32x32.
> + */
Can you please be a bit more specific here, following the other
examples? In particular, it should list whether the tile order is row-
or column-major.
Cheers,
Daniel
On Wed, Mar 21, 2018 at 11:28 PM, Paul Kocialkowski
<[email protected]> wrote:
> It turns out that the frontend is not capable of preserving the alpha
> component (that is always set to 0xff), so only support XRGB8888
> instead.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_backend.c | 4 ++++
> drivers/gpu/drm/sun4i/sun4i_frontend.c | 3 +--
> drivers/gpu/drm/sun4i/sun4i_layer.c | 4 ++--
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index b98dafda52f8..274a1db6fa8e 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -440,6 +440,10 @@ static bool sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
> if (IS_ERR(backend->frontend))
> return false;
>
> + /*
> + * TODO: Don't use the frontend for x2/x4 scaling and allow RGB formats
> + * with an alpha component then.
This and the commit log are kind of conflicting. Is it just the scalar
that doesn't
work with an alpha component, or the whole frontend?
Thanks
ChenYu
> + */
> return sun4i_backend_plane_uses_scaler(state);
> }
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> index ddf6cfa6dd23..3ea925584891 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> @@ -107,7 +107,7 @@ EXPORT_SYMBOL(sun4i_frontend_update_buffer);
> static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32 *val)
> {
> switch (fmt) {
> - case DRM_FORMAT_ARGB8888:
> + case DRM_FORMAT_XRGB8888:
> *val = 5;
> return 0;
>
> @@ -120,7 +120,6 @@ static int sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
> {
> switch (fmt) {
> case DRM_FORMAT_XRGB8888:
> - case DRM_FORMAT_ARGB8888:
> *val = 2;
> return 0;
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> index eb93df445a10..15238211a61a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> @@ -100,9 +100,9 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
> sun4i_frontend_update_coord(frontend, plane);
> sun4i_frontend_update_buffer(frontend, plane);
> sun4i_frontend_update_formats(frontend, plane,
> - DRM_FORMAT_ARGB8888);
> + DRM_FORMAT_XRGB8888);
> sun4i_backend_update_layer_frontend(backend, layer->id, plane,
> - DRM_FORMAT_ARGB8888);
> + DRM_FORMAT_XRGB8888);
> sun4i_frontend_enable(frontend);
> } else {
> sun4i_backend_update_layer_formats(backend, layer->id, plane);
> --
> 2.16.2
>
Hi,
On Wed, 2018-03-21 at 16:47 +0000, Daniel Stone wrote:
> Hi Paul,
>
> On 21 March 2018 at 15:29, Paul Kocialkowski
> <[email protected]> wrote:
> > +/*
> > + * Allwinner "MB32" tiled format
> > + *
> > + * This is the primary layout coming out of the VPU, where pixels
> > are tiled
> > + * 32x32.
> > + */
>
> Can you please be a bit more specific here, following the other
> examples? In particular, it should list whether the tile order is row-
> or column-major.
Yes that would definitely make sense, I'll do that in v2.
Thanks for the review!
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi Chen-Yu,
On Thu, 2018-03-22 at 14:47 +0800, Chen-Yu Tsai wrote:
> On Wed, Mar 21, 2018 at 11:28 PM, Paul Kocialkowski
> <[email protected]> wrote:
> > It turns out that the frontend is not capable of preserving the
> > alpha
> > component (that is always set to 0xff), so only support XRGB8888
> > instead.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 4 ++++
> > drivers/gpu/drm/sun4i/sun4i_frontend.c | 3 +--
> > drivers/gpu/drm/sun4i/sun4i_layer.c | 4 ++--
> > 3 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index b98dafda52f8..274a1db6fa8e 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -440,6 +440,10 @@ static bool
> > sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
> > if (IS_ERR(backend->frontend))
> > return false;
> >
> > + /*
> > + * TODO: Don't use the frontend for x2/x4 scaling and allow
> > RGB formats
> > + * with an alpha component then.
>
> This and the commit log are kind of conflicting. Is it just the scalar
> that doesn't
> work with an alpha component, or the whole frontend?
It's the whole frontend that does not support alpha in its output
formats. I'm talking about scaling here because that's still the main
reason to use the frontend. On the other hand, the backend allows
integer scaling with an alpha component, hence the comment.
Do you think I need to rework the comment/commit log?
Thanks for the review,
Paul
> > + */
> > return sun4i_backend_plane_uses_scaler(state);
> > }
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > index ddf6cfa6dd23..3ea925584891 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > @@ -107,7 +107,7 @@ EXPORT_SYMBOL(sun4i_frontend_update_buffer);
> > static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32
> > *val)
> > {
> > switch (fmt) {
> > - case DRM_FORMAT_ARGB8888:
> > + case DRM_FORMAT_XRGB8888:
> > *val = 5;
> > return 0;
> >
> > @@ -120,7 +120,6 @@ static int
> > sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
> > {
> > switch (fmt) {
> > case DRM_FORMAT_XRGB8888:
> > - case DRM_FORMAT_ARGB8888:
> > *val = 2;
> > return 0;
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > index eb93df445a10..15238211a61a 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > @@ -100,9 +100,9 @@ static void
> > sun4i_backend_layer_atomic_update(struct drm_plane *plane,
> > sun4i_frontend_update_coord(frontend, plane);
> > sun4i_frontend_update_buffer(frontend, plane);
> > sun4i_frontend_update_formats(frontend, plane,
> > - DRM_FORMAT_ARGB8888);
> > + DRM_FORMAT_XRGB8888);
> > sun4i_backend_update_layer_frontend(backend, layer-
> > >id, plane,
> > - DRM_FORMAT_ARGB8
> > 888);
> > + DRM_FORMAT_XRGB8
> > 888);
> > sun4i_frontend_enable(frontend);
> > } else {
> > sun4i_backend_update_layer_formats(backend, layer-
> > >id, plane);
> > --
> > 2.16.2
> >
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
On Thu, Mar 22, 2018 at 4:23 PM, Paul Kocialkowski
<[email protected]> wrote:
> Hi Chen-Yu,
>
> On Thu, 2018-03-22 at 14:47 +0800, Chen-Yu Tsai wrote:
>> On Wed, Mar 21, 2018 at 11:28 PM, Paul Kocialkowski
>> <[email protected]> wrote:
>> > It turns out that the frontend is not capable of preserving the
>> > alpha
>> > component (that is always set to 0xff), so only support XRGB8888
>> > instead.
>> >
>> > Signed-off-by: Paul Kocialkowski <[email protected]>
>> > ---
>> > drivers/gpu/drm/sun4i/sun4i_backend.c | 4 ++++
>> > drivers/gpu/drm/sun4i/sun4i_frontend.c | 3 +--
>> > drivers/gpu/drm/sun4i/sun4i_layer.c | 4 ++--
>> > 3 files changed, 7 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
>> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> > index b98dafda52f8..274a1db6fa8e 100644
>> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
>> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> > @@ -440,6 +440,10 @@ static bool
>> > sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
>> > if (IS_ERR(backend->frontend))
>> > return false;
>> >
>> > + /*
>> > + * TODO: Don't use the frontend for x2/x4 scaling and allow
>> > RGB formats
>> > + * with an alpha component then.
>>
>> This and the commit log are kind of conflicting. Is it just the scalar
>> that doesn't
>> work with an alpha component, or the whole frontend?
>
> It's the whole frontend that does not support alpha in its output
> formats. I'm talking about scaling here because that's still the main
> reason to use the frontend. On the other hand, the backend allows
> integer scaling with an alpha component, hence the comment.
I see. I parsed the TODO note incorrectly.
> Do you think I need to rework the comment/commit log?
I think
Allow integer scaling with alpha using just the backend.
would be slightly clearer.
Thanks
ChenYu
> Thanks for the review,
>
> Paul
>
>> > + */
>> > return sun4i_backend_plane_uses_scaler(state);
>> > }
>> >
>> > diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c
>> > b/drivers/gpu/drm/sun4i/sun4i_frontend.c
>> > index ddf6cfa6dd23..3ea925584891 100644
>> > --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
>> > +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
>> > @@ -107,7 +107,7 @@ EXPORT_SYMBOL(sun4i_frontend_update_buffer);
>> > static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32
>> > *val)
>> > {
>> > switch (fmt) {
>> > - case DRM_FORMAT_ARGB8888:
>> > + case DRM_FORMAT_XRGB8888:
>> > *val = 5;
>> > return 0;
>> >
>> > @@ -120,7 +120,6 @@ static int
>> > sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
>> > {
>> > switch (fmt) {
>> > case DRM_FORMAT_XRGB8888:
>> > - case DRM_FORMAT_ARGB8888:
>> > *val = 2;
>> > return 0;
>> >
>> > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
>> > b/drivers/gpu/drm/sun4i/sun4i_layer.c
>> > index eb93df445a10..15238211a61a 100644
>> > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
>> > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
>> > @@ -100,9 +100,9 @@ static void
>> > sun4i_backend_layer_atomic_update(struct drm_plane *plane,
>> > sun4i_frontend_update_coord(frontend, plane);
>> > sun4i_frontend_update_buffer(frontend, plane);
>> > sun4i_frontend_update_formats(frontend, plane,
>> > - DRM_FORMAT_ARGB8888);
>> > + DRM_FORMAT_XRGB8888);
>> > sun4i_backend_update_layer_frontend(backend, layer-
>> > >id, plane,
>> > - DRM_FORMAT_ARGB8
>> > 888);
>> > + DRM_FORMAT_XRGB8
>> > 888);
>> > sun4i_frontend_enable(frontend);
>> > } else {
>> > sun4i_backend_update_layer_formats(backend, layer-
>> > >id, plane);
>> > --
>> > 2.16.2
>> >
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> --
> Paul Kocialkowski, Bootlin (formerly Free Electrons)
> Embedded Linux and kernel engineering
> https://bootlin.com
Hi,
On Thu, 2018-03-22 at 16:37 +0800, Chen-Yu Tsai wrote:
> On Thu, Mar 22, 2018 at 4:23 PM, Paul Kocialkowski
> <[email protected]> wrote:
> > Hi Chen-Yu,
> >
> > On Thu, 2018-03-22 at 14:47 +0800, Chen-Yu Tsai wrote:
> > > On Wed, Mar 21, 2018 at 11:28 PM, Paul Kocialkowski
> > > <[email protected]> wrote:
> > > > It turns out that the frontend is not capable of preserving the
> > > > alpha
> > > > component (that is always set to 0xff), so only support XRGB8888
> > > > instead.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/sun4i/sun4i_backend.c | 4 ++++
> > > > drivers/gpu/drm/sun4i/sun4i_frontend.c | 3 +--
> > > > drivers/gpu/drm/sun4i/sun4i_layer.c | 4 ++--
> > > > 3 files changed, 7 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > index b98dafda52f8..274a1db6fa8e 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > @@ -440,6 +440,10 @@ static bool
> > > > sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
> > > > if (IS_ERR(backend->frontend))
> > > > return false;
> > > >
> > > > + /*
> > > > + * TODO: Don't use the frontend for x2/x4 scaling and
> > > > allow
> > > > RGB formats
> > > > + * with an alpha component then.
> > >
> > > This and the commit log are kind of conflicting. Is it just the
> > > scalar
> > > that doesn't
> > > work with an alpha component, or the whole frontend?
> >
> > It's the whole frontend that does not support alpha in its output
> > formats. I'm talking about scaling here because that's still the
> > main
> > reason to use the frontend. On the other hand, the backend allows
> > integer scaling with an alpha component, hence the comment.
>
> I see. I parsed the TODO note incorrectly.
>
> > Do you think I need to rework the comment/commit log?
>
> I think
>
> Allow integer scaling with alpha using just the backend.
>
> would be slightly clearer.
I agree, will do in the next version!
Cheers,
Paul
> > Thanks for the review,
> >
> > Paul
> >
> > > > + */
> > > > return sun4i_backend_plane_uses_scaler(state);
> > > > }
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > > > b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > > > index ddf6cfa6dd23..3ea925584891 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > > > @@ -107,7 +107,7 @@ EXPORT_SYMBOL(sun4i_frontend_update_buffer);
> > > > static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt,
> > > > u32
> > > > *val)
> > > > {
> > > > switch (fmt) {
> > > > - case DRM_FORMAT_ARGB8888:
> > > > + case DRM_FORMAT_XRGB8888:
> > > > *val = 5;
> > > > return 0;
> > > >
> > > > @@ -120,7 +120,6 @@ static int
> > > > sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
> > > > {
> > > > switch (fmt) {
> > > > case DRM_FORMAT_XRGB8888:
> > > > - case DRM_FORMAT_ARGB8888:
> > > > *val = 2;
> > > > return 0;
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > index eb93df445a10..15238211a61a 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > @@ -100,9 +100,9 @@ static void
> > > > sun4i_backend_layer_atomic_update(struct drm_plane *plane,
> > > > sun4i_frontend_update_coord(frontend, plane);
> > > > sun4i_frontend_update_buffer(frontend, plane);
> > > > sun4i_frontend_update_formats(frontend, plane,
> > > > - DRM_FORMAT_ARGB888
> > > > 8);
> > > > + DRM_FORMAT_XRGB888
> > > > 8);
> > > > sun4i_backend_update_layer_frontend(backend,
> > > > layer-
> > > > > id, plane,
> > > >
> > > > - DRM_FORMAT_A
> > > > RGB8
> > > > 888);
> > > > + DRM_FORMAT_X
> > > > RGB8
> > > > 888);
> > > > sun4i_frontend_enable(frontend);
> > > > } else {
> > > > sun4i_backend_update_layer_formats(backend,
> > > > layer-
> > > > > id, plane);
> > > >
> > > > --
> > > > 2.16.2
> > > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Paul Kocialkowski, Bootlin (formerly Free Electrons)
> > Embedded Linux and kernel engineering
> > https://bootlin.com
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
Beside the discussion with Chen-Yu,
On Wed, Mar 21, 2018 at 04:28:57PM +0100, Paul Kocialkowski wrote:
> It turns out that the frontend is not capable of preserving the alpha
> component (that is always set to 0xff), so only support XRGB8888
> instead.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
[...]
> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> index eb93df445a10..15238211a61a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> @@ -100,9 +100,9 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
> sun4i_frontend_update_coord(frontend, plane);
> sun4i_frontend_update_buffer(frontend, plane);
> sun4i_frontend_update_formats(frontend, plane,
> - DRM_FORMAT_ARGB8888);
> + DRM_FORMAT_XRGB8888);
> sun4i_backend_update_layer_frontend(backend, layer->id, plane,
> - DRM_FORMAT_ARGB8888);
> + DRM_FORMAT_XRGB8888);
Even though it's slightly related, these changes should be justified
in the commit log. From what you currently explain, this makes 0
difference, since XRGB and ARGB with an alpha component to 0xff is
exactly the same thing.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
On Thu, 2018-03-22 at 17:12 +0100, Maxime Ripard wrote:
> Hi,
>
> Beside the discussion with Chen-Yu,
>
> On Wed, Mar 21, 2018 at 04:28:57PM +0100, Paul Kocialkowski wrote:
> > It turns out that the frontend is not capable of preserving the
> > alpha
> > component (that is always set to 0xff), so only support XRGB8888
> > instead.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
>
> [...]
>
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > index eb93df445a10..15238211a61a 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > @@ -100,9 +100,9 @@ static void
> > sun4i_backend_layer_atomic_update(struct drm_plane *plane,
> > sun4i_frontend_update_coord(frontend, plane);
> > sun4i_frontend_update_buffer(frontend, plane);
> > sun4i_frontend_update_formats(frontend, plane,
> > - DRM_FORMAT_ARGB8888);
> > + DRM_FORMAT_XRGB8888);
> > sun4i_backend_update_layer_frontend(backend, layer-
> > >id, plane,
> > - DRM_FORMAT_ARGB
> > 8888);
> > + DRM_FORMAT_XRGB
> > 8888);
>
> Even though it's slightly related, these changes should be justified
> in the commit log. From what you currently explain, this makes 0
> difference, since XRGB and ARGB with an alpha component to 0xff is
> exactly the same thing.
The point of this is mostly to fix
sun4i_frontend_drm_format_to_input_fmt and
sun4i_frontend_drm_format_to_output_fmt, that were reporting the wrong
format (ARGB has never been supported).
The change in the internal format used between the frontend and backend
is a direct consequence of correcting the supported format, but brings
no functional change.
I will update the commit message in the next revision to make it clear
what the nature of this change is.
Thanks for the review!
Paul
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
On Wed, Mar 21, 2018 at 04:28:55PM +0100, Paul Kocialkowski wrote:
> This adds a dedicated function for disabling the layer video channel
> from the frontend to the backend. It is called automatically on an
> atomic update, as to disable the frontend by default (it will be enabled
> later on in the atomic update if necessary).
>
> It fixes an issue when enabling a layer that uses the frontend (e.g. for
> scaling) and then reusing the same layer without the frontend. Since the
> video channel to the frontend was never disabled, the backend was still
> trying to get data from there.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 21, 2018 at 04:28:56PM +0100, Paul Kocialkowski wrote:
> The YUV channel was only disabled in sun4i_backend_update_layer_formats,
> which is not called when the frontend is selected.
>
> Thus, creating a layer with a YUV format handled by the backend and then
> switching to a format that requires the frontend would keep the YUV
> channel enabled for the layer.
>
> This explicitly disables the YUV channel for the layer when using the
> frontend as well. It also sets the relevant interlace bit, which was
> missing in the frontend path as well.
This should be part of a separate patch. Usually, if you write "it
also does..." at the end of your commit log, it's a pretty good
indication that it should be another patch :)
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_backend.c | 17 ++++++++++++++++-
> drivers/gpu/drm/sun4i/sun4i_backend.h | 3 ++-
> drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
> 3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index e07a33adc51d..b98dafda52f8 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -294,8 +294,10 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
> }
>
> int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend,
> - int layer, uint32_t fmt)
> + int layer, struct drm_plane *plane,
> + uint32_t fmt)
> {
> + bool interlaced = false;
There's no need to pass the full drm_plane pointer, you can just pass
a boolean to tell if it is interlaced or not.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 21, 2018 at 04:29:01PM +0100, Paul Kocialkowski wrote:
> The frontend supports many YUV formats as input and also contains a
> color-space converter (CSC) block that can convert YUV input into
> RGB output. It also allows scaling between the input and output for
> every possible combination of supported formats.
>
> This adds support for all the (untiled) YUV video formats supported by
> the frontend, with associated changes in the backend and layer
> management.
>
> A specific dumb GEM create function translates a hardware constraint,
> that the stride must be an even number, when allocating dumb (linear)
> buffers.
This should be in a separate, potentially preliminary, patch.
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_backend.c | 10 +-
> drivers/gpu/drm/sun4i/sun4i_drv.c | 11 +-
> drivers/gpu/drm/sun4i/sun4i_drv.h | 4 +
> drivers/gpu/drm/sun4i/sun4i_frontend.c | 234 ++++++++++++++++++++++++++++-----
> drivers/gpu/drm/sun4i/sun4i_frontend.h | 48 ++++++-
> drivers/gpu/drm/sun4i/sun4i_layer.c | 34 +++--
> 6 files changed, 293 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index e8af9f3cf20b..3de7f3a427c3 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -500,6 +500,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> layer_state->uses_frontend = true;
> num_frontend_planes++;
> } else {
> + if (sun4i_format_is_yuv(fb->format->format)) {
> + DRM_DEBUG_DRIVER("Plane FB format is YUV\n");
> + num_yuv_planes++;
> + }
> +
> layer_state->uses_frontend = false;
> }
>
> @@ -510,11 +515,6 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> if (fb->format->has_alpha)
> num_alpha_planes++;
>
> - if (sun4i_format_is_yuv(fb->format->format)) {
> - DRM_DEBUG_DRIVER("Plane FB format is YUV\n");
> - num_yuv_planes++;
> - }
> -
Why is this needed?
> DRM_DEBUG_DRIVER("Plane zpos is %d\n",
> plane_state->normalized_zpos);
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index 3957c2ff6870..d374bb61c565 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -42,7 +42,7 @@ static struct drm_driver sun4i_drv_driver = {
> .minor = 0,
>
> /* GEM Operations */
> - .dumb_create = drm_gem_cma_dumb_create,
> + .dumb_create = drm_sun4i_gem_dumb_create,
> .gem_free_object_unlocked = drm_gem_cma_free_object,
> .gem_vm_ops = &drm_gem_cma_vm_ops,
>
> @@ -60,6 +60,15 @@ static struct drm_driver sun4i_drv_driver = {
> /* Frame Buffer Operations */
> };
>
> +int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> + struct drm_device *drm,
> + struct drm_mode_create_dumb *args)
> +{
> + args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp, 8), 2);
> +
> + return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
> +}
> +
> static void sun4i_remove_framebuffers(void)
> {
> struct apertures_struct *ap;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
> index 5750b8ce8b31..47969711a889 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> @@ -23,4 +23,8 @@ struct sun4i_drv {
> struct list_head tcon_list;
> };
>
> +int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> + struct drm_device *drm,
> + struct drm_mode_create_dumb *args);
> +
I'm not sure this is needed, you just need to move the function before
the structure definition.
> #endif /* _SUN4I_DRV_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> index 2dc33383be22..d9e58e96119c 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> @@ -16,6 +16,7 @@
> #include <linux/reset.h>
>
> #include "sun4i_drv.h"
> +#include "sun4i_format.h"
> #include "sun4i_frontend.h"
>
> static const u32 sun4i_frontend_vert_coef[32] = {
> @@ -89,26 +90,135 @@ void sun4i_frontend_update_buffer(struct sun4i_frontend *frontend,
> {
> struct drm_plane_state *state = plane->state;
> struct drm_framebuffer *fb = state->fb;
> + uint32_t format = fb->format->format;
> + uint32_t width, height;
> + uint32_t stride, offset;
> + bool swap;
> dma_addr_t paddr;
>
> - /* Set the line width */
> - DRM_DEBUG_DRIVER("Frontend stride: %d bytes\n", fb->pitches[0]);
Keeping that debug message would be valuable.
> + width = state->src_w >> 16;
> + height = state->src_h >> 16;
> +
You don't seem to be using these values anywhere.
> regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD0_REG,
> fb->pitches[0]);
>
> + if (drm_format_num_planes(format) > 1)
> + regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD1_REG,
> + fb->pitches[1]);
> +
> + if (drm_format_num_planes(format) > 2)
> + regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD2_REG,
> + fb->pitches[2]);
> +
> /* Set the physical address of the buffer in memory */
> paddr = drm_fb_cma_get_gem_addr(fb, state, 0);
> paddr -= PHYS_OFFSET;
> - DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
> + DRM_DEBUG_DRIVER("Setting buffer #0 address to %pad\n", &paddr);
> regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR0_REG, paddr);
> +
> + /* Some planar formats require chroma channel swapping by hand. */
> + swap = sun4i_frontend_format_chroma_requires_swap(format);
> +
> + if (drm_format_num_planes(format) > 1) {
> + paddr = drm_fb_cma_get_gem_addr(fb, state, swap ? 2 : 1);
> + paddr -= PHYS_OFFSET;
> + DRM_DEBUG_DRIVER("Setting buffer #1 address to %pad\n", &paddr);
> + regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR1_REG,
> + paddr);
> + }
> +
> + if (drm_format_num_planes(format) > 2) {
> + paddr = drm_fb_cma_get_gem_addr(fb, state, swap ? 1 : 2);
> + paddr -= PHYS_OFFSET;
> + DRM_DEBUG_DRIVER("Setting buffer #2 address to %pad\n", &paddr);
> + regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR2_REG,
> + paddr);
> + }
> }
> EXPORT_SYMBOL(sun4i_frontend_update_buffer);
>
> static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32 *val)
> {
> + if (sun4i_format_is_rgb(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_RGB;
> + else if (sun4i_format_is_yuv411(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV411;
> + else if (sun4i_format_is_yuv420(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV420;
> + else if (sun4i_format_is_yuv422(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV422;
> + else if (sun4i_format_is_yuv444(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV444;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int sun4i_frontend_drm_format_to_input_mode(uint32_t fmt, u32 *val)
> +{
> + if (sun4i_format_is_packed(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED;
> + else if (sun4i_format_is_semiplanar(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR;
> + else if (sun4i_format_is_planar(fmt))
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int sun4i_frontend_drm_format_to_input_sequence(uint32_t fmt, u32 *val)
> +{
> + /* Planar formats have an explicit input sequence. */
> + if (sun4i_format_is_planar(fmt)) {
> + *val = 0;
> + return 0;
> + }
> +
> switch (fmt) {
> + /* RGB */
> case DRM_FORMAT_XRGB8888:
> - *val = 5;
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_XRGB;
> + return 0;
> +
> + case DRM_FORMAT_BGRX8888:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_BGRX;
> + return 0;
> +
> + /* YUV420 */
> + case DRM_FORMAT_NV12:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV;
> + return 0;
> +
> + case DRM_FORMAT_NV21:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU;
> + return 0;
> +
> + /* YUV422 */
> + case DRM_FORMAT_YUYV:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YUYV;
> + return 0;
> +
> + case DRM_FORMAT_VYUY:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VYUY;
> + return 0;
> +
> + case DRM_FORMAT_YVYU:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YVYU;
> + return 0;
> +
> + case DRM_FORMAT_UYVY:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UYVY;
> + return 0;
> +
> + case DRM_FORMAT_NV16:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV;
> + return 0;
> +
> + case DRM_FORMAT_NV61:
> + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU;
> return 0;
>
> default:
> @@ -120,7 +230,11 @@ static int sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
> {
> switch (fmt) {
> case DRM_FORMAT_XRGB8888:
> - *val = 2;
> + *val = SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_XRGB8888;
> + return 0;
> +
> + case DRM_FORMAT_BGRX8888:
> + *val = SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_BGRX8888;
> return 0;
Are we using this anywhere?
>
> default:
> @@ -172,22 +286,52 @@ bool sun4i_frontend_format_is_supported(uint32_t fmt)
> return true;
> }
>
> +bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt)
> +{
> + switch (fmt) {
> + case DRM_FORMAT_YVU444:
> + case DRM_FORMAT_YVU422:
> + case DRM_FORMAT_YVU420:
> + case DRM_FORMAT_YVU411:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
> struct drm_plane *plane, uint32_t out_fmt)
> {
> struct drm_plane_state *state = plane->state;
> struct drm_framebuffer *fb = state->fb;
> + uint32_t format = fb->format->format;
> u32 out_fmt_val;
> u32 in_fmt_val;
> + u32 in_mod_val;
> + u32 in_ps_val;
> + u32 bypass;
> + unsigned int i;
> int ret;
>
> - ret = sun4i_frontend_drm_format_to_input_fmt(fb->format->format,
> - &in_fmt_val);
> + ret = sun4i_frontend_drm_format_to_input_fmt(format, &in_fmt_val);
> if (ret) {
> DRM_DEBUG_DRIVER("Invalid input format\n");
> return ret;
> }
>
> + ret = sun4i_frontend_drm_format_to_input_mode(format, &in_mod_val);
> + if (ret) {
> + DRM_DEBUG_DRIVER("Invalid input mode\n");
> + return ret;
> + }
> +
> + ret = sun4i_frontend_drm_format_to_input_sequence(format, &in_ps_val);
> + if (ret) {
> + DRM_DEBUG_DRIVER("Invalid pixel sequence\n");
> + return ret;
> + }
> +
> ret = sun4i_frontend_drm_format_to_output_fmt(out_fmt, &out_fmt_val);
> if (ret) {
> DRM_DEBUG_DRIVER("Invalid output format\n");
> @@ -205,10 +349,30 @@ int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
> regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_VERTPHASE1_REG, 0x400);
> regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_VERTPHASE1_REG, 0x400);
>
> + if (sun4i_format_is_yuv(format) &&
> + !sun4i_format_is_yuv(out_fmt)) {
> + /* Setup the CSC engine for YUV to RGB conversion. */
> +
> + for (i = 0; i < ARRAY_SIZE(sunxi_bt601_yuv2rgb_coef); i++)
> + regmap_write(frontend->regs,
> + SUN4I_FRONTEND_CSC_COEF_REG(i),
> + sunxi_bt601_yuv2rgb_coef[i]);
> +
> + regmap_update_bits(frontend->regs,
> + SUN4I_FRONTEND_FRM_CTRL_REG,
> + SUN4I_FRONTEND_FRM_CTRL_COEF_RDY,
> + SUN4I_FRONTEND_FRM_CTRL_COEF_RDY);
> +
> + bypass = 0;
> + } else {
> + bypass = SUN4I_FRONTEND_BYPASS_CSC_EN;
I guess this is also something you introduce that should be in a
separate patch.
> + }
> +
> + regmap_update_bits(frontend->regs, SUN4I_FRONTEND_BYPASS_REG,
> + SUN4I_FRONTEND_BYPASS_CSC_EN, bypass);
> +
> regmap_write(frontend->regs, SUN4I_FRONTEND_INPUT_FMT_REG,
> - SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(1) |
> - SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(in_fmt_val) |
> - SUN4I_FRONTEND_INPUT_FMT_PS(1));
> + in_mod_val | in_fmt_val | in_ps_val);
>
> /*
> * TODO: It look like the A31 and A80 at least will need the
> @@ -216,7 +380,7 @@ int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
> * ARGB8888).
> */
> regmap_write(frontend->regs, SUN4I_FRONTEND_OUTPUT_FMT_REG,
> - SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(out_fmt_val));
> + out_fmt_val);
>
> return 0;
> }
> @@ -226,31 +390,45 @@ void sun4i_frontend_update_coord(struct sun4i_frontend *frontend,
> struct drm_plane *plane)
> {
> struct drm_plane_state *state = plane->state;
> + struct drm_framebuffer *fb = state->fb;
> + uint32_t format = fb->format->format;
> + uint32_t luma_width, luma_height;
> + uint32_t chroma_width, chroma_height;
>
> /* Set height and width */
> - DRM_DEBUG_DRIVER("Frontend size W: %u H: %u\n",
> + DRM_DEBUG_DRIVER("Frontend crtc size W: %u H: %u\n",
The frontend is not the CRTC
> state->crtc_w, state->crtc_h);
> - regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_INSIZE_REG,
> - SUN4I_FRONTEND_INSIZE(state->src_h >> 16,
> - state->src_w >> 16));
> - regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_INSIZE_REG,
> - SUN4I_FRONTEND_INSIZE(state->src_h >> 16,
> - state->src_w >> 16));
>
> + luma_width = chroma_width = state->src_w >> 16;
> + luma_height = chroma_height = state->src_h >> 16;
> +
> + if (sun4i_format_is_yuv411(format)) {
> + chroma_width = DIV_ROUND_UP(luma_width, 4);
> + } else if (sun4i_format_is_yuv420(format)) {
> + chroma_width = DIV_ROUND_UP(luma_width, 2);
> + chroma_height = DIV_ROUND_UP(luma_height, 2);
> + } else if (sun4i_format_is_yuv422(format)) {
> + chroma_width = DIV_ROUND_UP(luma_width, 2);
> + }
> +
> + regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_INSIZE_REG,
> + SUN4I_FRONTEND_INSIZE(luma_height, luma_width));
> regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_OUTSIZE_REG,
> SUN4I_FRONTEND_OUTSIZE(state->crtc_h, state->crtc_w));
> + regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_HORZFACT_REG,
> + (luma_width << 16) / state->crtc_w);
> + regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_VERTFACT_REG,
> + (luma_height << 16) / state->crtc_h);
> +
> + /* These also have to be specified, even for interleaved formats. */
> + regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_INSIZE_REG,
> + SUN4I_FRONTEND_INSIZE(chroma_height, chroma_width));
> regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_OUTSIZE_REG,
> SUN4I_FRONTEND_OUTSIZE(state->crtc_h, state->crtc_w));
> -
> - regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_HORZFACT_REG,
> - state->src_w / state->crtc_w);
> regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_HORZFACT_REG,
> - state->src_w / state->crtc_w);
> -
> - regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_VERTFACT_REG,
> - state->src_h / state->crtc_h);
> + (chroma_width << 16) / state->crtc_w);
> regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_VERTFACT_REG,
> - state->src_h / state->crtc_h);
> + (chroma_height << 16) / state->crtc_h);
>
> regmap_write_bits(frontend->regs, SUN4I_FRONTEND_FRM_CTRL_REG,
> SUN4I_FRONTEND_FRM_CTRL_REG_RDY,
> @@ -382,10 +560,6 @@ static int sun4i_frontend_runtime_resume(struct device *dev)
> SUN4I_FRONTEND_EN_EN,
> SUN4I_FRONTEND_EN_EN);
>
> - regmap_update_bits(frontend->regs, SUN4I_FRONTEND_BYPASS_REG,
> - SUN4I_FRONTEND_BYPASS_CSC_EN,
> - SUN4I_FRONTEND_BYPASS_CSC_EN);
> -
> sun4i_frontend_scaler_init(frontend);
>
> return 0;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.h b/drivers/gpu/drm/sun4i/sun4i_frontend.h
> index a9cb908ced16..6dd1d18752f4 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_frontend.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.h
> @@ -21,17 +21,56 @@
> #define SUN4I_FRONTEND_BYPASS_REG 0x008
> #define SUN4I_FRONTEND_BYPASS_CSC_EN BIT(1)
>
> +#define SUN4I_FRONTEND_AGTH_SEL_REG 0x00C
> +#define SUN4I_FRONTEND_AGTH_SEL_ORIGINAL BIT(8)
> +
> #define SUN4I_FRONTEND_BUF_ADDR0_REG 0x020
> +#define SUN4I_FRONTEND_BUF_ADDR1_REG 0x024
> +#define SUN4I_FRONTEND_BUF_ADDR2_REG 0x028
> +
> +#define SUN4I_FRONTEND_TB_OFF0_REG 0x030
> +#define SUN4I_FRONTEND_TB_OFF1_REG 0x034
> +#define SUN4I_FRONTEND_TB_OFF2_REG 0x038
> +
> +#define SUN4I_FRONTEND_TB_OFF_X1(x1) ((x1) << 16)
> +#define SUN4I_FRONTEND_TB_OFF_Y0(y0) ((y0) << 8)
> +#define SUN4I_FRONTEND_TB_OFF_X0(x0) (x0)
>
> #define SUN4I_FRONTEND_LINESTRD0_REG 0x040
> +#define SUN4I_FRONTEND_LINESTRD1_REG 0x044
> +#define SUN4I_FRONTEND_LINESTRD2_REG 0x048
>
> #define SUN4I_FRONTEND_INPUT_FMT_REG 0x04c
> -#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(mod) ((mod) << 8)
> -#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(fmt) ((fmt) << 4)
> -#define SUN4I_FRONTEND_INPUT_FMT_PS(ps) (ps)
> +
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MASK GENMASK(10, 8)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR (0 << 8)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED (1 << 8)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR (2 << 8)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_PLANAR (4 << 8)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_SEMIPLANAR (6 << 8)
> +
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_MASK GENMASK(6, 4)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV444 (0 << 4)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV422 (1 << 4)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV420 (2 << 4)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV411 (3 << 4)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_RGB (5 << 4)
> +
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_MASK GENMASK(1, 0)
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UYVY 0
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YUYV 1
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VYUY 2
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YVYU 3
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV 0
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU 1
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_XRGB 0
> +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_BGRX 1
>
> #define SUN4I_FRONTEND_OUTPUT_FMT_REG 0x05c
> -#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(fmt) (fmt)
> +#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_BGRX8888 1
> +#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_XRGB8888 2
> +
> +#define SUN4I_FRONTEND_CSC_COEF_REG(c) (0x070 + (0x4 * (c)))
>
> #define SUN4I_FRONTEND_CH0_INSIZE_REG 0x100
> #define SUN4I_FRONTEND_INSIZE(h, w) ((((h) - 1) << 16) | (((w) - 1)))
> @@ -96,5 +135,6 @@ void sun4i_frontend_update_coord(struct sun4i_frontend *frontend,
> int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
> struct drm_plane *plane, uint32_t out_fmt);
> bool sun4i_frontend_format_is_supported(uint32_t fmt);
> +bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt);
>
> #endif /* _SUN4I_FRONTEND_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> index 15238211a61a..a39eed6a0e75 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> @@ -128,19 +128,37 @@ static const struct drm_plane_funcs sun4i_backend_layer_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> };
>
> -static const uint32_t sun4i_backend_layer_formats[] = {
> - DRM_FORMAT_ARGB8888,
> +static const uint32_t sun4i_layer_formats[] = {
> + /* RGB */
> DRM_FORMAT_ARGB4444,
> + DRM_FORMAT_RGBA4444,
> DRM_FORMAT_ARGB1555,
> DRM_FORMAT_RGBA5551,
> - DRM_FORMAT_RGBA4444,
> - DRM_FORMAT_RGB888,
> DRM_FORMAT_RGB565,
> - DRM_FORMAT_UYVY,
> - DRM_FORMAT_VYUY,
> + DRM_FORMAT_RGB888,
> DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_BGRX8888,
> + DRM_FORMAT_ARGB8888,
> + /* YUV444 */
> + DRM_FORMAT_YUV444,
> + DRM_FORMAT_YVU444,
> + /* YUV422 */
> DRM_FORMAT_YUYV,
> DRM_FORMAT_YVYU,
> + DRM_FORMAT_UYVY,
> + DRM_FORMAT_VYUY,
> + DRM_FORMAT_NV16,
> + DRM_FORMAT_NV61,
> + DRM_FORMAT_YUV422,
> + DRM_FORMAT_YVU422,
> + /* YUV420 */
> + DRM_FORMAT_NV12,
> + DRM_FORMAT_NV21,
> + DRM_FORMAT_YUV420,
> + DRM_FORMAT_YVU420,
> + /* YUV411 */
> + DRM_FORMAT_YUV411,
> + DRM_FORMAT_YVU411,
> };
>
> static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
> @@ -157,8 +175,8 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
> /* possible crtcs are set later */
> ret = drm_universal_plane_init(drm, &layer->plane, 0,
> &sun4i_backend_layer_funcs,
> - sun4i_backend_layer_formats,
> - ARRAY_SIZE(sun4i_backend_layer_formats),
> + sun4i_layer_formats,
> + ARRAY_SIZE(sun4i_layer_formats),
> NULL, type, NULL);
I stopped reviewing this, because it should be split in at least the
following commits, possibly more:
- one to add the custom GEM allocator
- one to move the yuv number check in atomic_check
- one to enable the input sequence computation
- one to enable the input mode computation
- one to move the CSC bypass setting from the probe to the update_fomats function
- one to introduce the interleaved YUV formats
- one to introduce the multi-planar YUV formats
- one to rename the sun4i_backend_layer_formats array
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
On Wed, Mar 21, 2018 at 04:29:03PM +0100, Paul Kocialkowski wrote:
> This introduces a dedicated ioctl for allocating tiled buffers in the
> Allwinner MB32 format, that comes with a handful of specific
> constraints. In particular, the stride of the buffers is expected to be
> aligned to 32 bytes.
You should have more details here. What are those constraints, what is
the expected user? Can you use it only for the scanout, like the dumb
buffers, or also for the other devices in the system?
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_drv.c | 96 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_drv.h | 2 +
> include/uapi/drm/sun4i_drm.h | 42 +++++++++++++++++
> 3 files changed, 140 insertions(+)
> create mode 100644 include/uapi/drm/sun4i_drm.h
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index d374bb61c565..e9cb03d34b44 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -21,11 +21,18 @@
> #include <drm/drm_gem_cma_helper.h>
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_of.h>
> +#include <drm/sun4i_drm.h>
>
> #include "sun4i_drv.h"
> #include "sun4i_frontend.h"
> #include "sun4i_framebuffer.h"
> #include "sun4i_tcon.h"
> +#include "sun4i_format.h"
> +
> +static const struct drm_ioctl_desc sun4i_drv_ioctls[] = {
> + DRM_IOCTL_DEF_DRV(SUN4I_GEM_CREATE_TILED, drm_sun4i_gem_create_tiled,
> + DRM_AUTH | DRM_RENDER_ALLOW),
Why do you need DRM_RENDER_ALLOW? as far as I know, this is only
useful for render-nodes.
> +};
>
> DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
>
> @@ -34,6 +41,8 @@ static struct drm_driver sun4i_drv_driver = {
>
> /* Generic Operations */
> .lastclose = drm_fb_helper_lastclose,
> + .ioctls = sun4i_drv_ioctls,
> + .num_ioctls = ARRAY_SIZE(sun4i_drv_ioctls),
> .fops = &sun4i_drv_fops,
> .name = "sun4i-drm",
> .desc = "Allwinner sun4i Display Engine",
> @@ -69,6 +78,93 @@ int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
> }
>
> +int drm_sun4i_gem_create_tiled(struct drm_device *drm, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_sun4i_gem_create_tiled *args = data;
> + struct drm_gem_cma_object *cma_obj;
> + struct drm_gem_object *gem_obj;
> + uint32_t luma_stride, chroma_stride;
> + uint32_t luma_height, chroma_height;
> + int ret;
> +
> + if (!sun4i_format_supports_tiling(args->format))
> + return -EINVAL;
> +
> + memset(args->pitches, 0, sizeof(args->pitches));
> + memset(args->offsets, 0, sizeof(args->offsets));
> +
> + /* Stride and height are aligned to 32 bytes for MB32 tiled format. */
> + luma_stride = ALIGN(args->width, 32);
> + luma_height = ALIGN(args->height, 32);
> +
> + if (sun4i_format_is_semiplanar(args->format)) {
> + chroma_stride = luma_stride;
> +
> + if (sun4i_format_is_yuv420(args->format))
> + chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 32);
> + else if (sun4i_format_is_yuv422(args->format))
> + chroma_height = luma_height;
> + else
> + return -EINVAL;
> +
> + args->pitches[0] = luma_stride;
> + args->pitches[1] = chroma_stride;
> +
> + args->offsets[0] = 0;
> + args->offsets[1] = luma_stride * luma_height;
> +
> + args->size = luma_stride * luma_height +
> + chroma_stride * chroma_height;
> + } else if (sun4i_format_is_planar(args->format)) {
> + if (sun4i_format_is_yuv411(args->format)) {
> + chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 4), 32);
> + chroma_height = luma_height;
> + } if (sun4i_format_is_yuv420(args->format)) {
> + chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
> + chroma_height = ALIGN(DIV_ROUND_UP(args->height, 2), 32);
> + } else if (sun4i_format_is_yuv422(args->format)) {
> + chroma_stride = ALIGN(DIV_ROUND_UP(args->width, 2), 32);
> + chroma_height = luma_height;
> + } else {
> + return -EINVAL;
> + }
> +
> + args->pitches[0] = luma_stride;
> + args->pitches[1] = chroma_stride;
> + args->pitches[2] = chroma_stride;
> +
> + args->offsets[0] = 0;
> + args->offsets[1] = luma_stride * luma_height;
> + args->offsets[2] = luma_stride * luma_height +
> + chroma_stride * chroma_height;
> +
> + args->size = luma_stride * luma_height +
> + chroma_stride * chroma_height * 2;
> + } else {
> + /* No support for packed formats in tiled mode. */
> + return -EINVAL;
> + }
> +
> + cma_obj = drm_gem_cma_create(drm, args->size);
> + if (IS_ERR(cma_obj))
> + return PTR_ERR(cma_obj);
> +
> + gem_obj = &cma_obj->base;
> +
> + /*
> + * allocate a id of idr table where the obj is registered
> + * and handle has the id what user can see.
> + */
> + ret = drm_gem_handle_create(file_priv, gem_obj, &args->handle);
> + /* drop reference from allocate - handle holds it now. */
> + drm_gem_object_put_unlocked(gem_obj);
> + if (ret)
> + return ret;
> +
> + return PTR_ERR_OR_ZERO(cma_obj);
> +}
> +
> static void sun4i_remove_framebuffers(void)
> {
> struct apertures_struct *ap;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i/sun4i_drv.h
> index 47969711a889..308ff4bfcdd5 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> @@ -26,5 +26,7 @@ struct sun4i_drv {
> int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> struct drm_device *drm,
> struct drm_mode_create_dumb *args);
> +int drm_sun4i_gem_create_tiled(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);
Do you need it to be non-static, and part of the header as well?
I'll let the DRM-misc maintainers comment on the validity of the new
ioctl.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 21, 2018 at 04:29:00PM +0100, Paul Kocialkowski wrote:
> This moves the various helpers and tables related to format detection
> and conversion to a dedicated file, while adding a bunch of new helpers
> (especially for YUV and tiling support) along the way.
The addition of new helpers should be in a separate patch.
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_backend.c | 54 ++--------
> drivers/gpu/drm/sun4i/sun4i_format.c | 193 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_format.h | 35 ++++++
> 4 files changed, 235 insertions(+), 48 deletions(-)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.c
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.h
>
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 582607c0c488..c89c42ff803e 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -4,6 +4,7 @@ sun4i-frontend-y += sun4i_frontend.o
>
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
> +sun4i-drm-y += sun4i_format.o
>
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index 1fad0714c70e..e8af9f3cf20b 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -29,6 +29,7 @@
> #include "sun4i_drv.h"
> #include "sun4i_frontend.h"
> #include "sun4i_layer.h"
> +#include "sun4i_format.h"
> #include "sunxi_engine.h"
>
> struct sun4i_backend_quirks {
> @@ -36,50 +37,6 @@ struct sun4i_backend_quirks {
> bool needs_output_muxing;
> };
>
> -static const u32 sunxi_rgb2yuv_coef[12] = {
> - 0x00000107, 0x00000204, 0x00000064, 0x00000108,
> - 0x00003f69, 0x00003ed6, 0x000001c1, 0x00000808,
> - 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
> -};
> -
> -static const u32 sunxi_bt601_yuv2rgb_coef[12] = {
> - 0x000004a7, 0x00001e6f, 0x00001cbf, 0x00000877,
> - 0x000004a7, 0x00000000, 0x00000662, 0x00003211,
> - 0x000004a7, 0x00000812, 0x00000000, 0x00002eb1,
> -};
> -
> -static inline bool sun4i_backend_format_is_planar_yuv(uint32_t format)
> -{
> - switch (format) {
> - case DRM_FORMAT_YUV411:
> - case DRM_FORMAT_YUV422:
> - case DRM_FORMAT_YUV444:
> - return true;
> - default:
> - return false;
> - }
> -}
> -
> -static inline bool sun4i_backend_format_is_packed_yuv422(uint32_t format)
> -{
> - switch (format) {
> - case DRM_FORMAT_YUYV:
> - case DRM_FORMAT_YVYU:
> - case DRM_FORMAT_UYVY:
> - case DRM_FORMAT_VYUY:
> - return true;
> -
> - default:
> - return false;
> - }
> -}
> -
> -static inline bool sun4i_backend_format_is_yuv(uint32_t format)
> -{
> - return sun4i_backend_format_is_planar_yuv(format) ||
> - sun4i_backend_format_is_packed_yuv422(format);
> -}
> -
> static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
> {
> int i;
> @@ -259,7 +216,7 @@ static int sun4i_backend_update_yuv_format(struct sun4i_backend *backend,
> SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN,
> SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN);
>
> - if (sun4i_backend_format_is_packed_yuv422(format))
> + if (sun4i_format_is_packed_yuv422(format))
> val |= SUN4I_BACKEND_IYUVCTL_FBFMT_PACKED_YUV422;
> else
> DRM_DEBUG_DRIVER("Unknown YUV format\n");
> @@ -310,7 +267,7 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
> DRM_DEBUG_DRIVER("Switching display backend interlaced mode %s\n",
> interlaced ? "on" : "off");
>
> - if (sun4i_backend_format_is_yuv(fb->format->format))
> + if (sun4i_format_is_yuv(fb->format->format))
> return sun4i_backend_update_yuv_format(backend, layer, plane);
>
> ret = sun4i_backend_drm_format_to_layer(fb->format->format, &val);
> @@ -404,7 +361,7 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
> */
> paddr -= PHYS_OFFSET;
>
> - if (sun4i_backend_format_is_yuv(fb->format->format))
> + if (sun4i_format_is_yuv(fb->format->format))
> return sun4i_backend_update_yuv_buffer(backend, fb, paddr);
>
> /* Write the 32 lower bits of the address (in bits) */
> @@ -549,10 +506,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> DRM_DEBUG_DRIVER("Plane FB format is %s\n",
> drm_get_format_name(fb->format->format,
> &format_name));
> +
> if (fb->format->has_alpha)
> num_alpha_planes++;
>
> - if (sun4i_backend_format_is_yuv(fb->format->format)) {
> + if (sun4i_format_is_yuv(fb->format->format)) {
> DRM_DEBUG_DRIVER("Plane FB format is YUV\n");
> num_yuv_planes++;
> }
> diff --git a/drivers/gpu/drm/sun4i/sun4i_format.c b/drivers/gpu/drm/sun4i/sun4i_format.c
> new file mode 100644
> index 000000000000..3830767e6d5b
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_format.c
> @@ -0,0 +1,193 @@
> +/*
> + * Copyright (C) 2015-2018 Free Electrons/Bootlin
> + *
> + * Maxime Ripard <[email protected]>
> + * Paul Kocialkowski <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/component.h>
> +#include <linux/kfifo.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_reserved_mem.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_of.h>
> +
> +#include "sun4i_drv.h"
> +#include "sun4i_format.h"
I guess a lot of these headers are not needed.
> +
> +const u32 sunxi_rgb2yuv_coef[12] = {
> + 0x00000107, 0x00000204, 0x00000064, 0x00000108,
> + 0x00003f69, 0x00003ed6, 0x000001c1, 0x00000808,
> + 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
> +};
> +
> +const u32 sunxi_bt601_yuv2rgb_coef[12] = {
> + 0x000004a7, 0x00001e6f, 0x00001cbf, 0x00000877,
> + 0x000004a7, 0x00000000, 0x00000662, 0x00003211,
> + 0x000004a7, 0x00000812, 0x00000000, 0x00002eb1,
> +};
> +
> +bool sun4i_format_is_rgb(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_ARGB4444:
> + case DRM_FORMAT_RGBA4444:
> + case DRM_FORMAT_ARGB1555:
> + case DRM_FORMAT_RGBA5551:
> + case DRM_FORMAT_RGB888:
> + case DRM_FORMAT_RGB565:
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_yuv(uint32_t format)
> +{
> + return sun4i_format_is_yuv411(format) ||
> + sun4i_format_is_yuv420(format) ||
> + sun4i_format_is_yuv422(format) ||
> + sun4i_format_is_yuv444(format);
> +}
> +
> +bool sun4i_format_is_yuv411(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_YUV411:
> + case DRM_FORMAT_YVU411:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_yuv420(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_NV12:
> + case DRM_FORMAT_NV21:
> + case DRM_FORMAT_YUV420:
> + case DRM_FORMAT_YVU420:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_yuv422(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_YUYV:
> + case DRM_FORMAT_YVYU:
> + case DRM_FORMAT_UYVY:
> + case DRM_FORMAT_VYUY:
> + case DRM_FORMAT_NV16:
> + case DRM_FORMAT_NV61:
> + case DRM_FORMAT_YUV422:
> + case DRM_FORMAT_YVU422:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_yuv444(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_YUV444:
> + case DRM_FORMAT_YVU444:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_packed(uint32_t format)
> +{
> + if (sun4i_format_is_rgb(format))
> + return true;
> +
> + switch (format) {
> + case DRM_FORMAT_YUYV:
> + case DRM_FORMAT_YVYU:
> + case DRM_FORMAT_UYVY:
> + case DRM_FORMAT_VYUY:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_semiplanar(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_NV12:
> + case DRM_FORMAT_NV21:
> + case DRM_FORMAT_NV16:
> + case DRM_FORMAT_NV61:
> + case DRM_FORMAT_NV24:
> + case DRM_FORMAT_NV42:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_is_planar(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_YUV410:
> + case DRM_FORMAT_YVU410:
> + case DRM_FORMAT_YUV411:
> + case DRM_FORMAT_YVU411:
> + case DRM_FORMAT_YUV420:
> + case DRM_FORMAT_YVU420:
> + case DRM_FORMAT_YUV422:
> + case DRM_FORMAT_YVU422:
> + case DRM_FORMAT_YUV444:
> + case DRM_FORMAT_YVU444:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +bool sun4i_format_supports_tiling(uint32_t format)
> +{
> + switch (format) {
> + /* Semiplanar */
> + case DRM_FORMAT_NV12:
> + case DRM_FORMAT_NV21:
> + case DRM_FORMAT_NV16:
> + case DRM_FORMAT_NV61:
> + /* Planar */
> + case DRM_FORMAT_YUV420:
> + case DRM_FORMAT_YVU420:
> + case DRM_FORMAT_YUV422:
> + case DRM_FORMAT_YVU422:
> + case DRM_FORMAT_YUV411:
> + case DRM_FORMAT_YVU411:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun4i_format.h b/drivers/gpu/drm/sun4i/sun4i_format.h
> new file mode 100644
> index 000000000000..1d6364cd3681
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_format.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2015-2018 Free Electrons/Bootlin
> + *
> + * Maxime Ripard <[email protected]>
> + * Paul Kocialkowski <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef _SUN4I_FORMAT_H_
> +#define _SUN4I_FORMAT_H_
> +
> +extern const u32 sunxi_rgb2yuv_coef[12];
> +extern const u32 sunxi_bt601_yuv2rgb_coef[12];
> +
> +bool sun4i_format_is_rgb(uint32_t format);
> +bool sun4i_format_is_yuv(uint32_t format);
> +bool sun4i_format_is_yuv411(uint32_t format);
> +bool sun4i_format_is_yuv420(uint32_t format);
> +bool sun4i_format_is_yuv422(uint32_t format);
> +bool sun4i_format_is_yuv444(uint32_t format);
> +bool sun4i_format_is_packed(uint32_t format);
> +bool sun4i_format_is_semiplanar(uint32_t format);
> +bool sun4i_format_is_planar(uint32_t format);
> +bool sun4i_format_supports_tiling(uint32_t format);
If we're going to add so many of them, then we should really consider
to move them to drm_fourcc.c instead. Every one has some variation of
some of these functions, we don't really need to duplicate it all the
time.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 21, 2018 at 04:28:59PM +0100, Paul Kocialkowski wrote:
> In order to check whether the frontend supports a specific format, an
> explicit list and a related helper are introduced.
>
> They are then used to determine whether the frontend can actually support
> the requested format when it was selected to be used.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_backend.c | 5 ++++
> drivers/gpu/drm/sun4i/sun4i_frontend.c | 44 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_frontend.h | 1 +
> 3 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index 7703ba989743..1fad0714c70e 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -532,6 +532,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> struct drm_format_name_buf format_name;
>
> if (sun4i_backend_plane_uses_frontend(plane_state)) {
> + if (!sun4i_frontend_format_is_supported(fb->format->format)) {
> + DRM_DEBUG_DRIVER("Frontend plane check failed\n");
> + return -EINVAL;
> + }
> +
So you're checking if the frontend doesn't support it and if the
backend doesn't support it. Who supports it then? :)
Like I was saying, this should be moved to the previous patch, within
sun4i_backend_plane_uses_frontend.
> +static const uint32_t sun4i_frontend_formats[] = {
> + /* RGB */
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_BGRX8888,
> + /* YUV444 */
> + DRM_FORMAT_YUV444,
> + DRM_FORMAT_YVU444,
> + /* YUV422 */
> + DRM_FORMAT_YUYV,
> + DRM_FORMAT_YVYU,
> + DRM_FORMAT_UYVY,
> + DRM_FORMAT_VYUY,
> + DRM_FORMAT_NV16,
> + DRM_FORMAT_NV61,
> + DRM_FORMAT_YUV422,
> + DRM_FORMAT_YVU422,
> + /* YUV420 */
> + DRM_FORMAT_NV12,
> + DRM_FORMAT_NV21,
> + DRM_FORMAT_YUV420,
> + DRM_FORMAT_YVU420,
> + /* YUV411 */
> + DRM_FORMAT_YUV411,
> + DRM_FORMAT_YVU411,
> +};
I think this list should reflect what the driver currently supports,
not what the hardware supports.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Mar 21, 2018 at 04:28:58PM +0100, Paul Kocialkowski wrote:
> In order to check whether the backend supports a specific format, an
> explicit list and a related helper are introduced.
>
> They are then used to determine whether the frontend should be used for
> a layer, when the format is not supported by the backend.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_backend.c | 48 ++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/sun4i/sun4i_backend.h | 1 +
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index 274a1db6fa8e..7703ba989743 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -172,6 +172,39 @@ static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
> return 0;
> }
>
> +static const uint32_t sun4i_backend_formats[] = {
> + /* RGB */
> + DRM_FORMAT_ARGB4444,
> + DRM_FORMAT_RGBA4444,
> + DRM_FORMAT_ARGB1555,
> + DRM_FORMAT_RGBA5551,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_RGB888,
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_BGRX8888,
> + DRM_FORMAT_ARGB8888,
> + /* YUV422 */
> + DRM_FORMAT_YUYV,
> + DRM_FORMAT_YVYU,
> + DRM_FORMAT_UYVY,
> + DRM_FORMAT_VYUY,
Ordering them by alphabetical order would be better.
> +};
> +
> +bool sun4i_backend_format_is_supported(uint32_t fmt)
> +{
> + bool found = false;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(sun4i_backend_formats); i++) {
> + if (sun4i_backend_formats[i] == fmt) {
> + found = true;
> + break;
return true?
> + }
> + }
> +
> + return found;
> +}
> +
> int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
> int layer, struct drm_plane *plane)
> {
> @@ -436,15 +469,28 @@ static bool sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
> {
> struct sun4i_layer *layer = plane_to_sun4i_layer(state->plane);
> struct sun4i_backend *backend = layer->backend;
> + struct drm_framebuffer *fb = state->fb;
>
> if (IS_ERR(backend->frontend))
> return false;
>
> + /*
> + * Let's pretend that every format is either supported by the backend or
> + * the frontend. This is not true in practice, as some tiling modes are
> + * not supported by either. There is still room to check this later in
> + * the atomic check process.
Then I guess there these tiling modes will not be exposed and we won't
ever get that far, wouldn't we?
> + */
> + if (!sun4i_backend_format_is_supported(fb->format->format))
> + return true;
Even though there's a comment, this is not really natural. We are
checking whether the frontend supports the current plane_state, so it
just makes more sense to check whether the frontend supports the
format, rather than if the backend doesn't support them.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
On Fri, 2018-03-23 at 10:55 +0100, Maxime Ripard wrote:
> On Wed, Mar 21, 2018 at 04:28:56PM +0100, Paul Kocialkowski wrote:
> > The YUV channel was only disabled in
> > sun4i_backend_update_layer_formats,
> > which is not called when the frontend is selected.
> >
> > Thus, creating a layer with a YUV format handled by the backend and
> > then
> > switching to a format that requires the frontend would keep the YUV
> > channel enabled for the layer.
> >
> > This explicitly disables the YUV channel for the layer when using
> > the
> > frontend as well. It also sets the relevant interlace bit, which was
> > missing in the frontend path as well.
>
> This should be part of a separate patch. Usually, if you write "it
> also does..." at the end of your commit log, it's a pretty good
> indication that it should be another patch :)
I must say, I figured that this part was missing in the frontend path by
chance and couldn't really test the feature, so I'm also tempted to drop
it altogether. What do you think?
Also, is interlacing actually used on any of the video outputs we
support? Perhaps RGB?
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 17 ++++++++++++++++-
> > drivers/gpu/drm/sun4i/sun4i_backend.h | 3 ++-
> > drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
> > 3 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index e07a33adc51d..b98dafda52f8 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -294,8 +294,10 @@ int sun4i_backend_update_layer_formats(struct
> > sun4i_backend *backend,
> > }
> >
> > int sun4i_backend_update_layer_frontend(struct sun4i_backend
> > *backend,
> > - int layer, uint32_t fmt)
> > + int layer, struct drm_plane
> > *plane,
> > + uint32_t fmt)
> > {
> > + bool interlaced = false;
>
> There's no need to pass the full drm_plane pointer, you can just pass
> a boolean to tell if it is interlaced or not.
Yes that'll do just as well.
Thanks,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
On Fri, 2018-03-23 at 11:03 +0100, Maxime Ripard wrote:
> On Wed, Mar 21, 2018 at 04:28:58PM +0100, Paul Kocialkowski wrote:
> > In order to check whether the backend supports a specific format, an
> > explicit list and a related helper are introduced.
> >
> > They are then used to determine whether the frontend should be used
> > for
> > a layer, when the format is not supported by the backend.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 48
> > ++++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/sun4i/sun4i_backend.h | 1 +
> > 2 files changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index 274a1db6fa8e..7703ba989743 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -172,6 +172,39 @@ static int
> > sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
> > return 0;
> > }
> >
> > +static const uint32_t sun4i_backend_formats[] = {
> > + /* RGB */
> > + DRM_FORMAT_ARGB4444,
> > + DRM_FORMAT_RGBA4444,
> > + DRM_FORMAT_ARGB1555,
> > + DRM_FORMAT_RGBA5551,
> > + DRM_FORMAT_RGB565,
> > + DRM_FORMAT_RGB888,
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_BGRX8888,
> > + DRM_FORMAT_ARGB8888,
> > + /* YUV422 */
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_VYUY,
>
> Ordering them by alphabetical order would be better.
Frankly I find it a lot harder to read when the formats are not grouped
by "family". This is the drm_fourcc enumeration order, which has some
kind of logic behind it. What is the advantage of alphabetical ordering
here?
> > +};
> > +
> > +bool sun4i_backend_format_is_supported(uint32_t fmt)
> > +{
> > + bool found = false;
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(sun4i_backend_formats); i++) {
> > + if (sun4i_backend_formats[i] == fmt) {
> > + found = true;
> > + break;
>
> return true?
Definitely.
> > + }
> > + }
> > +
> > + return found;
> > +}
> > +
> > int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
> > int layer, struct drm_plane
> > *plane)
> > {
> > @@ -436,15 +469,28 @@ static bool
> > sun4i_backend_plane_uses_frontend(struct drm_plane_state *state)
> > {
> > struct sun4i_layer *layer = plane_to_sun4i_layer(state-
> > >plane);
> > struct sun4i_backend *backend = layer->backend;
> > + struct drm_framebuffer *fb = state->fb;
> >
> > if (IS_ERR(backend->frontend))
> > return false;
> >
> > + /*
> > + * Let's pretend that every format is either supported by
> > the backend or
> > + * the frontend. This is not true in practice, as some
> > tiling modes are
> > + * not supported by either. There is still room to check
> > this later in
> > + * the atomic check process.
>
> Then I guess there these tiling modes will not be exposed and we won't
> ever get that far, wouldn't we?
This comment is indeed a bit irrelevant at this stage given that the
tiling modifier was not introduced yet. So in practice, this never
happens with this patch. I should probably move it to a subsequent one.
> > + */
> > + if (!sun4i_backend_format_is_supported(fb->format->format))
> > + return true;
>
> Even though there's a comment, this is not really natural. We are
> checking whether the frontend supports the current plane_state, so it
> just makes more sense to check whether the frontend supports the
> format, rather than if the backend doesn't support them.
The rationale behind this logic is that we should try to use the backend
first and only use the frontend as a last resort. Some formats are
supported by both and checking that the backend supports a format first
ensures that we don't bring up the frontend without need.
Cheers,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
On Tue, Mar 27, 2018 at 10:00:43AM +0200, Paul Kocialkowski wrote:
> Hi,
>
> On Fri, 2018-03-23 at 10:55 +0100, Maxime Ripard wrote:
> > On Wed, Mar 21, 2018 at 04:28:56PM +0100, Paul Kocialkowski wrote:
> > > The YUV channel was only disabled in
> > > sun4i_backend_update_layer_formats,
> > > which is not called when the frontend is selected.
> > >
> > > Thus, creating a layer with a YUV format handled by the backend and
> > > then
> > > switching to a format that requires the frontend would keep the YUV
> > > channel enabled for the layer.
> > >
> > > This explicitly disables the YUV channel for the layer when using
> > > the
> > > frontend as well. It also sets the relevant interlace bit, which was
> > > missing in the frontend path as well.
> >
> > This should be part of a separate patch. Usually, if you write "it
> > also does..." at the end of your commit log, it's a pretty good
> > indication that it should be another patch :)
>
> I must say, I figured that this part was missing in the frontend path by
> chance and couldn't really test the feature, so I'm also tempted to drop
> it altogether. What do you think?
If you haven't been able to test it, then yeah, don't submit it.
> Also, is interlacing actually used on any of the video outputs we
> support? Perhaps RGB?
Composite would be a better guess :)
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
On Fri, 2018-03-23 at 11:06 +0100, Maxime Ripard wrote:
> On Wed, Mar 21, 2018 at 04:28:59PM +0100, Paul Kocialkowski wrote:
> > In order to check whether the frontend supports a specific format,
> > an
> > explicit list and a related helper are introduced.
> >
> > They are then used to determine whether the frontend can actually
> > support
> > the requested format when it was selected to be used.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 5 ++++
> > drivers/gpu/drm/sun4i/sun4i_frontend.c | 44
> > ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/sun4i/sun4i_frontend.h | 1 +
> > 3 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index 7703ba989743..1fad0714c70e 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -532,6 +532,11 @@ static int sun4i_backend_atomic_check(struct
> > sunxi_engine *engine,
> > struct drm_format_name_buf format_name;
> >
> > if (sun4i_backend_plane_uses_frontend(plane_state))
> > {
> > + if (!sun4i_frontend_format_is_supported(fb-
> > >format->format)) {
> > + DRM_DEBUG_DRIVER("Frontend plane
> > check failed\n");
> > + return -EINVAL;
> > + }
> > +
>
> So you're checking if the frontend doesn't support it and if the
> backend doesn't support it. Who supports it then? :)
That's the case case where the format is not supported by either of the
hardware blocks. For instance, requesting ARGB with tiling will fail,
although ARGB without tiling will work. It seems that modetest assumes
that modifiers only apply to YUV (in which case there are no unsupported
cases), but I think userspace might still request RGB+tiling
combinations. I don't currently see a way to report to which format the
tiling applies (thus we have to expect that it can come with any of the
supported formats).
> Like I was saying, this should be moved to the previous patch, within
> sun4i_backend_plane_uses_frontend.
If we get both tests (no backend support and frontend support) into one
function, we cannot detect that the format is actually unsupported and
return an error in the atomic check.
> > +static const uint32_t sun4i_frontend_formats[] = {
> > + /* RGB */
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_BGRX8888,
> > + /* YUV444 */
> > + DRM_FORMAT_YUV444,
> > + DRM_FORMAT_YVU444,
> > + /* YUV422 */
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_VYUY,
> > + DRM_FORMAT_NV16,
> > + DRM_FORMAT_NV61,
> > + DRM_FORMAT_YUV422,
> > + DRM_FORMAT_YVU422,
> > + /* YUV420 */
> > + DRM_FORMAT_NV12,
> > + DRM_FORMAT_NV21,
> > + DRM_FORMAT_YUV420,
> > + DRM_FORMAT_YVU420,
> > + /* YUV411 */
> > + DRM_FORMAT_YUV411,
> > + DRM_FORMAT_YVU411,
> > +};
>
> I think this list should reflect what the driver currently supports,
> not what the hardware supports.
This list is not indended to reflect what the driver supports, but only
what the backend can support (to correctly select whether a
format/modifier couple should go to the frontend or backend)!
The complete list that is reported to userspace for global driver
support is sun4i_layer_formats in sun4i_layer.c.
Cheers,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
On Fri, 2018-03-23 at 11:13 +0100, Maxime Ripard wrote:
> On Wed, Mar 21, 2018 at 04:29:00PM +0100, Paul Kocialkowski wrote:
> > This moves the various helpers and tables related to format
> > detection
> > and conversion to a dedicated file, while adding a bunch of new
> > helpers
> > (especially for YUV and tiling support) along the way.
>
> The addition of new helpers should be in a separate patch.
Fair enough.
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/Makefile | 1 +
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 54 ++--------
> > drivers/gpu/drm/sun4i/sun4i_format.c | 193
> > ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/sun4i/sun4i_format.h | 35 ++++++
> > 4 files changed, 235 insertions(+), 48 deletions(-)
> > create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.c
> > create mode 100644 drivers/gpu/drm/sun4i/sun4i_format.h
> >
> > diff --git a/drivers/gpu/drm/sun4i/Makefile
> > b/drivers/gpu/drm/sun4i/Makefile
> > index 582607c0c488..c89c42ff803e 100644
> > --- a/drivers/gpu/drm/sun4i/Makefile
> > +++ b/drivers/gpu/drm/sun4i/Makefile
> > @@ -4,6 +4,7 @@ sun4i-frontend-y += sun4i_frontend.o
> >
> > sun4i-drm-y += sun4i_drv.o
> > sun4i-drm-y += sun4i_framebuffer.o
> > +sun4i-drm-y += sun4i_format.o
> >
> > sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> > sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index 1fad0714c70e..e8af9f3cf20b 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -29,6 +29,7 @@
> > #include "sun4i_drv.h"
> > #include "sun4i_frontend.h"
> > #include "sun4i_layer.h"
> > +#include "sun4i_format.h"
> > #include "sunxi_engine.h"
> >
> > struct sun4i_backend_quirks {
> > @@ -36,50 +37,6 @@ struct sun4i_backend_quirks {
> > bool needs_output_muxing;
> > };
> >
> > -static const u32 sunxi_rgb2yuv_coef[12] = {
> > - 0x00000107, 0x00000204, 0x00000064, 0x00000108,
> > - 0x00003f69, 0x00003ed6, 0x000001c1, 0x00000808,
> > - 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
> > -};
> > -
> > -static const u32 sunxi_bt601_yuv2rgb_coef[12] = {
> > - 0x000004a7, 0x00001e6f, 0x00001cbf, 0x00000877,
> > - 0x000004a7, 0x00000000, 0x00000662, 0x00003211,
> > - 0x000004a7, 0x00000812, 0x00000000, 0x00002eb1,
> > -};
> > -
> > -static inline bool sun4i_backend_format_is_planar_yuv(uint32_t
> > format)
> > -{
> > - switch (format) {
> > - case DRM_FORMAT_YUV411:
> > - case DRM_FORMAT_YUV422:
> > - case DRM_FORMAT_YUV444:
> > - return true;
> > - default:
> > - return false;
> > - }
> > -}
> > -
> > -static inline bool sun4i_backend_format_is_packed_yuv422(uint32_t
> > format)
> > -{
> > - switch (format) {
> > - case DRM_FORMAT_YUYV:
> > - case DRM_FORMAT_YVYU:
> > - case DRM_FORMAT_UYVY:
> > - case DRM_FORMAT_VYUY:
> > - return true;
> > -
> > - default:
> > - return false;
> > - }
> > -}
> > -
> > -static inline bool sun4i_backend_format_is_yuv(uint32_t format)
> > -{
> > - return sun4i_backend_format_is_planar_yuv(format) ||
> > - sun4i_backend_format_is_packed_yuv422(format);
> > -}
> > -
> > static void sun4i_backend_apply_color_correction(struct
> > sunxi_engine *engine)
> > {
> > int i;
> > @@ -259,7 +216,7 @@ static int
> > sun4i_backend_update_yuv_format(struct sun4i_backend *backend,
> > SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN,
> > SUN4I_BACKEND_ATTCTL_REG0_LAY_YUVEN);
> >
> > - if (sun4i_backend_format_is_packed_yuv422(format))
> > + if (sun4i_format_is_packed_yuv422(format))
> > val |= SUN4I_BACKEND_IYUVCTL_FBFMT_PACKED_YUV422;
> > else
> > DRM_DEBUG_DRIVER("Unknown YUV format\n");
> > @@ -310,7 +267,7 @@ int sun4i_backend_update_layer_formats(struct
> > sun4i_backend *backend,
> > DRM_DEBUG_DRIVER("Switching display backend interlaced mode
> > %s\n",
> > interlaced ? "on" : "off");
> >
> > - if (sun4i_backend_format_is_yuv(fb->format->format))
> > + if (sun4i_format_is_yuv(fb->format->format))
> > return sun4i_backend_update_yuv_format(backend,
> > layer, plane);
> >
> > ret = sun4i_backend_drm_format_to_layer(fb->format->format,
> > &val);
> > @@ -404,7 +361,7 @@ int sun4i_backend_update_layer_buffer(struct
> > sun4i_backend *backend,
> > */
> > paddr -= PHYS_OFFSET;
> >
> > - if (sun4i_backend_format_is_yuv(fb->format->format))
> > + if (sun4i_format_is_yuv(fb->format->format))
> > return sun4i_backend_update_yuv_buffer(backend, fb,
> > paddr);
> >
> > /* Write the 32 lower bits of the address (in bits) */
> > @@ -549,10 +506,11 @@ static int sun4i_backend_atomic_check(struct
> > sunxi_engine *engine,
> > DRM_DEBUG_DRIVER("Plane FB format is %s\n",
> > drm_get_format_name(fb->format-
> > >format,
> > &format_name))
> > ;
> > +
> > if (fb->format->has_alpha)
> > num_alpha_planes++;
> >
> > - if (sun4i_backend_format_is_yuv(fb->format-
> > >format)) {
> > + if (sun4i_format_is_yuv(fb->format->format)) {
> > DRM_DEBUG_DRIVER("Plane FB format is
> > YUV\n");
> > num_yuv_planes++;
> > }
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_format.c
> > b/drivers/gpu/drm/sun4i/sun4i_format.c
> > new file mode 100644
> > index 000000000000..3830767e6d5b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sun4i/sun4i_format.c
> > @@ -0,0 +1,193 @@
> > +/*
> > + * Copyright (C) 2015-2018 Free Electrons/Bootlin
> > + *
> > + * Maxime Ripard <[email protected]>
> > + * Paul Kocialkowski <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/component.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/of_reserved_mem.h>
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_fb_cma_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_of.h>
> > +
> > +#include "sun4i_drv.h"
> > +#include "sun4i_format.h"
>
> I guess a lot of these headers are not needed.
Yes indeed, I'll cleanup the list in v2.
> > +
> > +const u32 sunxi_rgb2yuv_coef[12] = {
> > + 0x00000107, 0x00000204, 0x00000064, 0x00000108,
> > + 0x00003f69, 0x00003ed6, 0x000001c1, 0x00000808,
> > + 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
> > +};
> > +
> > +const u32 sunxi_bt601_yuv2rgb_coef[12] = {
> > + 0x000004a7, 0x00001e6f, 0x00001cbf, 0x00000877,
> > + 0x000004a7, 0x00000000, 0x00000662, 0x00003211,
> > + 0x000004a7, 0x00000812, 0x00000000, 0x00002eb1,
> > +};
> > +
> > +bool sun4i_format_is_rgb(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_ARGB4444:
> > + case DRM_FORMAT_RGBA4444:
> > + case DRM_FORMAT_ARGB1555:
> > + case DRM_FORMAT_RGBA5551:
> > + case DRM_FORMAT_RGB888:
> > + case DRM_FORMAT_RGB565:
> > + case DRM_FORMAT_XRGB8888:
> > + case DRM_FORMAT_ARGB8888:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_yuv(uint32_t format)
> > +{
> > + return sun4i_format_is_yuv411(format) ||
> > + sun4i_format_is_yuv420(format) ||
> > + sun4i_format_is_yuv422(format) ||
> > + sun4i_format_is_yuv444(format);
> > +}
> > +
> > +bool sun4i_format_is_yuv411(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_YUV411:
> > + case DRM_FORMAT_YVU411:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_yuv420(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_NV12:
> > + case DRM_FORMAT_NV21:
> > + case DRM_FORMAT_YUV420:
> > + case DRM_FORMAT_YVU420:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_yuv422(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_YUYV:
> > + case DRM_FORMAT_YVYU:
> > + case DRM_FORMAT_UYVY:
> > + case DRM_FORMAT_VYUY:
> > + case DRM_FORMAT_NV16:
> > + case DRM_FORMAT_NV61:
> > + case DRM_FORMAT_YUV422:
> > + case DRM_FORMAT_YVU422:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_yuv444(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_YUV444:
> > + case DRM_FORMAT_YVU444:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_packed(uint32_t format)
> > +{
> > + if (sun4i_format_is_rgb(format))
> > + return true;
> > +
> > + switch (format) {
> > + case DRM_FORMAT_YUYV:
> > + case DRM_FORMAT_YVYU:
> > + case DRM_FORMAT_UYVY:
> > + case DRM_FORMAT_VYUY:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_semiplanar(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_NV12:
> > + case DRM_FORMAT_NV21:
> > + case DRM_FORMAT_NV16:
> > + case DRM_FORMAT_NV61:
> > + case DRM_FORMAT_NV24:
> > + case DRM_FORMAT_NV42:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_is_planar(uint32_t format)
> > +{
> > + switch (format) {
> > + case DRM_FORMAT_YUV410:
> > + case DRM_FORMAT_YVU410:
> > + case DRM_FORMAT_YUV411:
> > + case DRM_FORMAT_YVU411:
> > + case DRM_FORMAT_YUV420:
> > + case DRM_FORMAT_YVU420:
> > + case DRM_FORMAT_YUV422:
> > + case DRM_FORMAT_YVU422:
> > + case DRM_FORMAT_YUV444:
> > + case DRM_FORMAT_YVU444:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +bool sun4i_format_supports_tiling(uint32_t format)
> > +{
> > + switch (format) {
> > + /* Semiplanar */
> > + case DRM_FORMAT_NV12:
> > + case DRM_FORMAT_NV21:
> > + case DRM_FORMAT_NV16:
> > + case DRM_FORMAT_NV61:
> > + /* Planar */
> > + case DRM_FORMAT_YUV420:
> > + case DRM_FORMAT_YVU420:
> > + case DRM_FORMAT_YUV422:
> > + case DRM_FORMAT_YVU422:
> > + case DRM_FORMAT_YUV411:
> > + case DRM_FORMAT_YVU411:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_format.h
> > b/drivers/gpu/drm/sun4i/sun4i_format.h
> > new file mode 100644
> > index 000000000000..1d6364cd3681
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sun4i/sun4i_format.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * Copyright (C) 2015-2018 Free Electrons/Bootlin
> > + *
> > + * Maxime Ripard <[email protected]>
> > + * Paul Kocialkowski <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#ifndef _SUN4I_FORMAT_H_
> > +#define _SUN4I_FORMAT_H_
> > +
> > +extern const u32 sunxi_rgb2yuv_coef[12];
> > +extern const u32 sunxi_bt601_yuv2rgb_coef[12];
> > +
> > +bool sun4i_format_is_rgb(uint32_t format);
> > +bool sun4i_format_is_yuv(uint32_t format);
> > +bool sun4i_format_is_yuv411(uint32_t format);
> > +bool sun4i_format_is_yuv420(uint32_t format);
> > +bool sun4i_format_is_yuv422(uint32_t format);
> > +bool sun4i_format_is_yuv444(uint32_t format);
> > +bool sun4i_format_is_packed(uint32_t format);
> > +bool sun4i_format_is_semiplanar(uint32_t format);
> > +bool sun4i_format_is_planar(uint32_t format);
> > +bool sun4i_format_supports_tiling(uint32_t format);
>
> If we're going to add so many of them, then we should really consider
> to move them to drm_fourcc.c instead. Every one has some variation of
> some of these functions, we don't really need to duplicate it all the
> time.
Should I try to get that through in this patchset and have sun4i-drm be
their first user? Also, does introducing such a change require
identifying duplicates of these functions in each DRM driver's codebase?
Cheers,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
On Fri, 2018-03-23 at 11:30 +0100, Maxime Ripard wrote:
> On Wed, Mar 21, 2018 at 04:29:01PM +0100, Paul Kocialkowski wrote:
> > The frontend supports many YUV formats as input and also contains a
> > color-space converter (CSC) block that can convert YUV input into
> > RGB output. It also allows scaling between the input and output for
> > every possible combination of supported formats.
> >
> > This adds support for all the (untiled) YUV video formats supported
> > by
> > the frontend, with associated changes in the backend and layer
> > management.
> >
> > A specific dumb GEM create function translates a hardware
> > constraint,
> > that the stride must be an even number, when allocating dumb
> > (linear)
> > buffers.
>
> This should be in a separate, potentially preliminary, patch.
Sure thing.
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 10 +-
> > drivers/gpu/drm/sun4i/sun4i_drv.c | 11 +-
> > drivers/gpu/drm/sun4i/sun4i_drv.h | 4 +
> > drivers/gpu/drm/sun4i/sun4i_frontend.c | 234
> > ++++++++++++++++++++++++++++-----
> > drivers/gpu/drm/sun4i/sun4i_frontend.h | 48 ++++++-
> > drivers/gpu/drm/sun4i/sun4i_layer.c | 34 +++--
> > 6 files changed, 293 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index e8af9f3cf20b..3de7f3a427c3 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -500,6 +500,11 @@ static int sun4i_backend_atomic_check(struct
> > sunxi_engine *engine,
> > layer_state->uses_frontend = true;
> > num_frontend_planes++;
> > } else {
> > + if (sun4i_format_is_yuv(fb->format-
> > >format)) {
> > + DRM_DEBUG_DRIVER("Plane FB format
> > is YUV\n");
> > + num_yuv_planes++;
> > + }
> > +
> > layer_state->uses_frontend = false;
> > }
> >
> > @@ -510,11 +515,6 @@ static int sun4i_backend_atomic_check(struct
> > sunxi_engine *engine,
> > if (fb->format->has_alpha)
> > num_alpha_planes++;
> >
> > - if (sun4i_format_is_yuv(fb->format->format)) {
> > - DRM_DEBUG_DRIVER("Plane FB format is
> > YUV\n");
> > - num_yuv_planes++;
> > - }
> > -
>
> Why is this needed?
I forgot to comment on this in the commit message. We only need to count
YUV planes when they come directly to the backend, not when they are
piped through the frontend (which outputs RGB, not YUV, to the
frontend).
> > DRM_DEBUG_DRIVER("Plane zpos is %d\n",
> > plane_state->normalized_zpos);
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > index 3957c2ff6870..d374bb61c565 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > @@ -42,7 +42,7 @@ static struct drm_driver sun4i_drv_driver = {
> > .minor = 0,
> >
> > /* GEM Operations */
> > - .dumb_create = drm_gem_cma_dumb_create,
> > + .dumb_create = drm_sun4i_gem_dumb_create,
> > .gem_free_object_unlocked = drm_gem_cma_free_object,
> > .gem_vm_ops = &drm_gem_cma_vm_ops,
> >
> > @@ -60,6 +60,15 @@ static struct drm_driver sun4i_drv_driver = {
> > /* Frame Buffer Operations */
> > };
> >
> > +int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> > + struct drm_device *drm,
> > + struct drm_mode_create_dumb *args)
> > +{
> > + args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp,
> > 8), 2);
> > +
> > + return drm_gem_cma_dumb_create_internal(file_priv, drm,
> > args);
> > +}
> > +
> > static void sun4i_remove_framebuffers(void)
> > {
> > struct apertures_struct *ap;
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h
> > b/drivers/gpu/drm/sun4i/sun4i_drv.h
> > index 5750b8ce8b31..47969711a889 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> > @@ -23,4 +23,8 @@ struct sun4i_drv {
> > struct list_head tcon_list;
> > };
> >
> > +int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> > + struct drm_device *drm,
> > + struct drm_mode_create_dumb *args);
> > +
>
> I'm not sure this is needed, you just need to move the function before
> the structure definition.
Well, the sun4i_drv_driver definition is kind of at the start of the
file, so it felt better readability-wise to keep functions below it
instead of having the structure definition in a sandwich of functions.
> > #endif /* _SUN4I_DRV_H_ */
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > index 2dc33383be22..d9e58e96119c 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> > @@ -16,6 +16,7 @@
> > #include <linux/reset.h>
> >
> > #include "sun4i_drv.h"
> > +#include "sun4i_format.h"
> > #include "sun4i_frontend.h"
> >
> > static const u32 sun4i_frontend_vert_coef[32] = {
> > @@ -89,26 +90,135 @@ void sun4i_frontend_update_buffer(struct
> > sun4i_frontend *frontend,
> > {
> > struct drm_plane_state *state = plane->state;
> > struct drm_framebuffer *fb = state->fb;
> > + uint32_t format = fb->format->format;
> > + uint32_t width, height;
> > + uint32_t stride, offset;
> > + bool swap;
> > dma_addr_t paddr;
> >
> > - /* Set the line width */
> > - DRM_DEBUG_DRIVER("Frontend stride: %d bytes\n", fb-
> > >pitches[0]);
>
> Keeping that debug message would be valuable.
Agreed.
> > + width = state->src_w >> 16;
> > + height = state->src_h >> 16;
> > +
>
> You don't seem to be using these values anywhere.
You're right, width is only used for tiling adaptation, so it should be
introduced later. Height is totally unused so it will be dropped.
> > regmap_write(frontend->regs, SUN4I_FRONTEND_LINESTRD0_REG,
> > fb->pitches[0]);
> >
> > + if (drm_format_num_planes(format) > 1)
> > + regmap_write(frontend->regs,
> > SUN4I_FRONTEND_LINESTRD1_REG,
> > + fb->pitches[1]);
> > +
> > + if (drm_format_num_planes(format) > 2)
> > + regmap_write(frontend->regs,
> > SUN4I_FRONTEND_LINESTRD2_REG,
> > + fb->pitches[2]);
> > +
> > /* Set the physical address of the buffer in memory */
> > paddr = drm_fb_cma_get_gem_addr(fb, state, 0);
> > paddr -= PHYS_OFFSET;
> > - DRM_DEBUG_DRIVER("Setting buffer address to %pad\n",
> > &paddr);
> > + DRM_DEBUG_DRIVER("Setting buffer #0 address to %pad\n",
> > &paddr);
> > regmap_write(frontend->regs, SUN4I_FRONTEND_BUF_ADDR0_REG,
> > paddr);
> > +
> > + /* Some planar formats require chroma channel swapping by
> > hand. */
> > + swap = sun4i_frontend_format_chroma_requires_swap(format);
> > +
> > + if (drm_format_num_planes(format) > 1) {
> > + paddr = drm_fb_cma_get_gem_addr(fb, state, swap ? 2
> > : 1);
> > + paddr -= PHYS_OFFSET;
> > + DRM_DEBUG_DRIVER("Setting buffer #1 address to
> > %pad\n", &paddr);
> > + regmap_write(frontend->regs,
> > SUN4I_FRONTEND_BUF_ADDR1_REG,
> > + paddr);
> > + }
> > +
> > + if (drm_format_num_planes(format) > 2) {
> > + paddr = drm_fb_cma_get_gem_addr(fb, state, swap ? 1
> > : 2);
> > + paddr -= PHYS_OFFSET;
> > + DRM_DEBUG_DRIVER("Setting buffer #2 address to
> > %pad\n", &paddr);
> > + regmap_write(frontend->regs,
> > SUN4I_FRONTEND_BUF_ADDR2_REG,
> > + paddr);
> > + }
> > }
> > EXPORT_SYMBOL(sun4i_frontend_update_buffer);
> >
> > static int sun4i_frontend_drm_format_to_input_fmt(uint32_t fmt, u32
> > *val)
> > {
> > + if (sun4i_format_is_rgb(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_RGB;
> > + else if (sun4i_format_is_yuv411(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV411;
> > + else if (sun4i_format_is_yuv420(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV420;
> > + else if (sun4i_format_is_yuv422(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV422;
> > + else if (sun4i_format_is_yuv444(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV444;
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int sun4i_frontend_drm_format_to_input_mode(uint32_t fmt,
> > u32 *val)
> > +{
> > + if (sun4i_format_is_packed(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED;
> > + else if (sun4i_format_is_semiplanar(fmt))
> > + *val =
> > SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR;
> > + else if (sun4i_format_is_planar(fmt))
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR;
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int sun4i_frontend_drm_format_to_input_sequence(uint32_t
> > fmt, u32 *val)
> > +{
> > + /* Planar formats have an explicit input sequence. */
> > + if (sun4i_format_is_planar(fmt)) {
> > + *val = 0;
> > + return 0;
> > + }
> > +
> > switch (fmt) {
> > + /* RGB */
> > case DRM_FORMAT_XRGB8888:
> > - *val = 5;
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_XRGB;
> > + return 0;
> > +
> > + case DRM_FORMAT_BGRX8888:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_BGRX;
> > + return 0;
> > +
> > + /* YUV420 */
> > + case DRM_FORMAT_NV12:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV;
> > + return 0;
> > +
> > + case DRM_FORMAT_NV21:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU;
> > + return 0;
> > +
> > + /* YUV422 */
> > + case DRM_FORMAT_YUYV:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YUYV;
> > + return 0;
> > +
> > + case DRM_FORMAT_VYUY:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VYUY;
> > + return 0;
> > +
> > + case DRM_FORMAT_YVYU:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YVYU;
> > + return 0;
> > +
> > + case DRM_FORMAT_UYVY:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UYVY;
> > + return 0;
> > +
> > + case DRM_FORMAT_NV16:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV;
> > + return 0;
> > +
> > + case DRM_FORMAT_NV61:
> > + *val = SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU;
> > return 0;
> >
> > default:
> > @@ -120,7 +230,11 @@ static int
> > sun4i_frontend_drm_format_to_output_fmt(uint32_t fmt, u32 *val)
> > {
> > switch (fmt) {
> > case DRM_FORMAT_XRGB8888:
> > - *val = 2;
> > + *val = SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_XRGB8888;
> > + return 0;
> > +
> > + case DRM_FORMAT_BGRX8888:
> > + *val = SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_BGRX8888;
> > return 0;
>
> Are we using this anywhere?
I don't think so. It seems simple enough to be kept around in case we
need it someday, but I don't really have strong opinions about it.
> >
> > default:
> > @@ -172,22 +286,52 @@ bool
> > sun4i_frontend_format_is_supported(uint32_t fmt)
> > return true;
> > }
> >
> > +bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt)
> > +{
> > + switch (fmt) {
> > + case DRM_FORMAT_YVU444:
> > + case DRM_FORMAT_YVU422:
> > + case DRM_FORMAT_YVU420:
> > + case DRM_FORMAT_YVU411:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
> > struct drm_plane *plane, uint32_t
> > out_fmt)
> > {
> > struct drm_plane_state *state = plane->state;
> > struct drm_framebuffer *fb = state->fb;
> > + uint32_t format = fb->format->format;
> > u32 out_fmt_val;
> > u32 in_fmt_val;
> > + u32 in_mod_val;
> > + u32 in_ps_val;
> > + u32 bypass;
> > + unsigned int i;
> > int ret;
> >
> > - ret = sun4i_frontend_drm_format_to_input_fmt(fb->format-
> > >format,
> > - &in_fmt_val);
> > + ret = sun4i_frontend_drm_format_to_input_fmt(format,
> > &in_fmt_val);
> > if (ret) {
> > DRM_DEBUG_DRIVER("Invalid input format\n");
> > return ret;
> > }
> >
> > + ret = sun4i_frontend_drm_format_to_input_mode(format,
> > &in_mod_val);
> > + if (ret) {
> > + DRM_DEBUG_DRIVER("Invalid input mode\n");
> > + return ret;
> > + }
> > +
> > + ret = sun4i_frontend_drm_format_to_input_sequence(format,
> > &in_ps_val);
> > + if (ret) {
> > + DRM_DEBUG_DRIVER("Invalid pixel sequence\n");
> > + return ret;
> > + }
> > +
> > ret = sun4i_frontend_drm_format_to_output_fmt(out_fmt,
> > &out_fmt_val);
> > if (ret) {
> > DRM_DEBUG_DRIVER("Invalid output format\n");
> > @@ -205,10 +349,30 @@ int sun4i_frontend_update_formats(struct
> > sun4i_frontend *frontend,
> > regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH0_VERTPHASE1_REG, 0x400);
> > regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH1_VERTPHASE1_REG, 0x400);
> >
> > + if (sun4i_format_is_yuv(format) &&
> > + !sun4i_format_is_yuv(out_fmt)) {
> > + /* Setup the CSC engine for YUV to RGB conversion.
> > */
> > +
> > + for (i = 0; i <
> > ARRAY_SIZE(sunxi_bt601_yuv2rgb_coef); i++)
> > + regmap_write(frontend->regs,
> > + SUN4I_FRONTEND_CSC_COEF_REG(i)
> > ,
> > + sunxi_bt601_yuv2rgb_coef[i]);
> > +
> > + regmap_update_bits(frontend->regs,
> > + SUN4I_FRONTEND_FRM_CTRL_REG,
> > + SUN4I_FRONTEND_FRM_CTRL_COEF_RDY
> > ,
> > + SUN4I_FRONTEND_FRM_CTRL_COEF_RDY
> > );
> > +
> > + bypass = 0;
> > + } else {
> > + bypass = SUN4I_FRONTEND_BYPASS_CSC_EN;
>
> I guess this is also something you introduce that should be in a
> separate patch.
Right, since we're moving the CSC bypass control around, I agree it's
best to have it as a preleminary patch for YUV support.
> > + }
> > +
> > + regmap_update_bits(frontend->regs,
> > SUN4I_FRONTEND_BYPASS_REG,
> > + SUN4I_FRONTEND_BYPASS_CSC_EN, bypass);
> > +
> > regmap_write(frontend->regs, SUN4I_FRONTEND_INPUT_FMT_REG,
> > - SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(1) |
> > - SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(in_fmt_val)
> > |
> > - SUN4I_FRONTEND_INPUT_FMT_PS(1));
> > + in_mod_val | in_fmt_val | in_ps_val);
> >
> > /*
> > * TODO: It look like the A31 and A80 at least will need
> > the
> > @@ -216,7 +380,7 @@ int sun4i_frontend_update_formats(struct
> > sun4i_frontend *frontend,
> > * ARGB8888).
> > */
> > regmap_write(frontend->regs, SUN4I_FRONTEND_OUTPUT_FMT_REG,
> > - SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(out_fmt_val
> > ));
> > + out_fmt_val);
> >
> > return 0;
> > }
> > @@ -226,31 +390,45 @@ void sun4i_frontend_update_coord(struct
> > sun4i_frontend *frontend,
> > struct drm_plane *plane)
> > {
> > struct drm_plane_state *state = plane->state;
> > + struct drm_framebuffer *fb = state->fb;
> > + uint32_t format = fb->format->format;
> > + uint32_t luma_width, luma_height;
> > + uint32_t chroma_width, chroma_height;
> >
> > /* Set height and width */
> > - DRM_DEBUG_DRIVER("Frontend size W: %u H: %u\n",
> > + DRM_DEBUG_DRIVER("Frontend crtc size W: %u H: %u\n",
>
> The frontend is not the CRTC
Agreed.
> > state->crtc_w, state->crtc_h);
> > - regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_INSIZE_REG,
> > - SUN4I_FRONTEND_INSIZE(state->src_h >> 16,
> > - state->src_w >> 16));
> > - regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_INSIZE_REG,
> > - SUN4I_FRONTEND_INSIZE(state->src_h >> 16,
> > - state->src_w >> 16));
> >
> > + luma_width = chroma_width = state->src_w >> 16;
> > + luma_height = chroma_height = state->src_h >> 16;
> > +
> > + if (sun4i_format_is_yuv411(format)) {
> > + chroma_width = DIV_ROUND_UP(luma_width, 4);
> > + } else if (sun4i_format_is_yuv420(format)) {
> > + chroma_width = DIV_ROUND_UP(luma_width, 2);
> > + chroma_height = DIV_ROUND_UP(luma_height, 2);
> > + } else if (sun4i_format_is_yuv422(format)) {
> > + chroma_width = DIV_ROUND_UP(luma_width, 2);
> > + }
> > +
> > + regmap_write(frontend->regs, SUN4I_FRONTEND_CH0_INSIZE_REG,
> > + SUN4I_FRONTEND_INSIZE(luma_height,
> > luma_width));
> > regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH0_OUTSIZE_REG,
> > SUN4I_FRONTEND_OUTSIZE(state->crtc_h, state-
> > >crtc_w));
> > + regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH0_HORZFACT_REG,
> > + (luma_width << 16) / state->crtc_w);
> > + regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH0_VERTFACT_REG,
> > + (luma_height << 16) / state->crtc_h);
> > +
> > + /* These also have to be specified, even for interleaved
> > formats. */
> > + regmap_write(frontend->regs, SUN4I_FRONTEND_CH1_INSIZE_REG,
> > + SUN4I_FRONTEND_INSIZE(chroma_height,
> > chroma_width));
> > regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH1_OUTSIZE_REG,
> > SUN4I_FRONTEND_OUTSIZE(state->crtc_h, state-
> > >crtc_w));
> > -
> > - regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH0_HORZFACT_REG,
> > - state->src_w / state->crtc_w);
> > regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH1_HORZFACT_REG,
> > - state->src_w / state->crtc_w);
> > -
> > - regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH0_VERTFACT_REG,
> > - state->src_h / state->crtc_h);
> > + (chroma_width << 16) / state->crtc_w);
> > regmap_write(frontend->regs,
> > SUN4I_FRONTEND_CH1_VERTFACT_REG,
> > - state->src_h / state->crtc_h);
> > + (chroma_height << 16) / state->crtc_h);
> >
> > regmap_write_bits(frontend->regs,
> > SUN4I_FRONTEND_FRM_CTRL_REG,
> > SUN4I_FRONTEND_FRM_CTRL_REG_RDY,
> > @@ -382,10 +560,6 @@ static int sun4i_frontend_runtime_resume(struct
> > device *dev)
> > SUN4I_FRONTEND_EN_EN,
> > SUN4I_FRONTEND_EN_EN);
> >
> > - regmap_update_bits(frontend->regs,
> > SUN4I_FRONTEND_BYPASS_REG,
> > - SUN4I_FRONTEND_BYPASS_CSC_EN,
> > - SUN4I_FRONTEND_BYPASS_CSC_EN);
> > -
> > sun4i_frontend_scaler_init(frontend);
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.h
> > b/drivers/gpu/drm/sun4i/sun4i_frontend.h
> > index a9cb908ced16..6dd1d18752f4 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_frontend.h
> > +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.h
> > @@ -21,17 +21,56 @@
> > #define SUN4I_FRONTEND_BYPASS_REG 0x008
> > #define SUN4I_FRONTEND_BYPASS_CSC_EN BIT(1)
> >
> > +#define SUN4I_FRONTEND_AGTH_SEL_REG 0x00C
> > +#define SUN4I_FRONTEND_AGTH_SEL_ORIGINAL BIT(8)
> > +
> > #define SUN4I_FRONTEND_BUF_ADDR0_REG 0x020
> > +#define SUN4I_FRONTEND_BUF_ADDR1_REG 0x024
> > +#define SUN4I_FRONTEND_BUF_ADDR2_REG 0x028
> > +
> > +#define SUN4I_FRONTEND_TB_OFF0_REG 0x030
> > +#define SUN4I_FRONTEND_TB_OFF1_REG 0x034
> > +#define SUN4I_FRONTEND_TB_OFF2_REG 0x038
> > +
> > +#define SUN4I_FRONTEND_TB_OFF_X1(x1) ((x1)
> > << 16)
> > +#define SUN4I_FRONTEND_TB_OFF_Y0(y0) ((y0)
> > << 8)
> > +#define SUN4I_FRONTEND_TB_OFF_X0(x0) (x0)
> >
> > #define SUN4I_FRONTEND_LINESTRD0_REG 0x040
> > +#define SUN4I_FRONTEND_LINESTRD1_REG 0x044
> > +#define SUN4I_FRONTEND_LINESTRD2_REG 0x048
> >
> > #define SUN4I_FRONTEND_INPUT_FMT_REG 0x04c
> > -#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(mod) ((mod
> > ) << 8)
> > -#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(fmt) ((fmt
> > ) << 4)
> > -#define SUN4I_FRONTEND_INPUT_FMT_PS(ps) (ps)
> > +
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MASK
> > GENMASK(10, 8)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PLANAR (0
> > << 8)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_PACKED (1
> > << 8)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_SEMIPLANAR
> > (2 << 8)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_PLANAR
> > (4 << 8)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_MOD_MB32_SEMIPLANAR (6
> > << 8)
> > +
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_MASK
> > GENMASK(6, 4)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV444 (0
> > << 4)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV422 (1
> > << 4)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV420 (2
> > << 4)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_YUV411 (3
> > << 4)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_FMT_RGB
> > (5 << 4)
> > +
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_MASK
> > GENMASK(1, 0)
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UYVY
> > 0
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YUYV
> > 1
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VYUY
> > 2
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_YVYU
> > 3
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_UV
> > 0
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_VU
> > 1
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_XRGB
> > 0
> > +#define SUN4I_FRONTEND_INPUT_FMT_DATA_PS_BGRX
> > 1
> >
> > #define SUN4I_FRONTEND_OUTPUT_FMT_REG 0x05c
> > -#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(fmt) (fmt
> > )
> > +#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_BGRX8888 1
> > +#define SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT_XRGB8888 2
> > +
> > +#define SUN4I_FRONTEND_CSC_COEF_REG(c) (0x070 + (0x4
> > * (c)))
> >
> > #define SUN4I_FRONTEND_CH0_INSIZE_REG 0x100
> > #define SUN4I_FRONTEND_INSIZE(h, w) ((((h) -
> > 1) << 16) | (((w) - 1)))
> > @@ -96,5 +135,6 @@ void sun4i_frontend_update_coord(struct
> > sun4i_frontend *frontend,
> > int sun4i_frontend_update_formats(struct sun4i_frontend *frontend,
> > struct drm_plane *plane, uint32_t
> > out_fmt);
> > bool sun4i_frontend_format_is_supported(uint32_t fmt);
> > +bool sun4i_frontend_format_chroma_requires_swap(uint32_t fmt);
> >
> > #endif /* _SUN4I_FRONTEND_H_ */
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > index 15238211a61a..a39eed6a0e75 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > @@ -128,19 +128,37 @@ static const struct drm_plane_funcs
> > sun4i_backend_layer_funcs = {
> > .update_plane =
> > drm_atomic_helper_update_plane,
> > };
> >
> > -static const uint32_t sun4i_backend_layer_formats[] = {
> > - DRM_FORMAT_ARGB8888,
> > +static const uint32_t sun4i_layer_formats[] = {
> > + /* RGB */
> > DRM_FORMAT_ARGB4444,
> > + DRM_FORMAT_RGBA4444,
> > DRM_FORMAT_ARGB1555,
> > DRM_FORMAT_RGBA5551,
> > - DRM_FORMAT_RGBA4444,
> > - DRM_FORMAT_RGB888,
> > DRM_FORMAT_RGB565,
> > - DRM_FORMAT_UYVY,
> > - DRM_FORMAT_VYUY,
> > + DRM_FORMAT_RGB888,
> > DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_BGRX8888,
> > + DRM_FORMAT_ARGB8888,
> > + /* YUV444 */
> > + DRM_FORMAT_YUV444,
> > + DRM_FORMAT_YVU444,
> > + /* YUV422 */
> > DRM_FORMAT_YUYV,
> > DRM_FORMAT_YVYU,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_VYUY,
> > + DRM_FORMAT_NV16,
> > + DRM_FORMAT_NV61,
> > + DRM_FORMAT_YUV422,
> > + DRM_FORMAT_YVU422,
> > + /* YUV420 */
> > + DRM_FORMAT_NV12,
> > + DRM_FORMAT_NV21,
> > + DRM_FORMAT_YUV420,
> > + DRM_FORMAT_YVU420,
> > + /* YUV411 */
> > + DRM_FORMAT_YUV411,
> > + DRM_FORMAT_YVU411,
> > };
> >
> > static struct sun4i_layer *sun4i_layer_init_one(struct drm_device
> > *drm,
> > @@ -157,8 +175,8 @@ static struct sun4i_layer
> > *sun4i_layer_init_one(struct drm_device *drm,
> > /* possible crtcs are set later */
> > ret = drm_universal_plane_init(drm, &layer->plane, 0,
> > &sun4i_backend_layer_funcs,
> > - sun4i_backend_layer_formats,
> > - ARRAY_SIZE(sun4i_backend_lay
> > er_formats),
> > + sun4i_layer_formats,
> > + ARRAY_SIZE(sun4i_layer_forma
> > ts),
> > NULL, type, NULL);
>
> I stopped reviewing this, because it should be split in at least the
> following commits, possibly more:
> - one to add the custom GEM allocator
> - one to move the yuv number check in atomic_check
> - one to enable the input sequence computation
> - one to enable the input mode computation
> - one to move the CSC bypass setting from the probe to the
> update_fomats function
> - one to introduce the interleaved YUV formats
> - one to introduce the multi-planar YUV formats
> - one to rename the sun4i_backend_layer_formats array
I'll craft v2 in this direction then.
Cheers,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
On Fri, 2018-03-23 at 11:48 +0100, Maxime Ripard wrote:
> Hi,
>
> On Wed, Mar 21, 2018 at 04:29:03PM +0100, Paul Kocialkowski wrote:
> > This introduces a dedicated ioctl for allocating tiled buffers in
> > the
> > Allwinner MB32 format, that comes with a handful of specific
> > constraints. In particular, the stride of the buffers is expected to
> > be
> > aligned to 32 bytes.
>
> You should have more details here. What are those constraints, what is
> the expected user? Can you use it only for the scanout, like the dumb
> buffers, or also for the other devices in the system?
Agreed.
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_drv.c | 96
> > +++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/sun4i/sun4i_drv.h | 2 +
> > include/uapi/drm/sun4i_drm.h | 42 +++++++++++++++++
> > 3 files changed, 140 insertions(+)
> > create mode 100644 include/uapi/drm/sun4i_drm.h
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > index d374bb61c565..e9cb03d34b44 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > @@ -21,11 +21,18 @@
> > #include <drm/drm_gem_cma_helper.h>
> > #include <drm/drm_fb_helper.h>
> > #include <drm/drm_of.h>
> > +#include <drm/sun4i_drm.h>
> >
> > #include "sun4i_drv.h"
> > #include "sun4i_frontend.h"
> > #include "sun4i_framebuffer.h"
> > #include "sun4i_tcon.h"
> > +#include "sun4i_format.h"
> > +
> > +static const struct drm_ioctl_desc sun4i_drv_ioctls[] = {
> > + DRM_IOCTL_DEF_DRV(SUN4I_GEM_CREATE_TILED,
> > drm_sun4i_gem_create_tiled,
> > + DRM_AUTH | DRM_RENDER_ALLOW),
>
> Why do you need DRM_RENDER_ALLOW? as far as I know, this is only
> useful for render-nodes.
It's probably undeeded indeed.
> > +};
> >
> > DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
> >
> > @@ -34,6 +41,8 @@ static struct drm_driver sun4i_drv_driver = {
> >
> > /* Generic Operations */
> > .lastclose = drm_fb_helper_lastclose,
> > + .ioctls = sun4i_drv_ioctls,
> > + .num_ioctls = ARRAY_SIZE(sun4i_drv_ioctls),
> > .fops = &sun4i_drv_fops,
> > .name = "sun4i-drm",
> > .desc = "Allwinner sun4i Display
> > Engine",
> > @@ -69,6 +78,93 @@ int drm_sun4i_gem_dumb_create(struct drm_file
> > *file_priv,
> > return drm_gem_cma_dumb_create_internal(file_priv, drm,
> > args);
> > }
> >
> > +int drm_sun4i_gem_create_tiled(struct drm_device *drm, void *data,
> > + struct drm_file *file_priv)
> > +{
> > + struct drm_sun4i_gem_create_tiled *args = data;
> > + struct drm_gem_cma_object *cma_obj;
> > + struct drm_gem_object *gem_obj;
> > + uint32_t luma_stride, chroma_stride;
> > + uint32_t luma_height, chroma_height;
> > + int ret;
> > +
> > + if (!sun4i_format_supports_tiling(args->format))
> > + return -EINVAL;
> > +
> > + memset(args->pitches, 0, sizeof(args->pitches));
> > + memset(args->offsets, 0, sizeof(args->offsets));
> > +
> > + /* Stride and height are aligned to 32 bytes for MB32 tiled
> > format. */
> > + luma_stride = ALIGN(args->width, 32);
> > + luma_height = ALIGN(args->height, 32);
> > +
> > + if (sun4i_format_is_semiplanar(args->format)) {
> > + chroma_stride = luma_stride;
> > +
> > + if (sun4i_format_is_yuv420(args->format))
> > + chroma_height = ALIGN(DIV_ROUND_UP(args-
> > >height, 2), 32);
> > + else if (sun4i_format_is_yuv422(args->format))
> > + chroma_height = luma_height;
> > + else
> > + return -EINVAL;
> > +
> > + args->pitches[0] = luma_stride;
> > + args->pitches[1] = chroma_stride;
> > +
> > + args->offsets[0] = 0;
> > + args->offsets[1] = luma_stride * luma_height;
> > +
> > + args->size = luma_stride * luma_height +
> > + chroma_stride * chroma_height;
> > + } else if (sun4i_format_is_planar(args->format)) {
> > + if (sun4i_format_is_yuv411(args->format)) {
> > + chroma_stride = ALIGN(DIV_ROUND_UP(args-
> > >width, 4), 32);
> > + chroma_height = luma_height;
> > + } if (sun4i_format_is_yuv420(args->format)) {
> > + chroma_stride = ALIGN(DIV_ROUND_UP(args-
> > >width, 2), 32);
> > + chroma_height = ALIGN(DIV_ROUND_UP(args-
> > >height, 2), 32);
> > + } else if (sun4i_format_is_yuv422(args->format)) {
> > + chroma_stride = ALIGN(DIV_ROUND_UP(args-
> > >width, 2), 32);
> > + chroma_height = luma_height;
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + args->pitches[0] = luma_stride;
> > + args->pitches[1] = chroma_stride;
> > + args->pitches[2] = chroma_stride;
> > +
> > + args->offsets[0] = 0;
> > + args->offsets[1] = luma_stride * luma_height;
> > + args->offsets[2] = luma_stride * luma_height +
> > + chroma_stride * chroma_height;
> > +
> > + args->size = luma_stride * luma_height +
> > + chroma_stride * chroma_height * 2;
> > + } else {
> > + /* No support for packed formats in tiled mode. */
> > + return -EINVAL;
> > + }
> > +
> > + cma_obj = drm_gem_cma_create(drm, args->size);
> > + if (IS_ERR(cma_obj))
> > + return PTR_ERR(cma_obj);
> > +
> > + gem_obj = &cma_obj->base;
> > +
> > + /*
> > + * allocate a id of idr table where the obj is registered
> > + * and handle has the id what user can see.
> > + */
> > + ret = drm_gem_handle_create(file_priv, gem_obj, &args-
> > >handle);
> > + /* drop reference from allocate - handle holds it now. */
> > + drm_gem_object_put_unlocked(gem_obj);
> > + if (ret)
> > + return ret;
> > +
> > + return PTR_ERR_OR_ZERO(cma_obj);
> > +}
> > +
> > static void sun4i_remove_framebuffers(void)
> > {
> > struct apertures_struct *ap;
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h
> > b/drivers/gpu/drm/sun4i/sun4i_drv.h
> > index 47969711a889..308ff4bfcdd5 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.h
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h
> > @@ -26,5 +26,7 @@ struct sun4i_drv {
> > int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
> > struct drm_device *drm,
> > struct drm_mode_create_dumb *args);
> > +int drm_sun4i_gem_create_tiled(struct drm_device *dev, void *data,
> > + struct drm_file *file_priv);
>
> Do you need it to be non-static, and part of the header as well?
Here as well, I just find that it looks more readable that way, below
the drm driver structure definition instead of above it.
> I'll let the DRM-misc maintainers comment on the validity of the new
> ioctl.
Cheers,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
On Tue, 2018-03-27 at 10:17 +0200, Maxime Ripard wrote:
> On Tue, Mar 27, 2018 at 10:00:43AM +0200, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Fri, 2018-03-23 at 10:55 +0100, Maxime Ripard wrote:
> > > On Wed, Mar 21, 2018 at 04:28:56PM +0100, Paul Kocialkowski wrote:
> > > > The YUV channel was only disabled in
> > > > sun4i_backend_update_layer_formats,
> > > > which is not called when the frontend is selected.
> > > >
> > > > Thus, creating a layer with a YUV format handled by the backend
> > > > and
> > > > then
> > > > switching to a format that requires the frontend would keep the
> > > > YUV
> > > > channel enabled for the layer.
> > > >
> > > > This explicitly disables the YUV channel for the layer when
> > > > using
> > > > the
> > > > frontend as well. It also sets the relevant interlace bit, which
> > > > was
> > > > missing in the frontend path as well.
> > >
> > > This should be part of a separate patch. Usually, if you write "it
> > > also does..." at the end of your commit log, it's a pretty good
> > > indication that it should be another patch :)
> >
> > I must say, I figured that this part was missing in the frontend
> > path by
> > chance and couldn't really test the feature, so I'm also tempted to
> > drop
> > it altogether. What do you think?
>
> If you haven't been able to test it, then yeah, don't submit it.
Alright, noted.
> > Also, is interlacing actually used on any of the video outputs we
> > support? Perhaps RGB?
>
> Composite would be a better guess :)
Oh and I was wondering what CVBS was about. Now I know!
It seems that we don't support it for now apparently, anyway.
Thanks,
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
On Tue, Mar 27, 2018 at 4:44 PM, Paul Kocialkowski
<[email protected]> wrote:
> Hi,
>
> On Tue, 2018-03-27 at 10:17 +0200, Maxime Ripard wrote:
>> On Tue, Mar 27, 2018 at 10:00:43AM +0200, Paul Kocialkowski wrote:
>> > Hi,
>> >
>> > On Fri, 2018-03-23 at 10:55 +0100, Maxime Ripard wrote:
>> > > On Wed, Mar 21, 2018 at 04:28:56PM +0100, Paul Kocialkowski wrote:
>> > > > The YUV channel was only disabled in
>> > > > sun4i_backend_update_layer_formats,
>> > > > which is not called when the frontend is selected.
>> > > >
>> > > > Thus, creating a layer with a YUV format handled by the backend
>> > > > and
>> > > > then
>> > > > switching to a format that requires the frontend would keep the
>> > > > YUV
>> > > > channel enabled for the layer.
>> > > >
>> > > > This explicitly disables the YUV channel for the layer when
>> > > > using
>> > > > the
>> > > > frontend as well. It also sets the relevant interlace bit, which
>> > > > was
>> > > > missing in the frontend path as well.
>> > >
>> > > This should be part of a separate patch. Usually, if you write "it
>> > > also does..." at the end of your commit log, it's a pretty good
>> > > indication that it should be another patch :)
>> >
>> > I must say, I figured that this part was missing in the frontend
>> > path by
>> > chance and couldn't really test the feature, so I'm also tempted to
>> > drop
>> > it altogether. What do you think?
>>
>> If you haven't been able to test it, then yeah, don't submit it.
>
> Alright, noted.
>
>> > Also, is interlacing actually used on any of the video outputs we
>> > support? Perhaps RGB?
>>
>> Composite would be a better guess :)
>
> Oh and I was wondering what CVBS was about. Now I know!
> It seems that we don't support it for now apparently, anyway.
You could also try adding interlaced modes to HDMI.
ChenYu
On Tue, Mar 27, 2018 at 10:44:19AM +0200, Paul Kocialkowski wrote:
> > > Also, is interlacing actually used on any of the video outputs we
> > > support? Perhaps RGB?
> >
> > Composite would be a better guess :)
>
> Oh and I was wondering what CVBS was about. Now I know!
> It seems that we don't support it for now apparently, anyway.
What do you mean? We do support it.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, 2018-03-27 at 11:18 +0200, Maxime Ripard wrote:
> On Tue, Mar 27, 2018 at 10:44:19AM +0200, Paul Kocialkowski wrote:
> > > > Also, is interlacing actually used on any of the video outputs
> > > > we
> > > > support? Perhaps RGB?
> > >
> > > Composite would be a better guess :)
> >
> > Oh and I was wondering what CVBS was about. Now I know!
> > It seems that we don't support it for now apparently, anyway.
>
> What do you mean? We do support it.
Sorry, I think I was remembering the status for A10/A20 only. Glad to
see that it works on A13!
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
On Tue, Mar 27, 2018 at 10:27:44AM +0200, Paul Kocialkowski wrote:
> > > +bool sun4i_format_is_rgb(uint32_t format);
> > > +bool sun4i_format_is_yuv(uint32_t format);
> > > +bool sun4i_format_is_yuv411(uint32_t format);
> > > +bool sun4i_format_is_yuv420(uint32_t format);
> > > +bool sun4i_format_is_yuv422(uint32_t format);
> > > +bool sun4i_format_is_yuv444(uint32_t format);
> > > +bool sun4i_format_is_packed(uint32_t format);
> > > +bool sun4i_format_is_semiplanar(uint32_t format);
> > > +bool sun4i_format_is_planar(uint32_t format);
> > > +bool sun4i_format_supports_tiling(uint32_t format);
> >
> > If we're going to add so many of them, then we should really consider
> > to move them to drm_fourcc.c instead. Every one has some variation of
> > some of these functions, we don't really need to duplicate it all the
> > time.
>
> Should I try to get that through in this patchset and have sun4i-drm
> be their first user? Also, does introducing such a change require
> identifying duplicates of these functions in each DRM driver's
> codebase?
I guess converting at least one of them would prove how usable it
would be to other drivers, but I won't ask you to convert all of them
as part of this serie :)
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Mar 27, 2018 at 10:41:38AM +0200, Paul Kocialkowski wrote:
> > > +int drm_sun4i_gem_create_tiled(struct drm_device *dev, void *data,
> > > + struct drm_file *file_priv);
> >
> > Do you need it to be non-static, and part of the header as well?
>
> Here as well, I just find that it looks more readable that way, below
> the drm driver structure definition instead of above it.
But it also creates a global symbol for no particular reason, while
we're doing the function-first-structure-later pattern pretty much
everywhere else in the kernel.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Mar 27, 2018 at 10:08:48AM +0200, Paul Kocialkowski wrote:
> On Fri, 2018-03-23 at 11:03 +0100, Maxime Ripard wrote:
> > On Wed, Mar 21, 2018 at 04:28:58PM +0100, Paul Kocialkowski wrote:
> > > In order to check whether the backend supports a specific format, an
> > > explicit list and a related helper are introduced.
> > >
> > > They are then used to determine whether the frontend should be used
> > > for
> > > a layer, when the format is not supported by the backend.
> > >
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > ---
> > > drivers/gpu/drm/sun4i/sun4i_backend.c | 48
> > > ++++++++++++++++++++++++++++++++++-
> > > drivers/gpu/drm/sun4i/sun4i_backend.h | 1 +
> > > 2 files changed, 48 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > index 274a1db6fa8e..7703ba989743 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > @@ -172,6 +172,39 @@ static int
> > > sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
> > > return 0;
> > > }
> > >
> > > +static const uint32_t sun4i_backend_formats[] = {
> > > + /* RGB */
> > > + DRM_FORMAT_ARGB4444,
> > > + DRM_FORMAT_RGBA4444,
> > > + DRM_FORMAT_ARGB1555,
> > > + DRM_FORMAT_RGBA5551,
> > > + DRM_FORMAT_RGB565,
> > > + DRM_FORMAT_RGB888,
> > > + DRM_FORMAT_XRGB8888,
> > > + DRM_FORMAT_BGRX8888,
> > > + DRM_FORMAT_ARGB8888,
> > > + /* YUV422 */
> > > + DRM_FORMAT_YUYV,
> > > + DRM_FORMAT_YVYU,
> > > + DRM_FORMAT_UYVY,
> > > + DRM_FORMAT_VYUY,
> >
> > Ordering them by alphabetical order would be better.
>
> Frankly I find it a lot harder to read when the formats are not grouped
> by "family". This is the drm_fourcc enumeration order, which has some
> kind of logic behind it. What is the advantage of alphabetical ordering
> here?
It's self-sufficient and self-explanatory. The sorting here, while it
would make sense lack both: you need to refer to the drm_fourcc.h file
to get the sorting right (which makes it harder to edit and review,
and thus more error prone), and it assumes that the editor knows about
that sorting in the first place.
And it's an assumption we can't really make, since some people will
edit that structure in the first place without any background at all
with DRM, or even graphics in general.
While the assumption you have to make for the alphabetical order is
that one knows the latin alphabet, which is a pretty obvious one when
you're doing C programming.
> > > + */
> > > + if (!sun4i_backend_format_is_supported(fb->format->format))
> > > + return true;
> >
> > Even though there's a comment, this is not really natural. We are
> > checking whether the frontend supports the current plane_state, so it
> > just makes more sense to check whether the frontend supports the
> > format, rather than if the backend doesn't support them.
>
> The rationale behind this logic is that we should try to use the backend
> first and only use the frontend as a last resort. Some formats are
> supported by both and checking that the backend supports a format first
> ensures that we don't bring up the frontend without need.
You can achieve pretty much the same thing by doing:
if (!sun4i_frontend_format_is_supported())
return false;
if (!using_scaling)
return false;
if (using_2x_or_4x_scaling)
return false;
return true;
This is really about whitelisting vs blacklisting, so I'm not sure
what that would really change in the case you described above.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Mar 27, 2018 at 10:24:17AM +0200, Paul Kocialkowski wrote:
> Hi,
>
> On Fri, 2018-03-23 at 11:06 +0100, Maxime Ripard wrote:
> > On Wed, Mar 21, 2018 at 04:28:59PM +0100, Paul Kocialkowski wrote:
> > > In order to check whether the frontend supports a specific format,
> > > an
> > > explicit list and a related helper are introduced.
> > >
> > > They are then used to determine whether the frontend can actually
> > > support
> > > the requested format when it was selected to be used.
> > >
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > ---
> > > drivers/gpu/drm/sun4i/sun4i_backend.c | 5 ++++
> > > drivers/gpu/drm/sun4i/sun4i_frontend.c | 44
> > > ++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/sun4i/sun4i_frontend.h | 1 +
> > > 3 files changed, 50 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > index 7703ba989743..1fad0714c70e 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > @@ -532,6 +532,11 @@ static int sun4i_backend_atomic_check(struct
> > > sunxi_engine *engine,
> > > struct drm_format_name_buf format_name;
> > >
> > > if (sun4i_backend_plane_uses_frontend(plane_state))
> > > {
> > > + if (!sun4i_frontend_format_is_supported(fb-
> > > >format->format)) {
> > > + DRM_DEBUG_DRIVER("Frontend plane
> > > check failed\n");
> > > + return -EINVAL;
> > > + }
> > > +
> >
> > So you're checking if the frontend doesn't support it and if the
> > backend doesn't support it. Who supports it then? :)
>
> That's the case case where the format is not supported by either of the
> hardware blocks. For instance, requesting ARGB with tiling will fail,
> although ARGB without tiling will work. It seems that modetest assumes
> that modifiers only apply to YUV (in which case there are no unsupported
> cases), but I think userspace might still request RGB+tiling
> combinations. I don't currently see a way to report to which format the
> tiling applies (thus we have to expect that it can come with any of the
> supported formats).
Then I guess this is a check that could be moved out of that
condition, or even as part of the generic side of atomic_check. That
looks not really specific to the driver itself.
> > Like I was saying, this should be moved to the previous patch, within
> > sun4i_backend_plane_uses_frontend.
>
> If we get both tests (no backend support and frontend support) into one
> function, we cannot detect that the format is actually unsupported and
> return an error in the atomic check.
This is already covered by drm_atomic_plane_check:
https://elixir.bootlin.com/linux/v4.16-rc7/source/drivers/gpu/drm/drm_atomic.c#L884
So we'll only end up being called here if we have a format we support.
> > > +static const uint32_t sun4i_frontend_formats[] = {
> > > + /* RGB */
> > > + DRM_FORMAT_XRGB8888,
> > > + DRM_FORMAT_BGRX8888,
> > > + /* YUV444 */
> > > + DRM_FORMAT_YUV444,
> > > + DRM_FORMAT_YVU444,
> > > + /* YUV422 */
> > > + DRM_FORMAT_YUYV,
> > > + DRM_FORMAT_YVYU,
> > > + DRM_FORMAT_UYVY,
> > > + DRM_FORMAT_VYUY,
> > > + DRM_FORMAT_NV16,
> > > + DRM_FORMAT_NV61,
> > > + DRM_FORMAT_YUV422,
> > > + DRM_FORMAT_YVU422,
> > > + /* YUV420 */
> > > + DRM_FORMAT_NV12,
> > > + DRM_FORMAT_NV21,
> > > + DRM_FORMAT_YUV420,
> > > + DRM_FORMAT_YVU420,
> > > + /* YUV411 */
> > > + DRM_FORMAT_YUV411,
> > > + DRM_FORMAT_YVU411,
> > > +};
> >
> > I think this list should reflect what the driver currently supports,
> > not what the hardware supports.
>
> This list is not indended to reflect what the driver supports, but only
> what the backend can support (to correctly select whether a
> format/modifier couple should go to the frontend or backend)!
I know, and it's not my point. The whole point is that you don't need
that full list in the first place to achieve what you want to achieve
with this patch.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
Le jeudi 29 mars 2018 à 09:56 +0200, Maxime Ripard a écrit :
> On Tue, Mar 27, 2018 at 10:08:48AM +0200, Paul Kocialkowski wrote:
> > On Fri, 2018-03-23 at 11:03 +0100, Maxime Ripard wrote:
> > > On Wed, Mar 21, 2018 at 04:28:58PM +0100, Paul Kocialkowski wrote:
> > > > In order to check whether the backend supports a specific format, an
> > > > explicit list and a related helper are introduced.
> > > >
> > > > They are then used to determine whether the frontend should be used
> > > > for
> > > > a layer, when the format is not supported by the backend.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/sun4i/sun4i_backend.c | 48
> > > > ++++++++++++++++++++++++++++++++++-
> > > > drivers/gpu/drm/sun4i/sun4i_backend.h | 1 +
> > > > 2 files changed, 48 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > index 274a1db6fa8e..7703ba989743 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > @@ -172,6 +172,39 @@ static int
> > > > sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
> > > > return 0;
> > > > }
> > > >
> > > > +static const uint32_t sun4i_backend_formats[] = {
> > > > + /* RGB */
> > > > + DRM_FORMAT_ARGB4444,
> > > > + DRM_FORMAT_RGBA4444,
> > > > + DRM_FORMAT_ARGB1555,
> > > > + DRM_FORMAT_RGBA5551,
> > > > + DRM_FORMAT_RGB565,
> > > > + DRM_FORMAT_RGB888,
> > > > + DRM_FORMAT_XRGB8888,
> > > > + DRM_FORMAT_BGRX8888,
> > > > + DRM_FORMAT_ARGB8888,
> > > > + /* YUV422 */
> > > > + DRM_FORMAT_YUYV,
> > > > + DRM_FORMAT_YVYU,
> > > > + DRM_FORMAT_UYVY,
> > > > + DRM_FORMAT_VYUY,
> > >
> > > Ordering them by alphabetical order would be better.
> >
> > Frankly I find it a lot harder to read when the formats are not grouped
> > by "family". This is the drm_fourcc enumeration order, which has some
> > kind of logic behind it. What is the advantage of alphabetical ordering
> > here?
>
> It's self-sufficient and self-explanatory. The sorting here, while it
> would make sense lack both: you need to refer to the drm_fourcc.h file
> to get the sorting right (which makes it harder to edit and review,
> and thus more error prone), and it assumes that the editor knows about
> that sorting in the first place.
>
> And it's an assumption we can't really make, since some people will
> edit that structure in the first place without any background at all
> with DRM, or even graphics in general.
>
> While the assumption you have to make for the alphabetical order is
> that one knows the latin alphabet, which is a pretty obvious one when
> you're doing C programming.
>
> > > > + */
> > > > + if (!sun4i_backend_format_is_supported(fb->format->format))
> > > > + return true;
> > >
> > > Even though there's a comment, this is not really natural. We are
> > > checking whether the frontend supports the current plane_state, so it
> > > just makes more sense to check whether the frontend supports the
> > > format, rather than if the backend doesn't support them.
> >
> > The rationale behind this logic is that we should try to use the backend
> > first and only use the frontend as a last resort. Some formats are
> > supported by both and checking that the backend supports a format first
> > ensures that we don't bring up the frontend without need.
>
> You can achieve pretty much the same thing by doing:
>
> if (!sun4i_frontend_format_is_supported())
> return false;
>
> if (!using_scaling)
> return false;
>
> if (using_2x_or_4x_scaling)
> return false;
>
> return true;
>
> This is really about whitelisting vs blacklisting, so I'm not sure
> what that would really change in the case you described above.
These sequential tests for blacklisting don't fit the bill here. For
instance, it would always return false when not using scaling for a
format supported only by the frontend, while we'd need to use the
frontend for it.
We still need to know whether the format is supported or not for both
the frontend or the backend, because one is not sufficient to describe
the other and both are involved in the decision to use the frontend.
That is, the set of formats supported by the frontend is not the
complementary of the sets of formats supported by the backend (some
formats are supported by both), so we can't exchange one with the
negation of the other in the initial statement.
I agree with the inital comment though, that it seems more natural to
check that the frontend supports a format first.
I think the following combination would be the most comprehensive:
if (!sun4i_frontend_format_is_supported())
return false;
if (!sun4i_backend_format_is_supported())
return true;
if (using_2x_or_4x_scaling)
return false;
if (using_scaling)
return true;
return false;
What do you think?
Cheers,
Paul
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Hi,
Le vendredi 23 mars 2018 à 11:06 +0100, Maxime Ripard a écrit :
> On Wed, Mar 21, 2018 at 04:28:59PM +0100, Paul Kocialkowski wrote:
> > In order to check whether the frontend supports a specific format, an
> > explicit list and a related helper are introduced.
> >
> > They are then used to determine whether the frontend can actually support
> > the requested format when it was selected to be used.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 5 ++++
> > drivers/gpu/drm/sun4i/sun4i_frontend.c | 44 ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/sun4i/sun4i_frontend.h | 1 +
> > 3 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index 7703ba989743..1fad0714c70e 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -532,6 +532,11 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> > struct drm_format_name_buf format_name;
> >
> > if (sun4i_backend_plane_uses_frontend(plane_state)) {
> > + if (!sun4i_frontend_format_is_supported(fb->format->format)) {
> > + DRM_DEBUG_DRIVER("Frontend plane check failed\n");
> > + return -EINVAL;
> > + }
> > +
>
> So you're checking if the frontend doesn't support it and if the
> backend doesn't support it. Who supports it then? :)
>
> Like I was saying, this should be moved to the previous patch, within
> sun4i_backend_plane_uses_frontend.
You're right, this isn't the right place to put it.
sun4i_backend_plane_uses_frontend should definitely guarantee that the
format is supported.
> > +static const uint32_t sun4i_frontend_formats[] = {
> > + /* RGB */
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_BGRX8888,
> > + /* YUV444 */
> > + DRM_FORMAT_YUV444,
> > + DRM_FORMAT_YVU444,
> > + /* YUV422 */
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_VYUY,
> > + DRM_FORMAT_NV16,
> > + DRM_FORMAT_NV61,
> > + DRM_FORMAT_YUV422,
> > + DRM_FORMAT_YVU422,
> > + /* YUV420 */
> > + DRM_FORMAT_NV12,
> > + DRM_FORMAT_NV21,
> > + DRM_FORMAT_YUV420,
> > + DRM_FORMAT_YVU420,
> > + /* YUV411 */
> > + DRM_FORMAT_YUV411,
> > + DRM_FORMAT_YVU411,
> > +};
>
> I think this list should reflect what the driver currently supports,
> not what the hardware supports.
Agreed, this should be updated with the follow-up patch that adds YUV
support.
Cheers,
Paul
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
On Tue, Oct 16, 2018 at 03:55:40PM +0200, Paul Kocialkowski wrote:
> Hi,
>
> Le jeudi 29 mars 2018 ? 09:56 +0200, Maxime Ripard a ?crit :
> > On Tue, Mar 27, 2018 at 10:08:48AM +0200, Paul Kocialkowski wrote:
> > > On Fri, 2018-03-23 at 11:03 +0100, Maxime Ripard wrote:
> > > > On Wed, Mar 21, 2018 at 04:28:58PM +0100, Paul Kocialkowski wrote:
> > > > > In order to check whether the backend supports a specific format, an
> > > > > explicit list and a related helper are introduced.
> > > > >
> > > > > They are then used to determine whether the frontend should be used
> > > > > for
> > > > > a layer, when the format is not supported by the backend.
> > > > >
> > > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/sun4i/sun4i_backend.c | 48
> > > > > ++++++++++++++++++++++++++++++++++-
> > > > > drivers/gpu/drm/sun4i/sun4i_backend.h | 1 +
> > > > > 2 files changed, 48 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > > b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > > index 274a1db6fa8e..7703ba989743 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > > > @@ -172,6 +172,39 @@ static int
> > > > > sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static const uint32_t sun4i_backend_formats[] = {
> > > > > + /* RGB */
> > > > > + DRM_FORMAT_ARGB4444,
> > > > > + DRM_FORMAT_RGBA4444,
> > > > > + DRM_FORMAT_ARGB1555,
> > > > > + DRM_FORMAT_RGBA5551,
> > > > > + DRM_FORMAT_RGB565,
> > > > > + DRM_FORMAT_RGB888,
> > > > > + DRM_FORMAT_XRGB8888,
> > > > > + DRM_FORMAT_BGRX8888,
> > > > > + DRM_FORMAT_ARGB8888,
> > > > > + /* YUV422 */
> > > > > + DRM_FORMAT_YUYV,
> > > > > + DRM_FORMAT_YVYU,
> > > > > + DRM_FORMAT_UYVY,
> > > > > + DRM_FORMAT_VYUY,
> > > >
> > > > Ordering them by alphabetical order would be better.
> > >
> > > Frankly I find it a lot harder to read when the formats are not grouped
> > > by "family". This is the drm_fourcc enumeration order, which has some
> > > kind of logic behind it. What is the advantage of alphabetical ordering
> > > here?
> >
> > It's self-sufficient and self-explanatory. The sorting here, while it
> > would make sense lack both: you need to refer to the drm_fourcc.h file
> > to get the sorting right (which makes it harder to edit and review,
> > and thus more error prone), and it assumes that the editor knows about
> > that sorting in the first place.
> >
> > And it's an assumption we can't really make, since some people will
> > edit that structure in the first place without any background at all
> > with DRM, or even graphics in general.
> >
> > While the assumption you have to make for the alphabetical order is
> > that one knows the latin alphabet, which is a pretty obvious one when
> > you're doing C programming.
> >
> > > > > + */
> > > > > + if (!sun4i_backend_format_is_supported(fb->format->format))
> > > > > + return true;
> > > >
> > > > Even though there's a comment, this is not really natural. We are
> > > > checking whether the frontend supports the current plane_state, so it
> > > > just makes more sense to check whether the frontend supports the
> > > > format, rather than if the backend doesn't support them.
> > >
> > > The rationale behind this logic is that we should try to use the backend
> > > first and only use the frontend as a last resort. Some formats are
> > > supported by both and checking that the backend supports a format first
> > > ensures that we don't bring up the frontend without need.
> >
> > You can achieve pretty much the same thing by doing:
> >
> > if (!sun4i_frontend_format_is_supported())
> > return false;
> >
> > if (!using_scaling)
> > return false;
> >
> > if (using_2x_or_4x_scaling)
> > return false;
> >
> > return true;
> >
> > This is really about whitelisting vs blacklisting, so I'm not sure
> > what that would really change in the case you described above.
>
> These sequential tests for blacklisting don't fit the bill here. For
> instance, it would always return false when not using scaling for a
> format supported only by the frontend, while we'd need to use the
> frontend for it.
>
> We still need to know whether the format is supported or not for both
> the frontend or the backend, because one is not sufficient to describe
> the other and both are involved in the decision to use the frontend.
>
> That is, the set of formats supported by the frontend is not the
> complementary of the sets of formats supported by the backend (some
> formats are supported by both), so we can't exchange one with the
> negation of the other in the initial statement.
>
> I agree with the inital comment though, that it seems more natural to
> check that the frontend supports a format first.
>
> I think the following combination would be the most comprehensive:
>
> if (!sun4i_frontend_format_is_supported())
> return false;
>
> if (!sun4i_backend_format_is_supported())
> return true;
>
> if (using_2x_or_4x_scaling)
> return false;
>
> if (using_scaling)
> return true;
>
> return false;
>
> What do you think?
That works for me
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com