* [PATCH v3 1/3] drm: Plumb modifiers through plane init
@ 2017-05-16 21:31 Ben Widawsky
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Ben Widawsky @ 2017-05-16 21:31 UTC (permalink / raw)
To: Intel GFX, DRI Development; +Cc: Ben Widawsky, Liviu Dudau, Daniel Stone
This is the plumbing for supporting fb modifiers on planes. Modifiers
have already been introduced to some extent, but this series will extend
this to allow querying modifiers per plane. Based on this, the client to
enable optimal modifications for framebuffers.
This patch simply allows the DRM drivers to initialize their list of
supported modifiers upon initializing the plane.
v2: A minor addition from Daniel
v3: Updated commit message
s/INVALID/DRM_FORMAT_MOD_INVALID (Liviu)
Remove some excess newlines (Liviu)
Update comment for > 64 modifiers (Liviu)
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Reviewed-by: Daniel Stone <daniels@collabora.com> (v2)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/arc/arcpgu_crtc.c | 1 +
drivers/gpu/drm/arm/hdlcd_crtc.c | 1 +
drivers/gpu/drm/arm/malidp_planes.c | 2 +-
drivers/gpu/drm/armada/armada_crtc.c | 1 +
drivers/gpu/drm/armada/armada_overlay.c | 1 +
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 4 ++-
drivers/gpu/drm/drm_modeset_helper.c | 1 +
drivers/gpu/drm/drm_plane.c | 35 ++++++++++++++++++++++++-
drivers/gpu/drm/drm_simple_kms_helper.c | 3 +++
drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 +-
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 2 +-
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 1 +
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 5 +++-
drivers/gpu/drm/i915/intel_sprite.c | 4 +--
drivers/gpu/drm/imx/ipuv3-plane.c | 4 +--
drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
drivers/gpu/drm/meson/meson_plane.c | 1 +
drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 +--
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
drivers/gpu/drm/nouveau/nv50_display.c | 5 ++--
drivers/gpu/drm/omapdrm/omap_plane.c | 3 ++-
drivers/gpu/drm/qxl/qxl_display.c | 2 +-
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +--
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +--
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 +--
drivers/gpu/drm/sti/sti_cursor.c | 2 +-
drivers/gpu/drm/sti/sti_gdp.c | 2 +-
drivers/gpu/drm/sti/sti_hqvdp.c | 2 +-
drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
drivers/gpu/drm/tegra/dc.c | 12 ++++-----
drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4 +--
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 +--
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 +--
drivers/gpu/drm/zte/zx_plane.c | 2 +-
include/drm/drm_plane.h | 20 +++++++++++++-
include/drm/drm_simple_kms_helper.h | 1 +
include/uapi/drm/drm_fourcc.h | 11 ++++++++
41 files changed, 126 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a95916f1f..cd8a24c7c67d 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -218,6 +218,7 @@ static struct drm_plane *arc_pgu_plane_init(struct drm_device *drm)
ret = drm_universal_plane_init(drm, plane, 0xff, &arc_pgu_plane_funcs,
formats, ARRAY_SIZE(formats),
+ NULL,
DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 798a3cc480a2..0caa03ae8708 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -303,6 +303,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
formats, ARRAY_SIZE(formats),
+ NULL,
DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
devm_kfree(drm->dev, plane);
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 814fda23cead..b156610c68a5 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -400,7 +400,7 @@ int malidp_de_planes_init(struct drm_device *drm)
DRM_PLANE_TYPE_OVERLAY;
ret = drm_universal_plane_init(drm, &plane->base, crtcs,
&malidp_de_plane_funcs, formats,
- n, plane_type, NULL);
+ n, NULL, plane_type, NULL);
if (ret < 0)
goto cleanup;
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 4fe19fde84f9..ea48ef88f0e4 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1269,6 +1269,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
&armada_primary_plane_funcs,
armada_primary_formats,
ARRAY_SIZE(armada_primary_formats),
+ NULL,
DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
kfree(primary);
diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c
index 424e465ff407..0054144287ae 100644
--- a/drivers/gpu/drm/armada/armada_overlay.c
+++ b/drivers/gpu/drm/armada/armada_overlay.c
@@ -460,6 +460,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
&armada_ovl_plane_funcs,
armada_ovl_formats,
ARRAY_SIZE(armada_ovl_formats),
+ NULL,
DRM_PLANE_TYPE_OVERLAY, NULL);
if (ret) {
kfree(dplane);
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 29cc10d053eb..b5c6cf2d8c36 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -1058,7 +1058,9 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev,
ret = drm_universal_plane_init(dev, &plane->base, 0,
&layer_plane_funcs,
desc->formats->formats,
- desc->formats->nformats, type, NULL);
+ desc->formats->nformats,
+ NULL,
+ type, NULL);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 2b33825f2f93..9cb1eede0b4d 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -124,6 +124,7 @@ static struct drm_plane *create_primary_plane(struct drm_device *dev)
&drm_primary_helper_funcs,
safe_modeset_formats,
ARRAY_SIZE(safe_modeset_formats),
+ NULL,
DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
kfree(primary);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index fedd4d60d9cd..20203871e06d 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -70,6 +70,8 @@ static unsigned int drm_num_planes(struct drm_device *dev)
* @funcs: callbacks for the new plane
* @formats: array of supported formats (DRM_FORMAT\_\*)
* @format_count: number of elements in @formats
+ * @format_modifiers: array of struct drm_format modifiers terminated by
+ * DRM_FORMAT_MOD_INVALID
* @type: type of plane (overlay, primary, cursor)
* @name: printf style format string for the plane name, or NULL for default name
*
@@ -82,10 +84,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
uint32_t possible_crtcs,
const struct drm_plane_funcs *funcs,
const uint32_t *formats, unsigned int format_count,
+ const uint64_t *format_modifiers,
enum drm_plane_type type,
const char *name, ...)
{
struct drm_mode_config *config = &dev->mode_config;
+ unsigned int format_modifier_count = 0;
int ret;
ret = drm_mode_object_add(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
@@ -105,6 +109,30 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
return -ENOMEM;
}
+ /* First driver to need more than 64 formats needs to fix this. Each
+ * format is encoded as a bit and the current code only supports a u64.
+ */
+ if (WARN_ON(format_count > 64))
+ return -EINVAL;
+
+ if (format_modifiers) {
+ const uint64_t *temp_modifiers = format_modifiers;
+ while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+ format_modifier_count++;
+ }
+
+ plane->modifier_count = format_modifier_count;
+ plane->modifiers = kmalloc_array(format_modifier_count,
+ sizeof(format_modifiers[0]),
+ GFP_KERNEL);
+
+ if (format_modifier_count && !plane->modifiers) {
+ DRM_DEBUG_KMS("out of memory when allocating plane\n");
+ kfree(plane->format_types);
+ drm_mode_object_unregister(dev, &plane->base);
+ return -ENOMEM;
+ }
+
if (name) {
va_list ap;
@@ -117,12 +145,15 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
}
if (!plane->name) {
kfree(plane->format_types);
+ kfree(plane->modifiers);
drm_mode_object_unregister(dev, &plane->base);
return -ENOMEM;
}
memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
plane->format_count = format_count;
+ memcpy(plane->modifiers, format_modifiers,
+ format_modifier_count * sizeof(format_modifiers[0]));
plane->possible_crtcs = possible_crtcs;
plane->type = type;
@@ -205,7 +236,8 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
- formats, format_count, type, NULL);
+ formats, format_count,
+ NULL, type, NULL);
}
EXPORT_SYMBOL(drm_plane_init);
@@ -224,6 +256,7 @@ void drm_plane_cleanup(struct drm_plane *plane)
drm_modeset_lock_fini(&plane->mutex);
kfree(plane->format_types);
+ kfree(plane->modifiers);
drm_mode_object_unregister(dev, &plane->base);
BUG_ON(list_empty(&plane->head));
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index e084f9f8ca66..949f0ab0e267 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -193,6 +193,7 @@ EXPORT_SYMBOL(drm_simple_display_pipe_attach_bridge);
* @funcs: callbacks for the display pipe (optional)
* @formats: array of supported formats (DRM_FORMAT\_\*)
* @format_count: number of elements in @formats
+ * @format_modifiers: array of formats modifiers
* @connector: connector to attach and register (optional)
*
* Sets up a display pipeline which consist of a really simple
@@ -213,6 +214,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
struct drm_simple_display_pipe *pipe,
const struct drm_simple_display_pipe_funcs *funcs,
const uint32_t *formats, unsigned int format_count,
+ const uint64_t *format_modifiers,
struct drm_connector *connector)
{
struct drm_encoder *encoder = &pipe->encoder;
@@ -227,6 +229,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
ret = drm_universal_plane_init(dev, plane, 0,
&drm_simple_kms_plane_funcs,
formats, format_count,
+ format_modifiers,
DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index c2f17f30afab..75d4928dd196 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -284,7 +284,7 @@ int exynos_plane_init(struct drm_device *dev,
&exynos_plane_funcs,
config->pixel_formats,
config->num_pixel_formats,
- config->type, NULL);
+ NULL, config->type, NULL);
if (err) {
DRM_ERROR("failed to initialize plane\n");
return err;
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
index 0a20723aa6e1..9554b245746e 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -224,7 +224,7 @@ struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
&fsl_dcu_drm_plane_funcs,
fsl_dcu_drm_plane_formats,
ARRAY_SIZE(fsl_dcu_drm_plane_formats),
- DRM_PLANE_TYPE_PRIMARY, NULL);
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
kfree(primary);
primary = NULL;
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 59542bddc980..339e914cbaa3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -181,6 +181,7 @@ static struct drm_plane *hibmc_plane_init(struct hibmc_drm_private *priv)
ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
channel_formats1,
ARRAY_SIZE(channel_formats1),
+ NULL,
DRM_PLANE_TYPE_PRIMARY,
NULL);
if (ret) {
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index c96c228a9898..1acb8af12246 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -909,7 +909,7 @@ static int ade_plane_init(struct drm_device *dev, struct ade_plane *aplane,
return ret;
ret = drm_universal_plane_init(dev, &aplane->base, 1, &ade_plane_funcs,
- fmts, fmts_cnt, type, NULL);
+ fmts, fmts_cnt, NULL, type, NULL);
if (ret) {
DRM_ERROR("fail to init plane, ch=%d\n", aplane->ch);
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3617927af269..9dd67d51e7c9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13594,18 +13594,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
0, &intel_plane_funcs,
intel_primary_formats, num_formats,
+ NULL,
DRM_PLANE_TYPE_PRIMARY,
"plane 1%c", pipe_name(pipe));
else if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
0, &intel_plane_funcs,
intel_primary_formats, num_formats,
+ NULL,
DRM_PLANE_TYPE_PRIMARY,
"primary %c", pipe_name(pipe));
else
ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
0, &intel_plane_funcs,
intel_primary_formats, num_formats,
+ NULL,
DRM_PLANE_TYPE_PRIMARY,
"plane %c", plane_name(primary->plane));
if (ret)
@@ -13776,7 +13779,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
0, &intel_cursor_plane_funcs,
intel_cursor_formats,
ARRAY_SIZE(intel_cursor_formats),
- DRM_PLANE_TYPE_CURSOR,
+ NULL, DRM_PLANE_TYPE_CURSOR,
"cursor %c", pipe_name(pipe));
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f7d431427115..053802dd08c1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1165,13 +1165,13 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
possible_crtcs, &intel_plane_funcs,
plane_formats, num_plane_formats,
- DRM_PLANE_TYPE_OVERLAY,
+ NULL, DRM_PLANE_TYPE_OVERLAY,
"plane %d%c", plane + 2, pipe_name(pipe));
else
ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
possible_crtcs, &intel_plane_funcs,
plane_formats, num_plane_formats,
- DRM_PLANE_TYPE_OVERLAY,
+ NULL, DRM_PLANE_TYPE_OVERLAY,
"sprite %c", sprite_name(pipe, plane));
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index d63e853a0300..6c708c3b1cdc 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -718,8 +718,8 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
ret = drm_universal_plane_init(dev, &ipu_plane->base, possible_crtcs,
&ipu_plane_funcs, ipu_plane_formats,
- ARRAY_SIZE(ipu_plane_formats), type,
- NULL);
+ ARRAY_SIZE(ipu_plane_formats),
+ NULL, type, NULL);
if (ret) {
DRM_ERROR("failed to initialize plane\n");
kfree(ipu_plane);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index e405e89ed5e5..bec6d14dd070 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -173,7 +173,7 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
err = drm_universal_plane_init(dev, plane, possible_crtcs,
&mtk_plane_funcs, formats,
- ARRAY_SIZE(formats), type, NULL);
+ ARRAY_SIZE(formats), NULL, type, NULL);
if (err) {
DRM_ERROR("failed to initialize plane\n");
return err;
diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index a32d3b6e2e12..17e96fa47868 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -223,6 +223,7 @@ int meson_plane_create(struct meson_drm *priv)
&meson_plane_funcs,
supported_drm_formats,
ARRAY_SIZE(supported_drm_formats),
+ NULL,
DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
drm_plane_helper_add(plane, &meson_plane_helper_funcs);
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
index 53619d07677e..8f3417e45d4e 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
@@ -398,7 +398,7 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
ret = drm_universal_plane_init(dev, plane, 0xff, &mdp4_plane_funcs,
mdp4_plane->formats, mdp4_plane->nformats,
- type, NULL);
+ NULL, type, NULL);
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index a38c5fe6cc19..a815bc0e78d1 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -1140,12 +1140,12 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
ret = drm_universal_plane_init(dev, plane, 0xff,
&mdp5_cursor_plane_funcs,
mdp5_plane->formats, mdp5_plane->nformats,
- type, NULL);
+ NULL, type, NULL);
else
ret = drm_universal_plane_init(dev, plane, 0xff,
&mdp5_plane_funcs,
mdp5_plane->formats, mdp5_plane->nformats,
- type, NULL);
+ NULL, type, NULL);
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index d1b9c34c7c00..3ee3784a54f4 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -190,7 +190,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
}
ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs,
- mxsfb_formats, ARRAY_SIZE(mxsfb_formats),
+ mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, 0,
&mxsfb->connector);
if (ret < 0) {
dev_err(drm->dev, "Cannot setup simple display pipe\n");
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 0e58537352fe..dde71be463f8 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1084,8 +1084,9 @@ nv50_wndw_ctor(const struct nv50_wndw_func *func, struct drm_device *dev,
wndw->func = func;
wndw->dmac = dmac;
- ret = drm_universal_plane_init(dev, &wndw->plane, 0, &nv50_wndw, format,
- nformat, type, "%s-%d", name, index);
+ ret = drm_universal_plane_init(dev, &wndw->plane, 0, &nv50_wndw,
+ format, nformat, NULL,
+ type, "%s-%d", name, index);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 9168154d749e..9b8341d77468 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -370,7 +370,8 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
ret = drm_universal_plane_init(dev, plane, possible_crtcs,
&omap_plane_funcs, omap_plane->formats,
- omap_plane->nformats, type, NULL);
+ omap_plane->nformats,
+ NULL, type, NULL);
if (ret < 0)
goto error;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 058340a002c2..fcf1d2034449 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -788,7 +788,7 @@ static struct drm_plane *qxl_create_plane(struct qxl_device *qdev,
err = drm_universal_plane_init(&qdev->ddev, plane, possible_crtcs,
funcs, formats, num_formats,
- type, NULL);
+ NULL, type, NULL);
if (err)
goto free_plane;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index dcde6288da6c..2b02eccbfb70 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -743,8 +743,8 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)
ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
&rcar_du_plane_funcs, formats,
- ARRAY_SIZE(formats), type,
- NULL);
+ ARRAY_SIZE(formats),
+ NULL, type, NULL);
if (ret < 0)
return ret;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index b0ff304ce3dc..e0c054f9b57a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -368,8 +368,8 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp)
1 << vsp->index,
&rcar_du_vsp_plane_funcs,
formats_kms,
- ARRAY_SIZE(formats_kms), type,
- NULL);
+ ARRAY_SIZE(formats_kms),
+ NULL, type, NULL);
if (ret < 0)
return ret;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 3f7a82d1e095..d0cf57de9afc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1280,7 +1280,7 @@ static int vop_create_crtc(struct vop *vop)
0, &vop_plane_funcs,
win_data->phy->data_formats,
win_data->phy->nformats,
- win_data->type, NULL);
+ NULL, win_data->type, NULL);
if (ret) {
DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
ret);
@@ -1319,7 +1319,7 @@ static int vop_create_crtc(struct vop *vop)
&vop_plane_funcs,
win_data->phy->data_formats,
win_data->phy->nformats,
- win_data->type, NULL);
+ NULL, win_data->type, NULL);
if (ret) {
DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
ret);
diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
index cca75bddb9ad..97c25e204bf4 100644
--- a/drivers/gpu/drm/sti/sti_cursor.c
+++ b/drivers/gpu/drm/sti/sti_cursor.c
@@ -393,7 +393,7 @@ struct drm_plane *sti_cursor_create(struct drm_device *drm_dev,
&sti_cursor_plane_helpers_funcs,
cursor_supported_formats,
ARRAY_SIZE(cursor_supported_formats),
- DRM_PLANE_TYPE_CURSOR, NULL);
+ NULL, DRM_PLANE_TYPE_CURSOR, NULL);
if (res) {
DRM_ERROR("Failed to initialize universal plane\n");
goto err_plane;
diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index 88f16cdf6a4b..70391603c12d 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -932,7 +932,7 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
&sti_gdp_plane_helpers_funcs,
gdp_supported_formats,
ARRAY_SIZE(gdp_supported_formats),
- type, NULL);
+ NULL, type, NULL);
if (res) {
DRM_ERROR("Failed to initialize universal plane\n");
goto err;
diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c
index 66f843148ef7..9a1ff352820d 100644
--- a/drivers/gpu/drm/sti/sti_hqvdp.c
+++ b/drivers/gpu/drm/sti/sti_hqvdp.c
@@ -1296,7 +1296,7 @@ static struct drm_plane *sti_hqvdp_create(struct drm_device *drm_dev,
&sti_hqvdp_plane_helpers_funcs,
hqvdp_supported_formats,
ARRAY_SIZE(hqvdp_supported_formats),
- DRM_PLANE_TYPE_OVERLAY, NULL);
+ NULL, DRM_PLANE_TYPE_OVERLAY, NULL);
if (res) {
DRM_ERROR("Failed to initialize universal plane\n");
return NULL;
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index f26bde5b9117..2d9f8d01ac2e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -115,7 +115,7 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
ret = drm_universal_plane_init(drm, &layer->plane, 0,
&sun4i_backend_layer_funcs,
plane->formats, plane->nformats,
- plane->type, NULL);
+ NULL, plane->type, NULL);
if (ret) {
dev_err(drm->dev, "Couldn't initialize layer\n");
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 95b373f739f2..0380edd054ac 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -655,8 +655,8 @@ static struct drm_plane *tegra_dc_primary_plane_create(struct drm_device *drm,
err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
&tegra_primary_plane_funcs, formats,
- num_formats, DRM_PLANE_TYPE_PRIMARY,
- NULL);
+ num_formats, NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
if (err < 0) {
kfree(plane);
return ERR_PTR(err);
@@ -821,8 +821,8 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
err = drm_universal_plane_init(drm, &plane->base, 1 << dc->pipe,
&tegra_cursor_plane_funcs, formats,
- num_formats, DRM_PLANE_TYPE_CURSOR,
- NULL);
+ num_formats, NULL,
+ DRM_PLANE_TYPE_CURSOR, NULL);
if (err < 0) {
kfree(plane);
return ERR_PTR(err);
@@ -883,8 +883,8 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
err = drm_universal_plane_init(drm, &plane->base, 1 << dc->pipe,
&tegra_overlay_plane_funcs, formats,
- num_formats, DRM_PLANE_TYPE_OVERLAY,
- NULL);
+ num_formats, NULL,
+ DRM_PLANE_TYPE_OVERLAY, NULL);
if (err < 0) {
kfree(plane);
return ERR_PTR(err);
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index d34cd5393a9b..74b5e7afd6e9 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -861,7 +861,7 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
ret = drm_universal_plane_init(dev, plane, 0,
&vc4_plane_funcs,
formats, num_formats,
- type, NULL);
+ NULL, type, NULL);
drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index adcdbd0abef6..71ba455af915 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -298,7 +298,7 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
ret = drm_universal_plane_init(dev, plane, 1 << index,
&virtio_gpu_plane_funcs,
formats, nformats,
- type, NULL);
+ NULL, type, NULL);
if (ret)
goto err_plane_init;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index d3987bcf53f8..6f0bf6f9c6f8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -439,7 +439,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
0, &vmw_ldu_plane_funcs,
vmw_primary_plane_formats,
ARRAY_SIZE(vmw_primary_plane_formats),
- DRM_PLANE_TYPE_PRIMARY, NULL);
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
DRM_ERROR("Failed to initialize primary plane");
goto err_free;
@@ -454,7 +454,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
0, &vmw_ldu_cursor_funcs,
vmw_cursor_plane_formats,
ARRAY_SIZE(vmw_cursor_plane_formats),
- DRM_PLANE_TYPE_CURSOR, NULL);
+ NULL, DRM_PLANE_TYPE_CURSOR, NULL);
if (ret) {
DRM_ERROR("Failed to initialize cursor plane");
drm_plane_cleanup(&ldu->base.primary);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 8d7dc9def7c2..42780c9cb90f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -622,7 +622,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
0, &vmw_sou_plane_funcs,
vmw_primary_plane_formats,
ARRAY_SIZE(vmw_primary_plane_formats),
- DRM_PLANE_TYPE_PRIMARY, NULL);
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
DRM_ERROR("Failed to initialize primary plane");
goto err_free;
@@ -637,7 +637,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
0, &vmw_sou_cursor_funcs,
vmw_cursor_plane_formats,
ARRAY_SIZE(vmw_cursor_plane_formats),
- DRM_PLANE_TYPE_CURSOR, NULL);
+ NULL, DRM_PLANE_TYPE_CURSOR, NULL);
if (ret) {
DRM_ERROR("Failed to initialize cursor plane");
drm_plane_cleanup(&sou->base.primary);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index bad31bdf09b6..f18fb966ce9c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1456,7 +1456,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
0, &vmw_stdu_plane_funcs,
vmw_primary_plane_formats,
ARRAY_SIZE(vmw_primary_plane_formats),
- DRM_PLANE_TYPE_PRIMARY, NULL);
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
DRM_ERROR("Failed to initialize primary plane");
goto err_free;
@@ -1471,7 +1471,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
0, &vmw_stdu_cursor_funcs,
vmw_cursor_plane_formats,
ARRAY_SIZE(vmw_cursor_plane_formats),
- DRM_PLANE_TYPE_CURSOR, NULL);
+ NULL, DRM_PLANE_TYPE_CURSOR, NULL);
if (ret) {
DRM_ERROR("Failed to initialize cursor plane");
drm_plane_cleanup(&stdu->base.primary);
diff --git a/drivers/gpu/drm/zte/zx_plane.c b/drivers/gpu/drm/zte/zx_plane.c
index d646ac931663..ea29fee01f7d 100644
--- a/drivers/gpu/drm/zte/zx_plane.c
+++ b/drivers/gpu/drm/zte/zx_plane.c
@@ -539,7 +539,7 @@ int zx_plane_init(struct drm_device *drm, struct zx_plane *zplane,
ret = drm_universal_plane_init(drm, plane, VOU_CRTC_MASK,
&zx_plane_funcs, formats, format_count,
- type, NULL);
+ NULL, type, NULL);
if (ret) {
DRM_DEV_ERROR(dev, "failed to init universal plane: %d\n", ret);
return ret;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 9ab3e7044812..2db5eed53d7b 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -392,6 +392,20 @@ struct drm_plane_funcs {
*/
void (*atomic_print_state)(struct drm_printer *p,
const struct drm_plane_state *state);
+
+ /**
+ * @format_mod_supported:
+ *
+ * This optional hook is used for the DRM to determine if the given
+ * format/modifier combination is valid for the plane. This allows the
+ * DRM to generate the correct format bitmask (which formats apply to
+ * which modifier).
+ *
+ * True if the given modifier is valid for that format on the plane.
+ * False otherwise.
+ */
+ bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
+ uint64_t modifier);
};
/**
@@ -487,6 +501,9 @@ struct drm_plane {
unsigned int format_count;
bool format_default;
+ uint64_t *modifiers;
+ unsigned int modifier_count;
+
struct drm_crtc *crtc;
struct drm_framebuffer *fb;
@@ -527,13 +544,14 @@ struct drm_plane {
#define obj_to_plane(x) container_of(x, struct drm_plane, base)
-__printf(8, 9)
+__printf(9, 10)
int drm_universal_plane_init(struct drm_device *dev,
struct drm_plane *plane,
uint32_t possible_crtcs,
const struct drm_plane_funcs *funcs,
const uint32_t *formats,
unsigned int format_count,
+ const uint64_t *format_modifiers,
enum drm_plane_type type,
const char *name, ...);
int drm_plane_init(struct drm_device *dev,
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 2d36538e4a17..6d9adbb46293 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -122,6 +122,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
struct drm_simple_display_pipe *pipe,
const struct drm_simple_display_pipe_funcs *funcs,
const uint32_t *formats, unsigned int format_count,
+ const uint64_t *format_modifiers,
struct drm_connector *connector);
#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 55e301047b3e..47f0175e29c0 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -184,6 +184,8 @@ extern "C" {
#define DRM_FORMAT_MOD_VENDOR_VIVANTE 0x06
/* add more to the end as needed */
+#define DRM_FORMAT_RESERVED ((1ULL << 56) - 1)
+
#define fourcc_mod_code(vendor, val) \
((((__u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffULL))
@@ -196,6 +198,15 @@ extern "C" {
*/
/*
+ * Invalid Modifier
+ *
+ * This modifier can be used as a sentinel to terminate list, or to initialize a
+ * variable with an invalid modifier. It might also be used to report an error
+ * back to userspace for certain APIs.
+ */
+#define DRM_FORMAT_MOD_INVALID fourcc_mod_code(NONE, DRM_FORMAT_RESERVED)
+
+/*
* Linear Layout
*
* Just plain linear layout. Note that this is different from no specifying any
--
2.13.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-16 21:31 [PATCH v3 1/3] drm: Plumb modifiers through plane init Ben Widawsky
@ 2017-05-16 21:31 ` Ben Widawsky
2017-05-16 23:24 ` Liviu Dudau
` (2 more replies)
2017-05-16 21:31 ` [PATCH v5 3/3] drm/i915: Add format modifiers for Intel Ben Widawsky
` (2 subsequent siblings)
3 siblings, 3 replies; 16+ messages in thread
From: Ben Widawsky @ 2017-05-16 21:31 UTC (permalink / raw)
To: Intel GFX, DRI Development
Cc: Liviu Dudau, Ben Widawsky, Kristian H . Kristensen, Daniel Stone
Updated blob layout (Rob, Daniel, Kristian, xerpi)
v2:
* Removed __packed, and alignment (.+)
* Fix indent in drm_format_modifier fields (Liviu)
* Remove duplicated modifier > 64 check (Liviu)
* Change comment about modifier (Liviu)
* Remove arguments to blob creation, use plane instead (Liviu)
* Fix data types (Ben)
* Make the blob part of uapi (Daniel)
Cc: Rob Clark <robdclark@gmail.com>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
Cc: Liviu Dudau <liviu@dudau.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Daniel Stone <daniels@collabora.com>
---
drivers/gpu/drm/drm_mode_config.c | 7 +++
drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++++++++++++++++++++++++
include/drm/drm_mode_config.h | 6 +++
include/uapi/drm/drm_mode.h | 50 ++++++++++++++++++++++
4 files changed, 152 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index d9862259a2a7..6bfbc3839df5 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -337,6 +337,13 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
return -ENOMEM;
dev->mode_config.gamma_lut_size_property = prop;
+ prop = drm_property_create(dev,
+ DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
+ "IN_FORMATS", 0);
+ if (!prop)
+ return -ENOMEM;
+ dev->mode_config.modifiers = prop;
+
return 0;
}
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 20203871e06d..f5b032b5f761 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -62,6 +62,92 @@ static unsigned int drm_num_planes(struct drm_device *dev)
return num;
}
+static inline u32 *
+formats_ptr(struct drm_format_modifier_blob *blob)
+{
+ return (u32 *)(((char *)blob) + blob->formats_offset);
+}
+
+static inline struct drm_format_modifier *
+modifiers_ptr(struct drm_format_modifier_blob *blob)
+{
+ return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset);
+}
+
+static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
+{
+ const struct drm_mode_config *config = &dev->mode_config;
+ const uint64_t *temp_modifiers = plane->modifiers;
+ unsigned int format_modifier_count = 0;
+ struct drm_property_blob *blob = NULL;
+ struct drm_format_modifier *mod;
+ size_t blob_size = 0, formats_size, modifiers_size;
+ struct drm_format_modifier_blob *blob_data;
+ int i, j, ret = 0;
+
+ if (plane->modifiers)
+ while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+ format_modifier_count++;
+
+ formats_size = sizeof(*plane->format_types) * plane->format_count;
+ if (WARN_ON(!formats_size)) {
+ /* 0 formats are never expected */
+ return 0;
+ }
+
+ modifiers_size =
+ sizeof(struct drm_format_modifier) * format_modifier_count;
+
+ blob_size = sizeof(struct drm_format_modifier_blob);
+ blob_size += ALIGN(formats_size, 8);
+ blob_size += modifiers_size;
+
+ blob = drm_property_create_blob(dev, blob_size, NULL);
+ if (IS_ERR(blob))
+ return -1;
+
+ blob_data = (struct drm_format_modifier_blob *)blob->data;
+ blob_data->version = FORMAT_BLOB_CURRENT;
+ blob_data->count_formats = plane->format_count;
+ blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
+ blob_data->count_modifiers = format_modifier_count;
+
+ /* Modifiers offset is a pointer to a struct with a 64 bit field so it
+ * should be naturally aligned to 8B.
+ */
+ blob_data->modifiers_offset =
+ ALIGN(blob_data->formats_offset + formats_size, 8);
+
+ memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
+
+ /* If we can't determine support, just bail */
+ if (!plane->funcs->format_mod_supported)
+ goto done;
+
+ mod = modifiers_ptr(blob_data);
+ for (i = 0; i < format_modifier_count; i++) {
+ for (j = 0; j < plane->format_count; j++) {
+ if (plane->funcs->format_mod_supported(plane,
+ plane->format_types[j],
+ plane->modifiers[i])) {
+
+ mod->formats |= 1 << j;
+ }
+ }
+
+ mod->modifier = plane->modifiers[i];
+ mod->offset = 0;
+ mod->pad = 0;
+ mod++;
+ }
+
+done:
+ drm_object_attach_property(&plane->base, config->modifiers,
+ blob->base.id);
+
+ return ret;
+}
+
/**
* drm_universal_plane_init - Initialize a new universal plane object
* @dev: DRM device
@@ -180,6 +266,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
drm_object_attach_property(&plane->base, config->prop_src_h, 0);
}
+ if (config->allow_fb_modifiers)
+ create_in_format_blob(dev, plane);
+
return 0;
}
EXPORT_SYMBOL(drm_universal_plane_init);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 42981711189b..03776e659811 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -757,6 +757,12 @@ struct drm_mode_config {
*/
bool allow_fb_modifiers;
+ /**
+ * @modifiers: Plane property to list support modifier/format
+ * combination.
+ */
+ struct drm_property *modifiers;
+
/* cursor size */
uint32_t cursor_width, cursor_height;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 8c67fc03d53d..7ad567903fdc 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -665,6 +665,56 @@ struct drm_mode_atomic {
__u64 user_data;
};
+struct drm_format_modifier_blob {
+#define FORMAT_BLOB_CURRENT 1
+ /* Version of this blob format */
+ u32 version;
+
+ /* Flags */
+ u32 flags;
+
+ /* Number of fourcc formats supported */
+ u32 count_formats;
+
+ /* Where in this blob the formats exist (in bytes) */
+ u32 formats_offset;
+
+ /* Number of drm_format_modifiers */
+ u32 count_modifiers;
+
+ /* Where in this blob the modifiers exist (in bytes) */
+ u32 modifiers_offset;
+
+ /* u32 formats[] */
+ /* struct drm_format_modifier modifiers[] */
+};
+
+struct drm_format_modifier {
+ /* Bitmask of formats in get_plane format list this info applies to. The
+ * offset allows a sliding window of which 64 formats (bits).
+ *
+ * Some examples:
+ * In today's world with < 65 formats, and formats 0, and 2 are
+ * supported
+ * 0x0000000000000005
+ * ^-offset = 0, formats = 5
+ *
+ * If the number formats grew to 128, and formats 98-102 are
+ * supported with the modifier:
+ *
+ * 0x0000003c00000000 0000000000000000
+ * ^
+ * |__offset = 64, formats = 0x3c00000000
+ *
+ */
+ __u64 formats;
+ __u32 offset;
+ __u32 pad;
+
+ /* The modifier that applies to the >get_plane format list bitmask. */
+ __u64 modifier;
+};
+
/**
* Create a new 'blob' data property, copying length bytes from data pointer,
* and returning new blob ID.
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v5 3/3] drm/i915: Add format modifiers for Intel
2017-05-16 21:31 [PATCH v3 1/3] drm: Plumb modifiers through plane init Ben Widawsky
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
@ 2017-05-16 21:31 ` Ben Widawsky
2017-05-17 0:20 ` Emil Velikov
2017-05-16 21:51 ` ✗ Fi.CI.BAT: failure for series starting with [v3,1/3] drm: Plumb modifiers through plane init Patchwork
2017-05-17 10:17 ` [PATCH v3 1/3] " Liviu Dudau
3 siblings, 1 reply; 16+ messages in thread
From: Ben Widawsky @ 2017-05-16 21:31 UTC (permalink / raw)
To: Intel GFX, DRI Development; +Cc: Ben Widawsky, Kristian H . Kristensen
This was based on a patch originally by Kristian. It has been modified
pretty heavily to use the new callbacks from the previous patch.
v2:
- Add LINEAR and Yf modifiers to list (Ville)
- Combine i8xx and i965 into one list of formats (Ville)
- Allow 1010102 formats for Y/Yf tiled (Ville)
v3:
- Handle cursor formats (Ville)
- Put handling for LINEAR in the mod_support functions (Ville)
v4:
- List each modifier explicitly in supported modifiers (Ville)
- Handle the CURSOR plane (Ville)
v5:
- Split out cursor and sprite handling (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/intel_display.c | 131 +++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_sprite.c | 76 +++++++++++++++++++-
2 files changed, 201 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9dd67d51e7c9..3519c10abcc3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -72,6 +72,12 @@ static const uint32_t i965_primary_formats[] = {
DRM_FORMAT_XBGR2101010,
};
+static const uint64_t i9xx_format_modifiers[] = {
+ I915_FORMAT_MOD_X_TILED,
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
static const uint32_t skl_primary_formats[] = {
DRM_FORMAT_C8,
DRM_FORMAT_RGB565,
@@ -87,6 +93,14 @@ static const uint32_t skl_primary_formats[] = {
DRM_FORMAT_VYUY,
};
+static const uint64_t skl_format_modifiers[] = {
+ I915_FORMAT_MOD_Yf_TILED,
+ I915_FORMAT_MOD_Y_TILED,
+ I915_FORMAT_MOD_X_TILED,
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
/* Cursor formats */
static const uint32_t intel_cursor_formats[] = {
DRM_FORMAT_ARGB8888,
@@ -13381,6 +13395,103 @@ void intel_plane_destroy(struct drm_plane *plane)
kfree(to_intel_plane(plane));
}
+static bool i8xx_mod_supported(uint32_t format, uint64_t modifier)
+{
+ switch (format) {
+ case DRM_FORMAT_C8:
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_XRGB1555:
+ case DRM_FORMAT_XRGB8888:
+ return modifier == DRM_FORMAT_MOD_LINEAR ||
+ modifier == I915_FORMAT_MOD_X_TILED;
+ default:
+ return false;
+ }
+}
+
+static bool i965_mod_supported(uint32_t format, uint64_t modifier)
+{
+ switch (format) {
+ case DRM_FORMAT_C8:
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_XRGB2101010:
+ case DRM_FORMAT_XBGR2101010:
+ return modifier == DRM_FORMAT_MOD_LINEAR ||
+ modifier == I915_FORMAT_MOD_X_TILED;
+ default:
+ return false;
+ }
+}
+
+static bool skl_mod_supported(uint32_t format, uint64_t modifier)
+{
+ switch (format) {
+ case DRM_FORMAT_C8:
+ switch (modifier) {
+ case DRM_FORMAT_MOD_LINEAR:
+ case I915_FORMAT_MOD_X_TILED:
+ case I915_FORMAT_MOD_Y_TILED:
+ return true;
+ default:
+ return false;
+ }
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_ABGR8888:
+ case DRM_FORMAT_XRGB2101010:
+ case DRM_FORMAT_XBGR2101010:
+ case DRM_FORMAT_YUYV:
+ case DRM_FORMAT_YVYU:
+ case DRM_FORMAT_UYVY:
+ case DRM_FORMAT_VYUY:
+ /* All i915 modifiers are fine */
+ switch (modifier) {
+ case DRM_FORMAT_MOD_LINEAR:
+ case I915_FORMAT_MOD_X_TILED:
+ case I915_FORMAT_MOD_Y_TILED:
+ case I915_FORMAT_MOD_Yf_TILED:
+ return true;
+ default:
+ return false;
+ }
+ default:
+ return false;
+ }
+}
+
+static bool intel_primary_plane_format_mod_supported(struct drm_plane *plane,
+ uint32_t format,
+ uint64_t modifier)
+{
+ struct drm_i915_private *dev_priv = to_i915(plane->dev);
+
+ if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
+ return false;
+
+ if (INTEL_GEN(dev_priv) >= 9)
+ return skl_mod_supported(format, modifier);
+ else if (INTEL_GEN(dev_priv) >= 4)
+ return i965_mod_supported(format, modifier);
+ else
+ return i8xx_mod_supported(format, modifier);
+
+ return false;
+}
+
+static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
+ uint32_t format,
+ uint64_t modifier)
+{
+ if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
+ return false;
+
+ return modifier == DRM_FORMAT_MOD_LINEAR && format == DRM_FORMAT_ARGB8888;
+}
+
const struct drm_plane_funcs intel_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
@@ -13390,6 +13501,7 @@ const struct drm_plane_funcs intel_plane_funcs = {
.atomic_set_property = intel_plane_atomic_set_property,
.atomic_duplicate_state = intel_plane_duplicate_state,
.atomic_destroy_state = intel_plane_destroy_state,
+ .format_mod_supported = intel_primary_plane_format_mod_supported,
};
static int
@@ -13525,6 +13637,7 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
.atomic_set_property = intel_plane_atomic_set_property,
.atomic_duplicate_state = intel_plane_duplicate_state,
.atomic_destroy_state = intel_plane_destroy_state,
+ .format_mod_supported = intel_cursor_plane_format_mod_supported,
};
static struct intel_plane *
@@ -13535,6 +13648,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
const uint32_t *intel_primary_formats;
unsigned int supported_rotations;
unsigned int num_formats;
+ const uint64_t *intel_format_modifiers;
int ret;
primary = kzalloc(sizeof(*primary), GFP_KERNEL);
@@ -13573,18 +13687,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
if (INTEL_GEN(dev_priv) >= 9) {
intel_primary_formats = skl_primary_formats;
num_formats = ARRAY_SIZE(skl_primary_formats);
+ intel_format_modifiers = skl_format_modifiers;
primary->update_plane = skylake_update_primary_plane;
primary->disable_plane = skylake_disable_primary_plane;
} else if (INTEL_GEN(dev_priv) >= 4) {
intel_primary_formats = i965_primary_formats;
num_formats = ARRAY_SIZE(i965_primary_formats);
+ intel_format_modifiers = i9xx_format_modifiers;
primary->update_plane = i9xx_update_primary_plane;
primary->disable_plane = i9xx_disable_primary_plane;
} else {
intel_primary_formats = i8xx_primary_formats;
num_formats = ARRAY_SIZE(i8xx_primary_formats);
+ intel_format_modifiers = i9xx_format_modifiers;
primary->update_plane = i9xx_update_primary_plane;
primary->disable_plane = i9xx_disable_primary_plane;
@@ -13594,21 +13711,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
0, &intel_plane_funcs,
intel_primary_formats, num_formats,
- NULL,
+ intel_format_modifiers,
DRM_PLANE_TYPE_PRIMARY,
"plane 1%c", pipe_name(pipe));
else if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
0, &intel_plane_funcs,
intel_primary_formats, num_formats,
- NULL,
+ intel_format_modifiers,
DRM_PLANE_TYPE_PRIMARY,
"primary %c", pipe_name(pipe));
else
ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
0, &intel_plane_funcs,
intel_primary_formats, num_formats,
- NULL,
+ intel_format_modifiers,
DRM_PLANE_TYPE_PRIMARY,
"plane %c", plane_name(primary->plane));
if (ret)
@@ -13744,6 +13861,11 @@ intel_update_cursor_plane(struct drm_plane *plane,
intel_crtc_update_cursor(crtc, state);
}
+static const uint64_t cursor_format_modifiers[] = {
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
static struct intel_plane *
intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
{
@@ -13779,7 +13901,8 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
0, &intel_cursor_plane_funcs,
intel_cursor_formats,
ARRAY_SIZE(intel_cursor_formats),
- NULL, DRM_PLANE_TYPE_CURSOR,
+ cursor_format_modifiers,
+ DRM_PLANE_TYPE_CURSOR,
"cursor %c", pipe_name(pipe));
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 053802dd08c1..e7c5c0a94bbd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -30,6 +30,7 @@
* support.
*/
#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_crtc.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_rect.h>
@@ -1032,6 +1033,12 @@ static const uint32_t ilk_plane_formats[] = {
DRM_FORMAT_VYUY,
};
+static const uint64_t i9xx_plane_format_modifiers[] = {
+ I915_FORMAT_MOD_X_TILED,
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
static const uint32_t snb_plane_formats[] = {
DRM_FORMAT_XBGR8888,
DRM_FORMAT_XRGB8888,
@@ -1067,6 +1074,64 @@ static uint32_t skl_plane_formats[] = {
DRM_FORMAT_VYUY,
};
+static const uint64_t skl_plane_format_modifiers[] = {
+ I915_FORMAT_MOD_Yf_TILED,
+ I915_FORMAT_MOD_Y_TILED,
+ I915_FORMAT_MOD_X_TILED,
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
+static bool intel_sprite_plane_format_mod_supported(struct drm_plane *plane,
+ uint32_t format,
+ uint64_t modifier)
+{
+ struct drm_i915_private *dev_priv = to_i915(plane->dev);
+
+ if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
+ return false;
+
+ BUG_ON(plane->base.type != DRM_PLANE_TYPE_OVERLAY);
+
+ switch (format) {
+ case DRM_FORMAT_XBGR2101010:
+ case DRM_FORMAT_ABGR2101010:
+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+ return true;
+ break;
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_ABGR8888:
+ case DRM_FORMAT_ARGB8888:
+ if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+ return true;
+ break;
+ case DRM_FORMAT_XBGR8888:
+ if (INTEL_GEN(dev_priv) >= 6)
+ return true;
+ break;
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_YUYV:
+ case DRM_FORMAT_YVYU:
+ case DRM_FORMAT_UYVY:
+ case DRM_FORMAT_VYUY:
+ return true;
+ }
+
+ return false;
+}
+
+const struct drm_plane_funcs intel_sprite_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = intel_plane_destroy,
+ .set_property = drm_atomic_helper_plane_set_property,
+ .atomic_get_property = intel_plane_atomic_get_property,
+ .atomic_set_property = intel_plane_atomic_set_property,
+ .atomic_duplicate_state = intel_plane_duplicate_state,
+ .atomic_destroy_state = intel_plane_destroy_state,
+ .format_mod_supported = intel_sprite_plane_format_mod_supported,
+};
+
struct intel_plane *
intel_sprite_plane_create(struct drm_i915_private *dev_priv,
enum pipe pipe, int plane)
@@ -1075,6 +1140,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
struct intel_plane_state *state = NULL;
unsigned long possible_crtcs;
const uint32_t *plane_formats;
+ const uint64_t *modifiers;
unsigned int supported_rotations;
int num_plane_formats;
int ret;
@@ -1101,6 +1167,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
plane_formats = skl_plane_formats;
num_plane_formats = ARRAY_SIZE(skl_plane_formats);
+ modifiers = skl_plane_format_modifiers;
} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
intel_plane->can_scale = false;
intel_plane->max_downscale = 1;
@@ -1110,6 +1177,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
plane_formats = vlv_plane_formats;
num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
+ modifiers = i9xx_plane_format_modifiers;
} else if (INTEL_GEN(dev_priv) >= 7) {
if (IS_IVYBRIDGE(dev_priv)) {
intel_plane->can_scale = true;
@@ -1124,6 +1192,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
plane_formats = snb_plane_formats;
num_plane_formats = ARRAY_SIZE(snb_plane_formats);
+ modifiers = i9xx_plane_format_modifiers;
} else {
intel_plane->can_scale = true;
intel_plane->max_downscale = 16;
@@ -1131,6 +1200,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
intel_plane->update_plane = ilk_update_plane;
intel_plane->disable_plane = ilk_disable_plane;
+ modifiers = i9xx_plane_format_modifiers;
if (IS_GEN6(dev_priv)) {
plane_formats = snb_plane_formats;
num_plane_formats = ARRAY_SIZE(snb_plane_formats);
@@ -1165,13 +1235,15 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
possible_crtcs, &intel_plane_funcs,
plane_formats, num_plane_formats,
- NULL, DRM_PLANE_TYPE_OVERLAY,
+ modifiers,
+ DRM_PLANE_TYPE_OVERLAY,
"plane %d%c", plane + 2, pipe_name(pipe));
else
ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
possible_crtcs, &intel_plane_funcs,
plane_formats, num_plane_formats,
- NULL, DRM_PLANE_TYPE_OVERLAY,
+ modifiers,
+ DRM_PLANE_TYPE_OVERLAY,
"sprite %c", sprite_name(pipe, plane));
if (ret)
goto fail;
--
2.13.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [v3,1/3] drm: Plumb modifiers through plane init
2017-05-16 21:31 [PATCH v3 1/3] drm: Plumb modifiers through plane init Ben Widawsky
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
2017-05-16 21:31 ` [PATCH v5 3/3] drm/i915: Add format modifiers for Intel Ben Widawsky
@ 2017-05-16 21:51 ` Patchwork
2017-05-17 10:17 ` [PATCH v3 1/3] " Liviu Dudau
3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-05-16 21:51 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v3,1/3] drm: Plumb modifiers through plane init
URL : https://patchwork.freedesktop.org/series/24528/
State : failure
== Summary ==
Series 24528v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24528/revisions/1/mbox/
Test core_auth:
Subgroup basic-auth:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Test core_prop_blob:
Subgroup basic:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Test drv_getparams_basic:
Subgroup basic-eu-total:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Subgroup basic-subslice-total:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Test drv_hangman:
Subgroup error-state-basic:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Test gem_basic:
Subgroup bad-close:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Subgroup create-close:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Subgroup create-fd-close:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
pass -> SKIP (fi-snb-2600)
pass -> SKIP (fi-ivb-3520m)
pass -> SKIP (fi-byt-j1900)
pass -> SKIP (fi-byt-n2820)
pass -> SKIP (fi-hsw-4770)
pass -> SKIP (fi-bdw-gvtdvm)
pass -> SKIP (fi-bsw-n3050)
pass -> SKIP (fi-skl-gvtdvm)
Test gem_busy:
Subgroup basic-busy-default:
pass -> SKIP (fi-ilk-650)
pass -> SKIP (fi-snb-2520m)
WARNING: Long output truncated
fi-bxt-j4205 failed to connect after reboot
fi-skl-6700k failed to connect after reboot
713f8ec675d5a793326b94d9a0cc484608eff75e drm-tip: 2017y-05m-16d-15h-22m-08s UTC integration manifest
c866de0 drm: Create a format/modifier blob
ce7366f drm: Plumb modifiers through plane init
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4711/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
@ 2017-05-16 23:24 ` Liviu Dudau
2017-05-17 0:06 ` Emil Velikov
2017-05-23 16:39 ` Daniel Stone
2 siblings, 0 replies; 16+ messages in thread
From: Liviu Dudau @ 2017-05-16 23:24 UTC (permalink / raw)
To: Ben Widawsky
Cc: Intel GFX, Kristian H . Kristensen, DRI Development, Daniel Stone
On Tue, May 16, 2017 at 02:31:25PM -0700, Ben Widawsky wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
>
> v2:
> * Removed __packed, and alignment (.+)
> * Fix indent in drm_format_modifier fields (Liviu)
> * Remove duplicated modifier > 64 check (Liviu)
> * Change comment about modifier (Liviu)
> * Remove arguments to blob creation, use plane instead (Liviu)
> * Fix data types (Ben)
> * Make the blob part of uapi (Daniel)
Thanks for updating the patch!
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Cc: Liviu Dudau <liviu@dudau.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
Looks good to me!
Reviewed-by: Liviu Dudau <liviu@dudau.co.uk>
Best regards,
Liviu
> ---
> drivers/gpu/drm/drm_mode_config.c | 7 +++
> drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++++++++++++++++++++++++
> include/drm/drm_mode_config.h | 6 +++
> include/uapi/drm/drm_mode.h | 50 ++++++++++++++++++++++
> 4 files changed, 152 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index d9862259a2a7..6bfbc3839df5 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -337,6 +337,13 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> return -ENOMEM;
> dev->mode_config.gamma_lut_size_property = prop;
>
> + prop = drm_property_create(dev,
> + DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> + "IN_FORMATS", 0);
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.modifiers = prop;
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 20203871e06d..f5b032b5f761 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -62,6 +62,92 @@ static unsigned int drm_num_planes(struct drm_device *dev)
> return num;
> }
>
> +static inline u32 *
> +formats_ptr(struct drm_format_modifier_blob *blob)
> +{
> + return (u32 *)(((char *)blob) + blob->formats_offset);
> +}
> +
> +static inline struct drm_format_modifier *
> +modifiers_ptr(struct drm_format_modifier_blob *blob)
> +{
> + return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset);
> +}
> +
> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> +{
> + const struct drm_mode_config *config = &dev->mode_config;
> + const uint64_t *temp_modifiers = plane->modifiers;
> + unsigned int format_modifier_count = 0;
> + struct drm_property_blob *blob = NULL;
> + struct drm_format_modifier *mod;
> + size_t blob_size = 0, formats_size, modifiers_size;
> + struct drm_format_modifier_blob *blob_data;
> + int i, j, ret = 0;
> +
> + if (plane->modifiers)
> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> + format_modifier_count++;
> +
> + formats_size = sizeof(*plane->format_types) * plane->format_count;
> + if (WARN_ON(!formats_size)) {
> + /* 0 formats are never expected */
> + return 0;
> + }
> +
> + modifiers_size =
> + sizeof(struct drm_format_modifier) * format_modifier_count;
> +
> + blob_size = sizeof(struct drm_format_modifier_blob);
> + blob_size += ALIGN(formats_size, 8);
> + blob_size += modifiers_size;
> +
> + blob = drm_property_create_blob(dev, blob_size, NULL);
> + if (IS_ERR(blob))
> + return -1;
> +
> + blob_data = (struct drm_format_modifier_blob *)blob->data;
> + blob_data->version = FORMAT_BLOB_CURRENT;
> + blob_data->count_formats = plane->format_count;
> + blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> + blob_data->count_modifiers = format_modifier_count;
> +
> + /* Modifiers offset is a pointer to a struct with a 64 bit field so it
> + * should be naturally aligned to 8B.
> + */
> + blob_data->modifiers_offset =
> + ALIGN(blob_data->formats_offset + formats_size, 8);
> +
> + memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> +
> + /* If we can't determine support, just bail */
> + if (!plane->funcs->format_mod_supported)
> + goto done;
> +
> + mod = modifiers_ptr(blob_data);
> + for (i = 0; i < format_modifier_count; i++) {
> + for (j = 0; j < plane->format_count; j++) {
> + if (plane->funcs->format_mod_supported(plane,
> + plane->format_types[j],
> + plane->modifiers[i])) {
> +
> + mod->formats |= 1 << j;
> + }
> + }
> +
> + mod->modifier = plane->modifiers[i];
> + mod->offset = 0;
> + mod->pad = 0;
> + mod++;
> + }
> +
> +done:
> + drm_object_attach_property(&plane->base, config->modifiers,
> + blob->base.id);
> +
> + return ret;
> +}
> +
> /**
> * drm_universal_plane_init - Initialize a new universal plane object
> * @dev: DRM device
> @@ -180,6 +266,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> }
>
> + if (config->allow_fb_modifiers)
> + create_in_format_blob(dev, plane);
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_universal_plane_init);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 42981711189b..03776e659811 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -757,6 +757,12 @@ struct drm_mode_config {
> */
> bool allow_fb_modifiers;
>
> + /**
> + * @modifiers: Plane property to list support modifier/format
> + * combination.
> + */
> + struct drm_property *modifiers;
> +
> /* cursor size */
> uint32_t cursor_width, cursor_height;
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8c67fc03d53d..7ad567903fdc 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,56 @@ struct drm_mode_atomic {
> __u64 user_data;
> };
>
> +struct drm_format_modifier_blob {
> +#define FORMAT_BLOB_CURRENT 1
> + /* Version of this blob format */
> + u32 version;
> +
> + /* Flags */
> + u32 flags;
> +
> + /* Number of fourcc formats supported */
> + u32 count_formats;
> +
> + /* Where in this blob the formats exist (in bytes) */
> + u32 formats_offset;
> +
> + /* Number of drm_format_modifiers */
> + u32 count_modifiers;
> +
> + /* Where in this blob the modifiers exist (in bytes) */
> + u32 modifiers_offset;
> +
> + /* u32 formats[] */
> + /* struct drm_format_modifier modifiers[] */
> +};
> +
> +struct drm_format_modifier {
> + /* Bitmask of formats in get_plane format list this info applies to. The
> + * offset allows a sliding window of which 64 formats (bits).
> + *
> + * Some examples:
> + * In today's world with < 65 formats, and formats 0, and 2 are
> + * supported
> + * 0x0000000000000005
> + * ^-offset = 0, formats = 5
> + *
> + * If the number formats grew to 128, and formats 98-102 are
> + * supported with the modifier:
> + *
> + * 0x0000003c00000000 0000000000000000
> + * ^
> + * |__offset = 64, formats = 0x3c00000000
> + *
> + */
> + __u64 formats;
> + __u32 offset;
> + __u32 pad;
> +
> + /* The modifier that applies to the >get_plane format list bitmask. */
> + __u64 modifier;
> +};
> +
> /**
> * Create a new 'blob' data property, copying length bytes from data pointer,
> * and returning new blob ID.
> --
> 2.13.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
2017-05-16 23:24 ` Liviu Dudau
@ 2017-05-17 0:06 ` Emil Velikov
2017-05-18 0:46 ` [Intel-gfx] " Ben Widawsky
2017-05-23 16:39 ` Daniel Stone
2 siblings, 1 reply; 16+ messages in thread
From: Emil Velikov @ 2017-05-17 0:06 UTC (permalink / raw)
To: Ben Widawsky
Cc: Liviu Dudau, Intel GFX, Kristian H . Kristensen, Daniel Stone,
DRI Development
Hi Ben,
On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
>
> v2:
> * Removed __packed, and alignment (.+)
> * Fix indent in drm_format_modifier fields (Liviu)
> * Remove duplicated modifier > 64 check (Liviu)
> * Change comment about modifier (Liviu)
> * Remove arguments to blob creation, use plane instead (Liviu)
> * Fix data types (Ben)
> * Make the blob part of uapi (Daniel)
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Cc: Liviu Dudau <liviu@dudau.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
I think this is almost perfect, barring the UAPI nitpick.
The rest is somewhat of a bikeshedding.
With the UAPI resolved, regardless of the rest
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> +{
> + const struct drm_mode_config *config = &dev->mode_config;
> + const uint64_t *temp_modifiers = plane->modifiers;
> + unsigned int format_modifier_count = 0;
> + struct drm_property_blob *blob = NULL;
> + struct drm_format_modifier *mod;
> + size_t blob_size = 0, formats_size, modifiers_size;
There's no need to initialize blob and blob_size here.
> + struct drm_format_modifier_blob *blob_data;
> + int i, j, ret = 0;
Make i and j unsigned to match format_modifier_count and
plane->format_count respectively.
Then expand ret in the only place where it's used?
> +
> + if (plane->modifiers)
> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> + format_modifier_count++;
> +
> + formats_size = sizeof(*plane->format_types) * plane->format_count;
> + if (WARN_ON(!formats_size)) {
> + /* 0 formats are never expected */
> + return 0;
> + }
> +
> + modifiers_size =
> + sizeof(struct drm_format_modifier) * format_modifier_count;
> +
> + blob_size = sizeof(struct drm_format_modifier_blob);
> + blob_size += ALIGN(formats_size, 8);
Worth having the "Modifiers offset is a pointer..." comment moved/copied here?
> + blob_size += modifiers_size;
> +
> + blob = drm_property_create_blob(dev, blob_size, NULL);
> + if (IS_ERR(blob))
> + return -1;
> +
Maybe propagate the exact error... Hmm we don't seem to check if
create_in_format_blob fails either so perhaps it's not worth it.
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,56 @@ struct drm_mode_atomic {
> __u64 user_data;
> };
>
> +struct drm_format_modifier_blob {
> +#define FORMAT_BLOB_CURRENT 1
> + /* Version of this blob format */
> + u32 version;
> +
> + /* Flags */
> + u32 flags;
> +
> + /* Number of fourcc formats supported */
> + u32 count_formats;
> +
> + /* Where in this blob the formats exist (in bytes) */
> + u32 formats_offset;
> +
> + /* Number of drm_format_modifiers */
> + u32 count_modifiers;
> +
> + /* Where in this blob the modifiers exist (in bytes) */
> + u32 modifiers_offset;
> +
> + /* u32 formats[] */
> + /* struct drm_format_modifier modifiers[] */
> +};
> +
> +struct drm_format_modifier {
> + /* Bitmask of formats in get_plane format list this info applies to. The
> + * offset allows a sliding window of which 64 formats (bits).
> + *
> + * Some examples:
> + * In today's world with < 65 formats, and formats 0, and 2 are
> + * supported
> + * 0x0000000000000005
> + * ^-offset = 0, formats = 5
> + *
> + * If the number formats grew to 128, and formats 98-102 are
> + * supported with the modifier:
> + *
> + * 0x0000003c00000000 0000000000000000
> + * ^
> + * |__offset = 64, formats = 0x3c00000000
> + *
> + */
> + __u64 formats;
> + __u32 offset;
> + __u32 pad;
> +
> + /* The modifier that applies to the >get_plane format list bitmask. */
> + __u64 modifier;
Please drop the leading __ from the type names in UAPI headers.
Regards,
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 3/3] drm/i915: Add format modifiers for Intel
2017-05-16 21:31 ` [PATCH v5 3/3] drm/i915: Add format modifiers for Intel Ben Widawsky
@ 2017-05-17 0:20 ` Emil Velikov
2017-05-18 1:14 ` Ben Widawsky
0 siblings, 1 reply; 16+ messages in thread
From: Emil Velikov @ 2017-05-17 0:20 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Kristian H . Kristensen, DRI Development
Hi Ben,
A couple of small questions/suggestions that I hope you find useful.
Please don't block any of this work based on my comments.
On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
> +static bool intel_primary_plane_format_mod_supported(struct drm_plane *plane,
> + uint32_t format,
> + uint64_t modifier)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +
> + if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
> + return false;
> +
> + if (INTEL_GEN(dev_priv) >= 9)
> + return skl_mod_supported(format, modifier);
> + else if (INTEL_GEN(dev_priv) >= 4)
> + return i965_mod_supported(format, modifier);
> + else
> + return i8xx_mod_supported(format, modifier);
> +
Nit: if you rewrite this as below, the control flow should be clearer.
Thus the 'return false;' at the end, will be [more] obvious that it's
unreachable ;-)
if (INTEL_GEN(dev_priv) >= 9)
return skl_mod_supported(format, modifier);
if (INTEL_GEN(dev_priv) >= 4)
return i965_mod_supported(format, modifier);
return i8xx_mod_supported(format, modifier);
> + return false;
> +}
> +
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> +static bool intel_sprite_plane_format_mod_supported(struct drm_plane *plane,
> + uint32_t format,
> + uint64_t modifier)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +
> + if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
> + return false;
> +
> + BUG_ON(plane->base.type != DRM_PLANE_TYPE_OVERLAY);
> +
> + switch (format) {
> + case DRM_FORMAT_XBGR2101010:
> + case DRM_FORMAT_ABGR2101010:
> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + return true;
> + break;
> + case DRM_FORMAT_RGB565:
> + case DRM_FORMAT_ABGR8888:
> + case DRM_FORMAT_ARGB8888:
> + if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + return true;
> + break;
> + case DRM_FORMAT_XBGR8888:
> + if (INTEL_GEN(dev_priv) >= 6)
> + return true;
> + break;
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_YUYV:
> + case DRM_FORMAT_YVYU:
> + case DRM_FORMAT_UYVY:
> + case DRM_FORMAT_VYUY:
> + return true;
> + }
> +
> + return false;
> +}
> +
> +const struct drm_plane_funcs intel_sprite_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = intel_plane_destroy,
> + .set_property = drm_atomic_helper_plane_set_property,
> + .atomic_get_property = intel_plane_atomic_get_property,
> + .atomic_set_property = intel_plane_atomic_set_property,
> + .atomic_duplicate_state = intel_plane_duplicate_state,
> + .atomic_destroy_state = intel_plane_destroy_state,
> + .format_mod_supported = intel_sprite_plane_format_mod_supported,
> +};
> +
Having a dull moment - is intel_sprite_plane_funcs (+
intel_sprite_plane_format_mod_supported) unused or I'm seeing things?
If one is to keep it, for future work, perhaps it's worth adding a 2-3
word comment,
Regards,
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] drm: Plumb modifiers through plane init
2017-05-16 21:31 [PATCH v3 1/3] drm: Plumb modifiers through plane init Ben Widawsky
` (2 preceding siblings ...)
2017-05-16 21:51 ` ✗ Fi.CI.BAT: failure for series starting with [v3,1/3] drm: Plumb modifiers through plane init Patchwork
@ 2017-05-17 10:17 ` Liviu Dudau
2017-05-18 0:26 ` Ben Widawsky
3 siblings, 1 reply; 16+ messages in thread
From: Liviu Dudau @ 2017-05-17 10:17 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Daniel Stone, DRI Development
On Tue, May 16, 2017 at 02:31:24PM -0700, Ben Widawsky wrote:
> This is the plumbing for supporting fb modifiers on planes. Modifiers
> have already been introduced to some extent, but this series will extend
> this to allow querying modifiers per plane. Based on this, the client to
> enable optimal modifications for framebuffers.
>
> This patch simply allows the DRM drivers to initialize their list of
> supported modifiers upon initializing the plane.
>
> v2: A minor addition from Daniel
>
> v3: Updated commit message
> s/INVALID/DRM_FORMAT_MOD_INVALID (Liviu)
> Remove some excess newlines (Liviu)
> Update comment for > 64 modifiers (Liviu)
>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Reviewed-by: Daniel Stone <daniels@collabora.com> (v2)
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Minor nits, see below, but otherwise:
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Thanks,
Liviu
> ---
> drivers/gpu/drm/arc/arcpgu_crtc.c | 1 +
> drivers/gpu/drm/arm/hdlcd_crtc.c | 1 +
> drivers/gpu/drm/arm/malidp_planes.c | 2 +-
> drivers/gpu/drm/armada/armada_crtc.c | 1 +
> drivers/gpu/drm/armada/armada_overlay.c | 1 +
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 4 ++-
> drivers/gpu/drm/drm_modeset_helper.c | 1 +
> drivers/gpu/drm/drm_plane.c | 35 ++++++++++++++++++++++++-
> drivers/gpu/drm/drm_simple_kms_helper.c | 3 +++
> drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 +-
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 2 +-
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 1 +
> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 5 +++-
> drivers/gpu/drm/i915/intel_sprite.c | 4 +--
> drivers/gpu/drm/imx/ipuv3-plane.c | 4 +--
> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
> drivers/gpu/drm/meson/meson_plane.c | 1 +
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 +--
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
> drivers/gpu/drm/nouveau/nv50_display.c | 5 ++--
> drivers/gpu/drm/omapdrm/omap_plane.c | 3 ++-
> drivers/gpu/drm/qxl/qxl_display.c | 2 +-
> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +--
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +--
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 +--
> drivers/gpu/drm/sti/sti_cursor.c | 2 +-
> drivers/gpu/drm/sti/sti_gdp.c | 2 +-
> drivers/gpu/drm/sti/sti_hqvdp.c | 2 +-
> drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
> drivers/gpu/drm/tegra/dc.c | 12 ++++-----
> drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
> drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 +--
> drivers/gpu/drm/zte/zx_plane.c | 2 +-
> include/drm/drm_plane.h | 20 +++++++++++++-
> include/drm/drm_simple_kms_helper.h | 1 +
> include/uapi/drm/drm_fourcc.h | 11 ++++++++
> 41 files changed, 126 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a95916f1f..cd8a24c7c67d 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -218,6 +218,7 @@ static struct drm_plane *arc_pgu_plane_init(struct drm_device *drm)
>
> ret = drm_universal_plane_init(drm, plane, 0xff, &arc_pgu_plane_funcs,
> formats, ARRAY_SIZE(formats),
> + NULL,
> DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret)
> return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 798a3cc480a2..0caa03ae8708 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -303,6 +303,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>
> ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
> formats, ARRAY_SIZE(formats),
> + NULL,
> DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> devm_kfree(drm->dev, plane);
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index 814fda23cead..b156610c68a5 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -400,7 +400,7 @@ int malidp_de_planes_init(struct drm_device *drm)
> DRM_PLANE_TYPE_OVERLAY;
> ret = drm_universal_plane_init(drm, &plane->base, crtcs,
> &malidp_de_plane_funcs, formats,
> - n, plane_type, NULL);
> + n, NULL, plane_type, NULL);
> if (ret < 0)
> goto cleanup;
>
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index 4fe19fde84f9..ea48ef88f0e4 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -1269,6 +1269,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> &armada_primary_plane_funcs,
> armada_primary_formats,
> ARRAY_SIZE(armada_primary_formats),
> + NULL,
> DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> kfree(primary);
> diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c
> index 424e465ff407..0054144287ae 100644
> --- a/drivers/gpu/drm/armada/armada_overlay.c
> +++ b/drivers/gpu/drm/armada/armada_overlay.c
> @@ -460,6 +460,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
> &armada_ovl_plane_funcs,
> armada_ovl_formats,
> ARRAY_SIZE(armada_ovl_formats),
> + NULL,
> DRM_PLANE_TYPE_OVERLAY, NULL);
> if (ret) {
> kfree(dplane);
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index 29cc10d053eb..b5c6cf2d8c36 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> @@ -1058,7 +1058,9 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev,
> ret = drm_universal_plane_init(dev, &plane->base, 0,
> &layer_plane_funcs,
> desc->formats->formats,
> - desc->formats->nformats, type, NULL);
> + desc->formats->nformats,
> + NULL,
> + type, NULL);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 2b33825f2f93..9cb1eede0b4d 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -124,6 +124,7 @@ static struct drm_plane *create_primary_plane(struct drm_device *dev)
> &drm_primary_helper_funcs,
> safe_modeset_formats,
> ARRAY_SIZE(safe_modeset_formats),
> + NULL,
> DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> kfree(primary);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d60d9cd..20203871e06d 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -70,6 +70,8 @@ static unsigned int drm_num_planes(struct drm_device *dev)
> * @funcs: callbacks for the new plane
> * @formats: array of supported formats (DRM_FORMAT\_\*)
> * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by
> + * DRM_FORMAT_MOD_INVALID
> * @type: type of plane (overlay, primary, cursor)
> * @name: printf style format string for the plane name, or NULL for default name
> *
> @@ -82,10 +84,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> uint32_t possible_crtcs,
> const struct drm_plane_funcs *funcs,
> const uint32_t *formats, unsigned int format_count,
> + const uint64_t *format_modifiers,
> enum drm_plane_type type,
> const char *name, ...)
> {
> struct drm_mode_config *config = &dev->mode_config;
> + unsigned int format_modifier_count = 0;
> int ret;
>
> ret = drm_mode_object_add(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> @@ -105,6 +109,30 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> return -ENOMEM;
> }
>
> + /* First driver to need more than 64 formats needs to fix this. Each
Kernel's style asks for multi-line comments to start with an empty /* line
> + * format is encoded as a bit and the current code only supports a u64.
> + */
> + if (WARN_ON(format_count > 64))
> + return -EINVAL;
> +
> + if (format_modifiers) {
> + const uint64_t *temp_modifiers = format_modifiers;
> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> + format_modifier_count++;
> + }
> +
> + plane->modifier_count = format_modifier_count;
> + plane->modifiers = kmalloc_array(format_modifier_count,
> + sizeof(format_modifiers[0]),
> + GFP_KERNEL);
> +
> + if (format_modifier_count && !plane->modifiers) {
> + DRM_DEBUG_KMS("out of memory when allocating plane\n");
> + kfree(plane->format_types);
> + drm_mode_object_unregister(dev, &plane->base);
> + return -ENOMEM;
> + }
> +
> if (name) {
> va_list ap;
>
> @@ -117,12 +145,15 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> }
> if (!plane->name) {
> kfree(plane->format_types);
> + kfree(plane->modifiers);
> drm_mode_object_unregister(dev, &plane->base);
> return -ENOMEM;
> }
>
> memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
> plane->format_count = format_count;
> + memcpy(plane->modifiers, format_modifiers,
> + format_modifier_count * sizeof(format_modifiers[0]));
I'm still worried that we can reach the memcpy with a NULL format_modifiers and we are opening
a security hole here.
> plane->possible_crtcs = possible_crtcs;
> plane->type = type;
>
> @@ -205,7 +236,8 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>
> type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> - formats, format_count, type, NULL);
> + formats, format_count,
> + NULL, type, NULL);
> }
> EXPORT_SYMBOL(drm_plane_init);
>
> @@ -224,6 +256,7 @@ void drm_plane_cleanup(struct drm_plane *plane)
> drm_modeset_lock_fini(&plane->mutex);
>
> kfree(plane->format_types);
> + kfree(plane->modifiers);
> drm_mode_object_unregister(dev, &plane->base);
>
> BUG_ON(list_empty(&plane->head));
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index e084f9f8ca66..949f0ab0e267 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -193,6 +193,7 @@ EXPORT_SYMBOL(drm_simple_display_pipe_attach_bridge);
> * @funcs: callbacks for the display pipe (optional)
> * @formats: array of supported formats (DRM_FORMAT\_\*)
> * @format_count: number of elements in @formats
> + * @format_modifiers: array of formats modifiers
> * @connector: connector to attach and register (optional)
> *
> * Sets up a display pipeline which consist of a really simple
> @@ -213,6 +214,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> struct drm_simple_display_pipe *pipe,
> const struct drm_simple_display_pipe_funcs *funcs,
> const uint32_t *formats, unsigned int format_count,
> + const uint64_t *format_modifiers,
> struct drm_connector *connector)
> {
> struct drm_encoder *encoder = &pipe->encoder;
> @@ -227,6 +229,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> ret = drm_universal_plane_init(dev, plane, 0,
> &drm_simple_kms_plane_funcs,
> formats, format_count,
> + format_modifiers,
> DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index c2f17f30afab..75d4928dd196 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -284,7 +284,7 @@ int exynos_plane_init(struct drm_device *dev,
> &exynos_plane_funcs,
> config->pixel_formats,
> config->num_pixel_formats,
> - config->type, NULL);
> + NULL, config->type, NULL);
> if (err) {
> DRM_ERROR("failed to initialize plane\n");
> return err;
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> index 0a20723aa6e1..9554b245746e 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> @@ -224,7 +224,7 @@ struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
> &fsl_dcu_drm_plane_funcs,
> fsl_dcu_drm_plane_formats,
> ARRAY_SIZE(fsl_dcu_drm_plane_formats),
> - DRM_PLANE_TYPE_PRIMARY, NULL);
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> kfree(primary);
> primary = NULL;
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 59542bddc980..339e914cbaa3 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -181,6 +181,7 @@ static struct drm_plane *hibmc_plane_init(struct hibmc_drm_private *priv)
> ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
> channel_formats1,
> ARRAY_SIZE(channel_formats1),
> + NULL,
> DRM_PLANE_TYPE_PRIMARY,
> NULL);
> if (ret) {
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index c96c228a9898..1acb8af12246 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -909,7 +909,7 @@ static int ade_plane_init(struct drm_device *dev, struct ade_plane *aplane,
> return ret;
>
> ret = drm_universal_plane_init(dev, &aplane->base, 1, &ade_plane_funcs,
> - fmts, fmts_cnt, type, NULL);
> + fmts, fmts_cnt, NULL, type, NULL);
> if (ret) {
> DRM_ERROR("fail to init plane, ch=%d\n", aplane->ch);
> return ret;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3617927af269..9dd67d51e7c9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13594,18 +13594,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
> 0, &intel_plane_funcs,
> intel_primary_formats, num_formats,
> + NULL,
> DRM_PLANE_TYPE_PRIMARY,
> "plane 1%c", pipe_name(pipe));
> else if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
> ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
> 0, &intel_plane_funcs,
> intel_primary_formats, num_formats,
> + NULL,
> DRM_PLANE_TYPE_PRIMARY,
> "primary %c", pipe_name(pipe));
> else
> ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
> 0, &intel_plane_funcs,
> intel_primary_formats, num_formats,
> + NULL,
> DRM_PLANE_TYPE_PRIMARY,
> "plane %c", plane_name(primary->plane));
> if (ret)
> @@ -13776,7 +13779,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> 0, &intel_cursor_plane_funcs,
> intel_cursor_formats,
> ARRAY_SIZE(intel_cursor_formats),
> - DRM_PLANE_TYPE_CURSOR,
> + NULL, DRM_PLANE_TYPE_CURSOR,
> "cursor %c", pipe_name(pipe));
> if (ret)
> goto fail;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index f7d431427115..053802dd08c1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1165,13 +1165,13 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
> possible_crtcs, &intel_plane_funcs,
> plane_formats, num_plane_formats,
> - DRM_PLANE_TYPE_OVERLAY,
> + NULL, DRM_PLANE_TYPE_OVERLAY,
> "plane %d%c", plane + 2, pipe_name(pipe));
> else
> ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
> possible_crtcs, &intel_plane_funcs,
> plane_formats, num_plane_formats,
> - DRM_PLANE_TYPE_OVERLAY,
> + NULL, DRM_PLANE_TYPE_OVERLAY,
> "sprite %c", sprite_name(pipe, plane));
> if (ret)
> goto fail;
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index d63e853a0300..6c708c3b1cdc 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -718,8 +718,8 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
>
> ret = drm_universal_plane_init(dev, &ipu_plane->base, possible_crtcs,
> &ipu_plane_funcs, ipu_plane_formats,
> - ARRAY_SIZE(ipu_plane_formats), type,
> - NULL);
> + ARRAY_SIZE(ipu_plane_formats),
> + NULL, type, NULL);
> if (ret) {
> DRM_ERROR("failed to initialize plane\n");
> kfree(ipu_plane);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index e405e89ed5e5..bec6d14dd070 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -173,7 +173,7 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>
> err = drm_universal_plane_init(dev, plane, possible_crtcs,
> &mtk_plane_funcs, formats,
> - ARRAY_SIZE(formats), type, NULL);
> + ARRAY_SIZE(formats), NULL, type, NULL);
> if (err) {
> DRM_ERROR("failed to initialize plane\n");
> return err;
> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> index a32d3b6e2e12..17e96fa47868 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -223,6 +223,7 @@ int meson_plane_create(struct meson_drm *priv)
> &meson_plane_funcs,
> supported_drm_formats,
> ARRAY_SIZE(supported_drm_formats),
> + NULL,
> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>
> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> index 53619d07677e..8f3417e45d4e 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> @@ -398,7 +398,7 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
> type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> ret = drm_universal_plane_init(dev, plane, 0xff, &mdp4_plane_funcs,
> mdp4_plane->formats, mdp4_plane->nformats,
> - type, NULL);
> + NULL, type, NULL);
> if (ret)
> goto fail;
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index a38c5fe6cc19..a815bc0e78d1 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -1140,12 +1140,12 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> ret = drm_universal_plane_init(dev, plane, 0xff,
> &mdp5_cursor_plane_funcs,
> mdp5_plane->formats, mdp5_plane->nformats,
> - type, NULL);
> + NULL, type, NULL);
> else
> ret = drm_universal_plane_init(dev, plane, 0xff,
> &mdp5_plane_funcs,
> mdp5_plane->formats, mdp5_plane->nformats,
> - type, NULL);
> + NULL, type, NULL);
> if (ret)
> goto fail;
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index d1b9c34c7c00..3ee3784a54f4 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -190,7 +190,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
> }
>
> ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs,
> - mxsfb_formats, ARRAY_SIZE(mxsfb_formats),
> + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, 0,
> &mxsfb->connector);
> if (ret < 0) {
> dev_err(drm->dev, "Cannot setup simple display pipe\n");
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index 0e58537352fe..dde71be463f8 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -1084,8 +1084,9 @@ nv50_wndw_ctor(const struct nv50_wndw_func *func, struct drm_device *dev,
> wndw->func = func;
> wndw->dmac = dmac;
>
> - ret = drm_universal_plane_init(dev, &wndw->plane, 0, &nv50_wndw, format,
> - nformat, type, "%s-%d", name, index);
> + ret = drm_universal_plane_init(dev, &wndw->plane, 0, &nv50_wndw,
> + format, nformat, NULL,
> + type, "%s-%d", name, index);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 9168154d749e..9b8341d77468 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -370,7 +370,8 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>
> ret = drm_universal_plane_init(dev, plane, possible_crtcs,
> &omap_plane_funcs, omap_plane->formats,
> - omap_plane->nformats, type, NULL);
> + omap_plane->nformats,
> + NULL, type, NULL);
> if (ret < 0)
> goto error;
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 058340a002c2..fcf1d2034449 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -788,7 +788,7 @@ static struct drm_plane *qxl_create_plane(struct qxl_device *qdev,
>
> err = drm_universal_plane_init(&qdev->ddev, plane, possible_crtcs,
> funcs, formats, num_formats,
> - type, NULL);
> + NULL, type, NULL);
> if (err)
> goto free_plane;
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index dcde6288da6c..2b02eccbfb70 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -743,8 +743,8 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)
>
> ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
> &rcar_du_plane_funcs, formats,
> - ARRAY_SIZE(formats), type,
> - NULL);
> + ARRAY_SIZE(formats),
> + NULL, type, NULL);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index b0ff304ce3dc..e0c054f9b57a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -368,8 +368,8 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp)
> 1 << vsp->index,
> &rcar_du_vsp_plane_funcs,
> formats_kms,
> - ARRAY_SIZE(formats_kms), type,
> - NULL);
> + ARRAY_SIZE(formats_kms),
> + NULL, type, NULL);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 3f7a82d1e095..d0cf57de9afc 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1280,7 +1280,7 @@ static int vop_create_crtc(struct vop *vop)
> 0, &vop_plane_funcs,
> win_data->phy->data_formats,
> win_data->phy->nformats,
> - win_data->type, NULL);
> + NULL, win_data->type, NULL);
> if (ret) {
> DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
> ret);
> @@ -1319,7 +1319,7 @@ static int vop_create_crtc(struct vop *vop)
> &vop_plane_funcs,
> win_data->phy->data_formats,
> win_data->phy->nformats,
> - win_data->type, NULL);
> + NULL, win_data->type, NULL);
> if (ret) {
> DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
> ret);
> diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
> index cca75bddb9ad..97c25e204bf4 100644
> --- a/drivers/gpu/drm/sti/sti_cursor.c
> +++ b/drivers/gpu/drm/sti/sti_cursor.c
> @@ -393,7 +393,7 @@ struct drm_plane *sti_cursor_create(struct drm_device *drm_dev,
> &sti_cursor_plane_helpers_funcs,
> cursor_supported_formats,
> ARRAY_SIZE(cursor_supported_formats),
> - DRM_PLANE_TYPE_CURSOR, NULL);
> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
> if (res) {
> DRM_ERROR("Failed to initialize universal plane\n");
> goto err_plane;
> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
> index 88f16cdf6a4b..70391603c12d 100644
> --- a/drivers/gpu/drm/sti/sti_gdp.c
> +++ b/drivers/gpu/drm/sti/sti_gdp.c
> @@ -932,7 +932,7 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
> &sti_gdp_plane_helpers_funcs,
> gdp_supported_formats,
> ARRAY_SIZE(gdp_supported_formats),
> - type, NULL);
> + NULL, type, NULL);
> if (res) {
> DRM_ERROR("Failed to initialize universal plane\n");
> goto err;
> diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c
> index 66f843148ef7..9a1ff352820d 100644
> --- a/drivers/gpu/drm/sti/sti_hqvdp.c
> +++ b/drivers/gpu/drm/sti/sti_hqvdp.c
> @@ -1296,7 +1296,7 @@ static struct drm_plane *sti_hqvdp_create(struct drm_device *drm_dev,
> &sti_hqvdp_plane_helpers_funcs,
> hqvdp_supported_formats,
> ARRAY_SIZE(hqvdp_supported_formats),
> - DRM_PLANE_TYPE_OVERLAY, NULL);
> + NULL, DRM_PLANE_TYPE_OVERLAY, NULL);
> if (res) {
> DRM_ERROR("Failed to initialize universal plane\n");
> return NULL;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> index f26bde5b9117..2d9f8d01ac2e 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> @@ -115,7 +115,7 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
> ret = drm_universal_plane_init(drm, &layer->plane, 0,
> &sun4i_backend_layer_funcs,
> plane->formats, plane->nformats,
> - plane->type, NULL);
> + NULL, plane->type, NULL);
> if (ret) {
> dev_err(drm->dev, "Couldn't initialize layer\n");
> return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 95b373f739f2..0380edd054ac 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -655,8 +655,8 @@ static struct drm_plane *tegra_dc_primary_plane_create(struct drm_device *drm,
>
> err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
> &tegra_primary_plane_funcs, formats,
> - num_formats, DRM_PLANE_TYPE_PRIMARY,
> - NULL);
> + num_formats, NULL,
> + DRM_PLANE_TYPE_PRIMARY, NULL);
> if (err < 0) {
> kfree(plane);
> return ERR_PTR(err);
> @@ -821,8 +821,8 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
>
> err = drm_universal_plane_init(drm, &plane->base, 1 << dc->pipe,
> &tegra_cursor_plane_funcs, formats,
> - num_formats, DRM_PLANE_TYPE_CURSOR,
> - NULL);
> + num_formats, NULL,
> + DRM_PLANE_TYPE_CURSOR, NULL);
> if (err < 0) {
> kfree(plane);
> return ERR_PTR(err);
> @@ -883,8 +883,8 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
>
> err = drm_universal_plane_init(drm, &plane->base, 1 << dc->pipe,
> &tegra_overlay_plane_funcs, formats,
> - num_formats, DRM_PLANE_TYPE_OVERLAY,
> - NULL);
> + num_formats, NULL,
> + DRM_PLANE_TYPE_OVERLAY, NULL);
> if (err < 0) {
> kfree(plane);
> return ERR_PTR(err);
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index d34cd5393a9b..74b5e7afd6e9 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -861,7 +861,7 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
> ret = drm_universal_plane_init(dev, plane, 0,
> &vc4_plane_funcs,
> formats, num_formats,
> - type, NULL);
> + NULL, type, NULL);
>
> drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index adcdbd0abef6..71ba455af915 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -298,7 +298,7 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
> ret = drm_universal_plane_init(dev, plane, 1 << index,
> &virtio_gpu_plane_funcs,
> formats, nformats,
> - type, NULL);
> + NULL, type, NULL);
> if (ret)
> goto err_plane_init;
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> index d3987bcf53f8..6f0bf6f9c6f8 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> @@ -439,7 +439,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
> 0, &vmw_ldu_plane_funcs,
> vmw_primary_plane_formats,
> ARRAY_SIZE(vmw_primary_plane_formats),
> - DRM_PLANE_TYPE_PRIMARY, NULL);
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize primary plane");
> goto err_free;
> @@ -454,7 +454,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
> 0, &vmw_ldu_cursor_funcs,
> vmw_cursor_plane_formats,
> ARRAY_SIZE(vmw_cursor_plane_formats),
> - DRM_PLANE_TYPE_CURSOR, NULL);
> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize cursor plane");
> drm_plane_cleanup(&ldu->base.primary);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 8d7dc9def7c2..42780c9cb90f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -622,7 +622,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
> 0, &vmw_sou_plane_funcs,
> vmw_primary_plane_formats,
> ARRAY_SIZE(vmw_primary_plane_formats),
> - DRM_PLANE_TYPE_PRIMARY, NULL);
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize primary plane");
> goto err_free;
> @@ -637,7 +637,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
> 0, &vmw_sou_cursor_funcs,
> vmw_cursor_plane_formats,
> ARRAY_SIZE(vmw_cursor_plane_formats),
> - DRM_PLANE_TYPE_CURSOR, NULL);
> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize cursor plane");
> drm_plane_cleanup(&sou->base.primary);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index bad31bdf09b6..f18fb966ce9c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1456,7 +1456,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
> 0, &vmw_stdu_plane_funcs,
> vmw_primary_plane_formats,
> ARRAY_SIZE(vmw_primary_plane_formats),
> - DRM_PLANE_TYPE_PRIMARY, NULL);
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize primary plane");
> goto err_free;
> @@ -1471,7 +1471,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
> 0, &vmw_stdu_cursor_funcs,
> vmw_cursor_plane_formats,
> ARRAY_SIZE(vmw_cursor_plane_formats),
> - DRM_PLANE_TYPE_CURSOR, NULL);
> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
> if (ret) {
> DRM_ERROR("Failed to initialize cursor plane");
> drm_plane_cleanup(&stdu->base.primary);
> diff --git a/drivers/gpu/drm/zte/zx_plane.c b/drivers/gpu/drm/zte/zx_plane.c
> index d646ac931663..ea29fee01f7d 100644
> --- a/drivers/gpu/drm/zte/zx_plane.c
> +++ b/drivers/gpu/drm/zte/zx_plane.c
> @@ -539,7 +539,7 @@ int zx_plane_init(struct drm_device *drm, struct zx_plane *zplane,
>
> ret = drm_universal_plane_init(drm, plane, VOU_CRTC_MASK,
> &zx_plane_funcs, formats, format_count,
> - type, NULL);
> + NULL, type, NULL);
> if (ret) {
> DRM_DEV_ERROR(dev, "failed to init universal plane: %d\n", ret);
> return ret;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9ab3e7044812..2db5eed53d7b 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -392,6 +392,20 @@ struct drm_plane_funcs {
> */
> void (*atomic_print_state)(struct drm_printer *p,
> const struct drm_plane_state *state);
> +
> + /**
> + * @format_mod_supported:
> + *
> + * This optional hook is used for the DRM to determine if the given
> + * format/modifier combination is valid for the plane. This allows the
> + * DRM to generate the correct format bitmask (which formats apply to
> + * which modifier).
> + *
Add a "Returns:" line here in the comment to match the style of the file's comments.
> + * True if the given modifier is valid for that format on the plane.
> + * False otherwise.
> + */
> + bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
> + uint64_t modifier);
> };
>
> /**
> @@ -487,6 +501,9 @@ struct drm_plane {
> unsigned int format_count;
> bool format_default;
>
> + uint64_t *modifiers;
> + unsigned int modifier_count;
> +
> struct drm_crtc *crtc;
> struct drm_framebuffer *fb;
>
> @@ -527,13 +544,14 @@ struct drm_plane {
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>
> -__printf(8, 9)
> +__printf(9, 10)
> int drm_universal_plane_init(struct drm_device *dev,
> struct drm_plane *plane,
> uint32_t possible_crtcs,
> const struct drm_plane_funcs *funcs,
> const uint32_t *formats,
> unsigned int format_count,
> + const uint64_t *format_modifiers,
> enum drm_plane_type type,
> const char *name, ...);
> int drm_plane_init(struct drm_device *dev,
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index 2d36538e4a17..6d9adbb46293 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -122,6 +122,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> struct drm_simple_display_pipe *pipe,
> const struct drm_simple_display_pipe_funcs *funcs,
> const uint32_t *formats, unsigned int format_count,
> + const uint64_t *format_modifiers,
> struct drm_connector *connector);
>
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 55e301047b3e..47f0175e29c0 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -184,6 +184,8 @@ extern "C" {
> #define DRM_FORMAT_MOD_VENDOR_VIVANTE 0x06
> /* add more to the end as needed */
>
> +#define DRM_FORMAT_RESERVED ((1ULL << 56) - 1)
> +
> #define fourcc_mod_code(vendor, val) \
> ((((__u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffULL))
>
> @@ -196,6 +198,15 @@ extern "C" {
> */
>
> /*
> + * Invalid Modifier
> + *
> + * This modifier can be used as a sentinel to terminate list, or to initialize a
s/terminate list/terminate the format modifiers list/
> + * variable with an invalid modifier. It might also be used to report an error
> + * back to userspace for certain APIs.
> + */
> +#define DRM_FORMAT_MOD_INVALID fourcc_mod_code(NONE, DRM_FORMAT_RESERVED)
> +
> +/*
> * Linear Layout
> *
> * Just plain linear layout. Note that this is different from no specifying any
> --
> 2.13.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] drm: Plumb modifiers through plane init
2017-05-17 10:17 ` [PATCH v3 1/3] " Liviu Dudau
@ 2017-05-18 0:26 ` Ben Widawsky
2017-05-18 8:45 ` Liviu Dudau
0 siblings, 1 reply; 16+ messages in thread
From: Ben Widawsky @ 2017-05-18 0:26 UTC (permalink / raw)
To: Liviu Dudau; +Cc: Intel GFX, Daniel Stone, DRI Development
On 17-05-17 11:17:57, Liviu Dudau wrote:
>On Tue, May 16, 2017 at 02:31:24PM -0700, Ben Widawsky wrote:
>> This is the plumbing for supporting fb modifiers on planes. Modifiers
>> have already been introduced to some extent, but this series will extend
>> this to allow querying modifiers per plane. Based on this, the client to
>> enable optimal modifications for framebuffers.
>>
>> This patch simply allows the DRM drivers to initialize their list of
>> supported modifiers upon initializing the plane.
>>
>> v2: A minor addition from Daniel
>>
>> v3: Updated commit message
>> s/INVALID/DRM_FORMAT_MOD_INVALID (Liviu)
>> Remove some excess newlines (Liviu)
>> Update comment for > 64 modifiers (Liviu)
>>
>> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
>> Reviewed-by: Daniel Stone <daniels@collabora.com> (v2)
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
>Minor nits, see below, but otherwise:
>
>Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
>
>Thanks,
>Liviu
>
Thank you. I took them all but the memcpy change, which I'm pretty sure is okay.
Take a quick look again and let me know.
>> ---
>> drivers/gpu/drm/arc/arcpgu_crtc.c | 1 +
>> drivers/gpu/drm/arm/hdlcd_crtc.c | 1 +
>> drivers/gpu/drm/arm/malidp_planes.c | 2 +-
>> drivers/gpu/drm/armada/armada_crtc.c | 1 +
>> drivers/gpu/drm/armada/armada_overlay.c | 1 +
>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 4 ++-
>> drivers/gpu/drm/drm_modeset_helper.c | 1 +
>> drivers/gpu/drm/drm_plane.c | 35 ++++++++++++++++++++++++-
>> drivers/gpu/drm/drm_simple_kms_helper.c | 3 +++
>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 +-
>> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 2 +-
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 1 +
>> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 5 +++-
>> drivers/gpu/drm/i915/intel_sprite.c | 4 +--
>> drivers/gpu/drm/imx/ipuv3-plane.c | 4 +--
>> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
>> drivers/gpu/drm/meson/meson_plane.c | 1 +
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 +--
>> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
>> drivers/gpu/drm/nouveau/nv50_display.c | 5 ++--
>> drivers/gpu/drm/omapdrm/omap_plane.c | 3 ++-
>> drivers/gpu/drm/qxl/qxl_display.c | 2 +-
>> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +--
>> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +--
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 +--
>> drivers/gpu/drm/sti/sti_cursor.c | 2 +-
>> drivers/gpu/drm/sti/sti_gdp.c | 2 +-
>> drivers/gpu/drm/sti/sti_hqvdp.c | 2 +-
>> drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
>> drivers/gpu/drm/tegra/dc.c | 12 ++++-----
>> drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
>> drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
>> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4 +--
>> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 +--
>> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 +--
>> drivers/gpu/drm/zte/zx_plane.c | 2 +-
>> include/drm/drm_plane.h | 20 +++++++++++++-
>> include/drm/drm_simple_kms_helper.h | 1 +
>> include/uapi/drm/drm_fourcc.h | 11 ++++++++
>> 41 files changed, 126 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
>> index ad9a95916f1f..cd8a24c7c67d 100644
>> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
>> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
>> @@ -218,6 +218,7 @@ static struct drm_plane *arc_pgu_plane_init(struct drm_device *drm)
>>
>> ret = drm_universal_plane_init(drm, plane, 0xff, &arc_pgu_plane_funcs,
>> formats, ARRAY_SIZE(formats),
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret)
>> return ERR_PTR(ret);
>> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> index 798a3cc480a2..0caa03ae8708 100644
>> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
>> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> @@ -303,6 +303,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>>
>> ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
>> formats, ARRAY_SIZE(formats),
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> devm_kfree(drm->dev, plane);
>> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
>> index 814fda23cead..b156610c68a5 100644
>> --- a/drivers/gpu/drm/arm/malidp_planes.c
>> +++ b/drivers/gpu/drm/arm/malidp_planes.c
>> @@ -400,7 +400,7 @@ int malidp_de_planes_init(struct drm_device *drm)
>> DRM_PLANE_TYPE_OVERLAY;
>> ret = drm_universal_plane_init(drm, &plane->base, crtcs,
>> &malidp_de_plane_funcs, formats,
>> - n, plane_type, NULL);
>> + n, NULL, plane_type, NULL);
>> if (ret < 0)
>> goto cleanup;
>>
>> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
>> index 4fe19fde84f9..ea48ef88f0e4 100644
>> --- a/drivers/gpu/drm/armada/armada_crtc.c
>> +++ b/drivers/gpu/drm/armada/armada_crtc.c
>> @@ -1269,6 +1269,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>> &armada_primary_plane_funcs,
>> armada_primary_formats,
>> ARRAY_SIZE(armada_primary_formats),
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> kfree(primary);
>> diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c
>> index 424e465ff407..0054144287ae 100644
>> --- a/drivers/gpu/drm/armada/armada_overlay.c
>> +++ b/drivers/gpu/drm/armada/armada_overlay.c
>> @@ -460,6 +460,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
>> &armada_ovl_plane_funcs,
>> armada_ovl_formats,
>> ARRAY_SIZE(armada_ovl_formats),
>> + NULL,
>> DRM_PLANE_TYPE_OVERLAY, NULL);
>> if (ret) {
>> kfree(dplane);
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> index 29cc10d053eb..b5c6cf2d8c36 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> @@ -1058,7 +1058,9 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev,
>> ret = drm_universal_plane_init(dev, &plane->base, 0,
>> &layer_plane_funcs,
>> desc->formats->formats,
>> - desc->formats->nformats, type, NULL);
>> + desc->formats->nformats,
>> + NULL,
>> + type, NULL);
>> if (ret)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
>> index 2b33825f2f93..9cb1eede0b4d 100644
>> --- a/drivers/gpu/drm/drm_modeset_helper.c
>> +++ b/drivers/gpu/drm/drm_modeset_helper.c
>> @@ -124,6 +124,7 @@ static struct drm_plane *create_primary_plane(struct drm_device *dev)
>> &drm_primary_helper_funcs,
>> safe_modeset_formats,
>> ARRAY_SIZE(safe_modeset_formats),
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> kfree(primary);
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index fedd4d60d9cd..20203871e06d 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -70,6 +70,8 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>> * @funcs: callbacks for the new plane
>> * @formats: array of supported formats (DRM_FORMAT\_\*)
>> * @format_count: number of elements in @formats
>> + * @format_modifiers: array of struct drm_format modifiers terminated by
>> + * DRM_FORMAT_MOD_INVALID
>> * @type: type of plane (overlay, primary, cursor)
>> * @name: printf style format string for the plane name, or NULL for default name
>> *
>> @@ -82,10 +84,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> uint32_t possible_crtcs,
>> const struct drm_plane_funcs *funcs,
>> const uint32_t *formats, unsigned int format_count,
>> + const uint64_t *format_modifiers,
>> enum drm_plane_type type,
>> const char *name, ...)
>> {
>> struct drm_mode_config *config = &dev->mode_config;
>> + unsigned int format_modifier_count = 0;
>> int ret;
>>
>> ret = drm_mode_object_add(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
>> @@ -105,6 +109,30 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> return -ENOMEM;
>> }
>>
>> + /* First driver to need more than 64 formats needs to fix this. Each
>
>Kernel's style asks for multi-line comments to start with an empty /* line
>
Okay, fine. I'm not the only one doing this in this file :-)
>> + * format is encoded as a bit and the current code only supports a u64.
>> + */
>> + if (WARN_ON(format_count > 64))
>> + return -EINVAL;
>> +
>> + if (format_modifiers) {
>> + const uint64_t *temp_modifiers = format_modifiers;
>> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>> + format_modifier_count++;
>> + }
>> +
>> + plane->modifier_count = format_modifier_count;
>> + plane->modifiers = kmalloc_array(format_modifier_count,
>> + sizeof(format_modifiers[0]),
>> + GFP_KERNEL);
>> +
>> + if (format_modifier_count && !plane->modifiers) {
>> + DRM_DEBUG_KMS("out of memory when allocating plane\n");
>> + kfree(plane->format_types);
>> + drm_mode_object_unregister(dev, &plane->base);
>> + return -ENOMEM;
>> + }
>> +
>> if (name) {
>> va_list ap;
>>
>> @@ -117,12 +145,15 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> }
>> if (!plane->name) {
>> kfree(plane->format_types);
>> + kfree(plane->modifiers);
>> drm_mode_object_unregister(dev, &plane->base);
>> return -ENOMEM;
>> }
>>
>> memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
>> plane->format_count = format_count;
>> + memcpy(plane->modifiers, format_modifiers,
>> + format_modifier_count * sizeof(format_modifiers[0]));
>
>I'm still worried that we can reach the memcpy with a NULL format_modifiers and we are opening
>a security hole here.
>
I didn't notice your feedback here before, I apologize. I'm pretty certain you
can't get here with !format_modifiers (well you can, but then the 'n' for memcpy
will be 0). format_modifier_count is only !0 if format_modifiers is !NULL.
>> plane->possible_crtcs = possible_crtcs;
>> plane->type = type;
>>
>> @@ -205,7 +236,8 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>
>> type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
>> return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
>> - formats, format_count, type, NULL);
>> + formats, format_count,
>> + NULL, type, NULL);
>> }
>> EXPORT_SYMBOL(drm_plane_init);
>>
>> @@ -224,6 +256,7 @@ void drm_plane_cleanup(struct drm_plane *plane)
>> drm_modeset_lock_fini(&plane->mutex);
>>
>> kfree(plane->format_types);
>> + kfree(plane->modifiers);
>> drm_mode_object_unregister(dev, &plane->base);
>>
>> BUG_ON(list_empty(&plane->head));
>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>> index e084f9f8ca66..949f0ab0e267 100644
>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>> @@ -193,6 +193,7 @@ EXPORT_SYMBOL(drm_simple_display_pipe_attach_bridge);
>> * @funcs: callbacks for the display pipe (optional)
>> * @formats: array of supported formats (DRM_FORMAT\_\*)
>> * @format_count: number of elements in @formats
>> + * @format_modifiers: array of formats modifiers
>> * @connector: connector to attach and register (optional)
>> *
>> * Sets up a display pipeline which consist of a really simple
>> @@ -213,6 +214,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>> struct drm_simple_display_pipe *pipe,
>> const struct drm_simple_display_pipe_funcs *funcs,
>> const uint32_t *formats, unsigned int format_count,
>> + const uint64_t *format_modifiers,
>> struct drm_connector *connector)
>> {
>> struct drm_encoder *encoder = &pipe->encoder;
>> @@ -227,6 +229,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>> ret = drm_universal_plane_init(dev, plane, 0,
>> &drm_simple_kms_plane_funcs,
>> formats, format_count,
>> + format_modifiers,
>> DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret)
>> return ret;
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index c2f17f30afab..75d4928dd196 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -284,7 +284,7 @@ int exynos_plane_init(struct drm_device *dev,
>> &exynos_plane_funcs,
>> config->pixel_formats,
>> config->num_pixel_formats,
>> - config->type, NULL);
>> + NULL, config->type, NULL);
>> if (err) {
>> DRM_ERROR("failed to initialize plane\n");
>> return err;
>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
>> index 0a20723aa6e1..9554b245746e 100644
>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
>> @@ -224,7 +224,7 @@ struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>> &fsl_dcu_drm_plane_funcs,
>> fsl_dcu_drm_plane_formats,
>> ARRAY_SIZE(fsl_dcu_drm_plane_formats),
>> - DRM_PLANE_TYPE_PRIMARY, NULL);
>> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> kfree(primary);
>> primary = NULL;
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> index 59542bddc980..339e914cbaa3 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -181,6 +181,7 @@ static struct drm_plane *hibmc_plane_init(struct hibmc_drm_private *priv)
>> ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
>> channel_formats1,
>> ARRAY_SIZE(channel_formats1),
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY,
>> NULL);
>> if (ret) {
>> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
>> index c96c228a9898..1acb8af12246 100644
>> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
>> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
>> @@ -909,7 +909,7 @@ static int ade_plane_init(struct drm_device *dev, struct ade_plane *aplane,
>> return ret;
>>
>> ret = drm_universal_plane_init(dev, &aplane->base, 1, &ade_plane_funcs,
>> - fmts, fmts_cnt, type, NULL);
>> + fmts, fmts_cnt, NULL, type, NULL);
>> if (ret) {
>> DRM_ERROR("fail to init plane, ch=%d\n", aplane->ch);
>> return ret;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3617927af269..9dd67d51e7c9 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13594,18 +13594,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>> ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
>> 0, &intel_plane_funcs,
>> intel_primary_formats, num_formats,
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY,
>> "plane 1%c", pipe_name(pipe));
>> else if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
>> ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
>> 0, &intel_plane_funcs,
>> intel_primary_formats, num_formats,
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY,
>> "primary %c", pipe_name(pipe));
>> else
>> ret = drm_universal_plane_init(&dev_priv->drm, &primary->base,
>> 0, &intel_plane_funcs,
>> intel_primary_formats, num_formats,
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY,
>> "plane %c", plane_name(primary->plane));
>> if (ret)
>> @@ -13776,7 +13779,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>> 0, &intel_cursor_plane_funcs,
>> intel_cursor_formats,
>> ARRAY_SIZE(intel_cursor_formats),
>> - DRM_PLANE_TYPE_CURSOR,
>> + NULL, DRM_PLANE_TYPE_CURSOR,
>> "cursor %c", pipe_name(pipe));
>> if (ret)
>> goto fail;
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index f7d431427115..053802dd08c1 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -1165,13 +1165,13 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>> ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
>> possible_crtcs, &intel_plane_funcs,
>> plane_formats, num_plane_formats,
>> - DRM_PLANE_TYPE_OVERLAY,
>> + NULL, DRM_PLANE_TYPE_OVERLAY,
>> "plane %d%c", plane + 2, pipe_name(pipe));
>> else
>> ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
>> possible_crtcs, &intel_plane_funcs,
>> plane_formats, num_plane_formats,
>> - DRM_PLANE_TYPE_OVERLAY,
>> + NULL, DRM_PLANE_TYPE_OVERLAY,
>> "sprite %c", sprite_name(pipe, plane));
>> if (ret)
>> goto fail;
>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> index d63e853a0300..6c708c3b1cdc 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> @@ -718,8 +718,8 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
>>
>> ret = drm_universal_plane_init(dev, &ipu_plane->base, possible_crtcs,
>> &ipu_plane_funcs, ipu_plane_formats,
>> - ARRAY_SIZE(ipu_plane_formats), type,
>> - NULL);
>> + ARRAY_SIZE(ipu_plane_formats),
>> + NULL, type, NULL);
>> if (ret) {
>> DRM_ERROR("failed to initialize plane\n");
>> kfree(ipu_plane);
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
>> index e405e89ed5e5..bec6d14dd070 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
>> @@ -173,7 +173,7 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>
>> err = drm_universal_plane_init(dev, plane, possible_crtcs,
>> &mtk_plane_funcs, formats,
>> - ARRAY_SIZE(formats), type, NULL);
>> + ARRAY_SIZE(formats), NULL, type, NULL);
>> if (err) {
>> DRM_ERROR("failed to initialize plane\n");
>> return err;
>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
>> index a32d3b6e2e12..17e96fa47868 100644
>> --- a/drivers/gpu/drm/meson/meson_plane.c
>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>> @@ -223,6 +223,7 @@ int meson_plane_create(struct meson_drm *priv)
>> &meson_plane_funcs,
>> supported_drm_formats,
>> ARRAY_SIZE(supported_drm_formats),
>> + NULL,
>> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>>
>> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
>> index 53619d07677e..8f3417e45d4e 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
>> @@ -398,7 +398,7 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
>> type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
>> ret = drm_universal_plane_init(dev, plane, 0xff, &mdp4_plane_funcs,
>> mdp4_plane->formats, mdp4_plane->nformats,
>> - type, NULL);
>> + NULL, type, NULL);
>> if (ret)
>> goto fail;
>>
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> index a38c5fe6cc19..a815bc0e78d1 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> @@ -1140,12 +1140,12 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
>> ret = drm_universal_plane_init(dev, plane, 0xff,
>> &mdp5_cursor_plane_funcs,
>> mdp5_plane->formats, mdp5_plane->nformats,
>> - type, NULL);
>> + NULL, type, NULL);
>> else
>> ret = drm_universal_plane_init(dev, plane, 0xff,
>> &mdp5_plane_funcs,
>> mdp5_plane->formats, mdp5_plane->nformats,
>> - type, NULL);
>> + NULL, type, NULL);
>> if (ret)
>> goto fail;
>>
>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
>> index d1b9c34c7c00..3ee3784a54f4 100644
>> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
>> @@ -190,7 +190,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
>> }
>>
>> ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs,
>> - mxsfb_formats, ARRAY_SIZE(mxsfb_formats),
>> + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, 0,
>> &mxsfb->connector);
>> if (ret < 0) {
>> dev_err(drm->dev, "Cannot setup simple display pipe\n");
>> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
>> index 0e58537352fe..dde71be463f8 100644
>> --- a/drivers/gpu/drm/nouveau/nv50_display.c
>> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
>> @@ -1084,8 +1084,9 @@ nv50_wndw_ctor(const struct nv50_wndw_func *func, struct drm_device *dev,
>> wndw->func = func;
>> wndw->dmac = dmac;
>>
>> - ret = drm_universal_plane_init(dev, &wndw->plane, 0, &nv50_wndw, format,
>> - nformat, type, "%s-%d", name, index);
>> + ret = drm_universal_plane_init(dev, &wndw->plane, 0, &nv50_wndw,
>> + format, nformat, NULL,
>> + type, "%s-%d", name, index);
>> if (ret)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index 9168154d749e..9b8341d77468 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -370,7 +370,8 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>>
>> ret = drm_universal_plane_init(dev, plane, possible_crtcs,
>> &omap_plane_funcs, omap_plane->formats,
>> - omap_plane->nformats, type, NULL);
>> + omap_plane->nformats,
>> + NULL, type, NULL);
>> if (ret < 0)
>> goto error;
>>
>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
>> index 058340a002c2..fcf1d2034449 100644
>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>> @@ -788,7 +788,7 @@ static struct drm_plane *qxl_create_plane(struct qxl_device *qdev,
>>
>> err = drm_universal_plane_init(&qdev->ddev, plane, possible_crtcs,
>> funcs, formats, num_formats,
>> - type, NULL);
>> + NULL, type, NULL);
>> if (err)
>> goto free_plane;
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
>> index dcde6288da6c..2b02eccbfb70 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
>> @@ -743,8 +743,8 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)
>>
>> ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
>> &rcar_du_plane_funcs, formats,
>> - ARRAY_SIZE(formats), type,
>> - NULL);
>> + ARRAY_SIZE(formats),
>> + NULL, type, NULL);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> index b0ff304ce3dc..e0c054f9b57a 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> @@ -368,8 +368,8 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp)
>> 1 << vsp->index,
>> &rcar_du_vsp_plane_funcs,
>> formats_kms,
>> - ARRAY_SIZE(formats_kms), type,
>> - NULL);
>> + ARRAY_SIZE(formats_kms),
>> + NULL, type, NULL);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 3f7a82d1e095..d0cf57de9afc 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -1280,7 +1280,7 @@ static int vop_create_crtc(struct vop *vop)
>> 0, &vop_plane_funcs,
>> win_data->phy->data_formats,
>> win_data->phy->nformats,
>> - win_data->type, NULL);
>> + NULL, win_data->type, NULL);
>> if (ret) {
>> DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
>> ret);
>> @@ -1319,7 +1319,7 @@ static int vop_create_crtc(struct vop *vop)
>> &vop_plane_funcs,
>> win_data->phy->data_formats,
>> win_data->phy->nformats,
>> - win_data->type, NULL);
>> + NULL, win_data->type, NULL);
>> if (ret) {
>> DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
>> ret);
>> diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
>> index cca75bddb9ad..97c25e204bf4 100644
>> --- a/drivers/gpu/drm/sti/sti_cursor.c
>> +++ b/drivers/gpu/drm/sti/sti_cursor.c
>> @@ -393,7 +393,7 @@ struct drm_plane *sti_cursor_create(struct drm_device *drm_dev,
>> &sti_cursor_plane_helpers_funcs,
>> cursor_supported_formats,
>> ARRAY_SIZE(cursor_supported_formats),
>> - DRM_PLANE_TYPE_CURSOR, NULL);
>> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
>> if (res) {
>> DRM_ERROR("Failed to initialize universal plane\n");
>> goto err_plane;
>> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
>> index 88f16cdf6a4b..70391603c12d 100644
>> --- a/drivers/gpu/drm/sti/sti_gdp.c
>> +++ b/drivers/gpu/drm/sti/sti_gdp.c
>> @@ -932,7 +932,7 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
>> &sti_gdp_plane_helpers_funcs,
>> gdp_supported_formats,
>> ARRAY_SIZE(gdp_supported_formats),
>> - type, NULL);
>> + NULL, type, NULL);
>> if (res) {
>> DRM_ERROR("Failed to initialize universal plane\n");
>> goto err;
>> diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c
>> index 66f843148ef7..9a1ff352820d 100644
>> --- a/drivers/gpu/drm/sti/sti_hqvdp.c
>> +++ b/drivers/gpu/drm/sti/sti_hqvdp.c
>> @@ -1296,7 +1296,7 @@ static struct drm_plane *sti_hqvdp_create(struct drm_device *drm_dev,
>> &sti_hqvdp_plane_helpers_funcs,
>> hqvdp_supported_formats,
>> ARRAY_SIZE(hqvdp_supported_formats),
>> - DRM_PLANE_TYPE_OVERLAY, NULL);
>> + NULL, DRM_PLANE_TYPE_OVERLAY, NULL);
>> if (res) {
>> DRM_ERROR("Failed to initialize universal plane\n");
>> return NULL;
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
>> index f26bde5b9117..2d9f8d01ac2e 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
>> @@ -115,7 +115,7 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
>> ret = drm_universal_plane_init(drm, &layer->plane, 0,
>> &sun4i_backend_layer_funcs,
>> plane->formats, plane->nformats,
>> - plane->type, NULL);
>> + NULL, plane->type, NULL);
>> if (ret) {
>> dev_err(drm->dev, "Couldn't initialize layer\n");
>> return ERR_PTR(ret);
>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>> index 95b373f739f2..0380edd054ac 100644
>> --- a/drivers/gpu/drm/tegra/dc.c
>> +++ b/drivers/gpu/drm/tegra/dc.c
>> @@ -655,8 +655,8 @@ static struct drm_plane *tegra_dc_primary_plane_create(struct drm_device *drm,
>>
>> err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
>> &tegra_primary_plane_funcs, formats,
>> - num_formats, DRM_PLANE_TYPE_PRIMARY,
>> - NULL);
>> + num_formats, NULL,
>> + DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (err < 0) {
>> kfree(plane);
>> return ERR_PTR(err);
>> @@ -821,8 +821,8 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
>>
>> err = drm_universal_plane_init(drm, &plane->base, 1 << dc->pipe,
>> &tegra_cursor_plane_funcs, formats,
>> - num_formats, DRM_PLANE_TYPE_CURSOR,
>> - NULL);
>> + num_formats, NULL,
>> + DRM_PLANE_TYPE_CURSOR, NULL);
>> if (err < 0) {
>> kfree(plane);
>> return ERR_PTR(err);
>> @@ -883,8 +883,8 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
>>
>> err = drm_universal_plane_init(drm, &plane->base, 1 << dc->pipe,
>> &tegra_overlay_plane_funcs, formats,
>> - num_formats, DRM_PLANE_TYPE_OVERLAY,
>> - NULL);
>> + num_formats, NULL,
>> + DRM_PLANE_TYPE_OVERLAY, NULL);
>> if (err < 0) {
>> kfree(plane);
>> return ERR_PTR(err);
>> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
>> index d34cd5393a9b..74b5e7afd6e9 100644
>> --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> @@ -861,7 +861,7 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
>> ret = drm_universal_plane_init(dev, plane, 0,
>> &vc4_plane_funcs,
>> formats, num_formats,
>> - type, NULL);
>> + NULL, type, NULL);
>>
>> drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> index adcdbd0abef6..71ba455af915 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> @@ -298,7 +298,7 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
>> ret = drm_universal_plane_init(dev, plane, 1 << index,
>> &virtio_gpu_plane_funcs,
>> formats, nformats,
>> - type, NULL);
>> + NULL, type, NULL);
>> if (ret)
>> goto err_plane_init;
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
>> index d3987bcf53f8..6f0bf6f9c6f8 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
>> @@ -439,7 +439,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
>> 0, &vmw_ldu_plane_funcs,
>> vmw_primary_plane_formats,
>> ARRAY_SIZE(vmw_primary_plane_formats),
>> - DRM_PLANE_TYPE_PRIMARY, NULL);
>> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> DRM_ERROR("Failed to initialize primary plane");
>> goto err_free;
>> @@ -454,7 +454,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
>> 0, &vmw_ldu_cursor_funcs,
>> vmw_cursor_plane_formats,
>> ARRAY_SIZE(vmw_cursor_plane_formats),
>> - DRM_PLANE_TYPE_CURSOR, NULL);
>> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
>> if (ret) {
>> DRM_ERROR("Failed to initialize cursor plane");
>> drm_plane_cleanup(&ldu->base.primary);
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
>> index 8d7dc9def7c2..42780c9cb90f 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
>> @@ -622,7 +622,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
>> 0, &vmw_sou_plane_funcs,
>> vmw_primary_plane_formats,
>> ARRAY_SIZE(vmw_primary_plane_formats),
>> - DRM_PLANE_TYPE_PRIMARY, NULL);
>> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> DRM_ERROR("Failed to initialize primary plane");
>> goto err_free;
>> @@ -637,7 +637,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
>> 0, &vmw_sou_cursor_funcs,
>> vmw_cursor_plane_formats,
>> ARRAY_SIZE(vmw_cursor_plane_formats),
>> - DRM_PLANE_TYPE_CURSOR, NULL);
>> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
>> if (ret) {
>> DRM_ERROR("Failed to initialize cursor plane");
>> drm_plane_cleanup(&sou->base.primary);
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> index bad31bdf09b6..f18fb966ce9c 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> @@ -1456,7 +1456,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>> 0, &vmw_stdu_plane_funcs,
>> vmw_primary_plane_formats,
>> ARRAY_SIZE(vmw_primary_plane_formats),
>> - DRM_PLANE_TYPE_PRIMARY, NULL);
>> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> if (ret) {
>> DRM_ERROR("Failed to initialize primary plane");
>> goto err_free;
>> @@ -1471,7 +1471,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>> 0, &vmw_stdu_cursor_funcs,
>> vmw_cursor_plane_formats,
>> ARRAY_SIZE(vmw_cursor_plane_formats),
>> - DRM_PLANE_TYPE_CURSOR, NULL);
>> + NULL, DRM_PLANE_TYPE_CURSOR, NULL);
>> if (ret) {
>> DRM_ERROR("Failed to initialize cursor plane");
>> drm_plane_cleanup(&stdu->base.primary);
>> diff --git a/drivers/gpu/drm/zte/zx_plane.c b/drivers/gpu/drm/zte/zx_plane.c
>> index d646ac931663..ea29fee01f7d 100644
>> --- a/drivers/gpu/drm/zte/zx_plane.c
>> +++ b/drivers/gpu/drm/zte/zx_plane.c
>> @@ -539,7 +539,7 @@ int zx_plane_init(struct drm_device *drm, struct zx_plane *zplane,
>>
>> ret = drm_universal_plane_init(drm, plane, VOU_CRTC_MASK,
>> &zx_plane_funcs, formats, format_count,
>> - type, NULL);
>> + NULL, type, NULL);
>> if (ret) {
>> DRM_DEV_ERROR(dev, "failed to init universal plane: %d\n", ret);
>> return ret;
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 9ab3e7044812..2db5eed53d7b 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -392,6 +392,20 @@ struct drm_plane_funcs {
>> */
>> void (*atomic_print_state)(struct drm_printer *p,
>> const struct drm_plane_state *state);
>> +
>> + /**
>> + * @format_mod_supported:
>> + *
>> + * This optional hook is used for the DRM to determine if the given
>> + * format/modifier combination is valid for the plane. This allows the
>> + * DRM to generate the correct format bitmask (which formats apply to
>> + * which modifier).
>> + *
>
>Add a "Returns:" line here in the comment to match the style of the file's comments.
>
Got it.
>> + * True if the given modifier is valid for that format on the plane.
>> + * False otherwise.
>> + */
>> + bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
>> + uint64_t modifier);
>> };
>>
>> /**
>> @@ -487,6 +501,9 @@ struct drm_plane {
>> unsigned int format_count;
>> bool format_default;
>>
>> + uint64_t *modifiers;
>> + unsigned int modifier_count;
>> +
>> struct drm_crtc *crtc;
>> struct drm_framebuffer *fb;
>>
>> @@ -527,13 +544,14 @@ struct drm_plane {
>>
>> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>>
>> -__printf(8, 9)
>> +__printf(9, 10)
>> int drm_universal_plane_init(struct drm_device *dev,
>> struct drm_plane *plane,
>> uint32_t possible_crtcs,
>> const struct drm_plane_funcs *funcs,
>> const uint32_t *formats,
>> unsigned int format_count,
>> + const uint64_t *format_modifiers,
>> enum drm_plane_type type,
>> const char *name, ...);
>> int drm_plane_init(struct drm_device *dev,
>> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
>> index 2d36538e4a17..6d9adbb46293 100644
>> --- a/include/drm/drm_simple_kms_helper.h
>> +++ b/include/drm/drm_simple_kms_helper.h
>> @@ -122,6 +122,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>> struct drm_simple_display_pipe *pipe,
>> const struct drm_simple_display_pipe_funcs *funcs,
>> const uint32_t *formats, unsigned int format_count,
>> + const uint64_t *format_modifiers,
>> struct drm_connector *connector);
>>
>> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>> index 55e301047b3e..47f0175e29c0 100644
>> --- a/include/uapi/drm/drm_fourcc.h
>> +++ b/include/uapi/drm/drm_fourcc.h
>> @@ -184,6 +184,8 @@ extern "C" {
>> #define DRM_FORMAT_MOD_VENDOR_VIVANTE 0x06
>> /* add more to the end as needed */
>>
>> +#define DRM_FORMAT_RESERVED ((1ULL << 56) - 1)
>> +
>> #define fourcc_mod_code(vendor, val) \
>> ((((__u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffULL))
>>
>> @@ -196,6 +198,15 @@ extern "C" {
>> */
>>
>> /*
>> + * Invalid Modifier
>> + *
>> + * This modifier can be used as a sentinel to terminate list, or to initialize a
>
>s/terminate list/terminate the format modifiers list/
>
>
Got it.
>
>> + * variable with an invalid modifier. It might also be used to report an error
>> + * back to userspace for certain APIs.
>> + */
>> +#define DRM_FORMAT_MOD_INVALID fourcc_mod_code(NONE, DRM_FORMAT_RESERVED)
>> +
>> +/*
>> * Linear Layout
>> *
>> * Just plain linear layout. Note that this is different from no specifying any
>> --
>> 2.13.0
>>
>
>--
>====================
>| I would like to |
>| fix the world, |
>| but they're not |
>| giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-17 0:06 ` Emil Velikov
@ 2017-05-18 0:46 ` Ben Widawsky
2017-05-18 9:02 ` Ville Syrjälä
2017-05-18 9:49 ` Emil Velikov
0 siblings, 2 replies; 16+ messages in thread
From: Ben Widawsky @ 2017-05-18 0:46 UTC (permalink / raw)
To: Emil Velikov
Cc: Intel GFX, Kristian H . Kristensen, Daniel Stone, DRI Development
On 17-05-17 01:06:16, Emil Velikov wrote:
>Hi Ben,
>
>On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
>> Updated blob layout (Rob, Daniel, Kristian, xerpi)
>>
>> v2:
>> * Removed __packed, and alignment (.+)
>> * Fix indent in drm_format_modifier fields (Liviu)
>> * Remove duplicated modifier > 64 check (Liviu)
>> * Change comment about modifier (Liviu)
>> * Remove arguments to blob creation, use plane instead (Liviu)
>> * Fix data types (Ben)
>> * Make the blob part of uapi (Daniel)
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Daniel Stone <daniels@collabora.com>
>> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>> Cc: Liviu Dudau <liviu@dudau.co.uk>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>
>I think this is almost perfect, barring the UAPI nitpick.
>The rest is somewhat of a bikeshedding.
>
>With the UAPI resolved, regardless of the rest
>Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>
>
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>
>> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
>> +{
>> + const struct drm_mode_config *config = &dev->mode_config;
>> + const uint64_t *temp_modifiers = plane->modifiers;
>> + unsigned int format_modifier_count = 0;
>> + struct drm_property_blob *blob = NULL;
>> + struct drm_format_modifier *mod;
>> + size_t blob_size = 0, formats_size, modifiers_size;
>There's no need to initialize blob and blob_size here.
>
>> + struct drm_format_modifier_blob *blob_data;
>> + int i, j, ret = 0;
>Make i and j unsigned to match format_modifier_count and
>plane->format_count respectively.
>Then expand ret in the only place where it's used?
>
Oh. ret has lost it's utility over the iterations of this patch. Make i and j
unsigned and dropped ret.
>> +
>> + if (plane->modifiers)
>> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>> + format_modifier_count++;
>> +
>> + formats_size = sizeof(*plane->format_types) * plane->format_count;
>> + if (WARN_ON(!formats_size)) {
>> + /* 0 formats are never expected */
>> + return 0;
>> + }
>> +
>> + modifiers_size =
>> + sizeof(struct drm_format_modifier) * format_modifier_count;
>> +
>> + blob_size = sizeof(struct drm_format_modifier_blob);
>> + blob_size += ALIGN(formats_size, 8);
>Worth having the "Modifiers offset is a pointer..." comment moved/copied here?
>
Yes it is.
>> + blob_size += modifiers_size;
>> +
>> + blob = drm_property_create_blob(dev, blob_size, NULL);
>> + if (IS_ERR(blob))
>> + return -1;
>> +
>Maybe propagate the exact error... Hmm we don't seem to check if
>create_in_format_blob fails either so perhaps it's not worth it.
>
>
In this case it's almost definitely ENOMEM. All values should be verified - I
think the other errors are there for when userspace is utilizing blob creation.
So I'll just leave it.
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -665,6 +665,56 @@ struct drm_mode_atomic {
>> __u64 user_data;
>> };
>>
>> +struct drm_format_modifier_blob {
>> +#define FORMAT_BLOB_CURRENT 1
>> + /* Version of this blob format */
>> + u32 version;
>> +
>> + /* Flags */
>> + u32 flags;
>> +
>> + /* Number of fourcc formats supported */
>> + u32 count_formats;
>> +
>> + /* Where in this blob the formats exist (in bytes) */
>> + u32 formats_offset;
>> +
>> + /* Number of drm_format_modifiers */
>> + u32 count_modifiers;
>> +
>> + /* Where in this blob the modifiers exist (in bytes) */
>> + u32 modifiers_offset;
>> +
>> + /* u32 formats[] */
>> + /* struct drm_format_modifier modifiers[] */
>> +};
>> +
>> +struct drm_format_modifier {
>> + /* Bitmask of formats in get_plane format list this info applies to. The
>> + * offset allows a sliding window of which 64 formats (bits).
>> + *
>> + * Some examples:
>> + * In today's world with < 65 formats, and formats 0, and 2 are
>> + * supported
>> + * 0x0000000000000005
>> + * ^-offset = 0, formats = 5
>> + *
>> + * If the number formats grew to 128, and formats 98-102 are
>> + * supported with the modifier:
G>> + *
>> + * 0x0000003c00000000 0000000000000000
>> + * ^
>> + * |__offset = 64, formats = 0x3c00000000
>> + *
>> + */
>> + __u64 formats;
>> + __u32 offset;
>> + __u32 pad;
>> +
>> + /* The modifier that applies to the >get_plane format list bitmask. */
>> + __u64 modifier;
>Please drop the leading __ from the type names in UAPI headers.
>
Many other structures have the __, can you explain why please (this has probably
been beaten to death already; but I don't know)?
>Regards,
>Emil
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 3/3] drm/i915: Add format modifiers for Intel
2017-05-17 0:20 ` Emil Velikov
@ 2017-05-18 1:14 ` Ben Widawsky
2017-05-18 9:55 ` Emil Velikov
0 siblings, 1 reply; 16+ messages in thread
From: Ben Widawsky @ 2017-05-18 1:14 UTC (permalink / raw)
To: Emil Velikov; +Cc: Intel GFX, Kristian H . Kristensen, DRI Development
On 17-05-17 01:20:50, Emil Velikov wrote:
>Hi Ben,
>
>A couple of small questions/suggestions that I hope you find useful.
>Please don't block any of this work based on my comments.
>
>On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
>
>> +static bool intel_primary_plane_format_mod_supported(struct drm_plane *plane,
>> + uint32_t format,
>> + uint64_t modifier)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
>> +
>> + if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
>> + return false;
>> +
>> + if (INTEL_GEN(dev_priv) >= 9)
>> + return skl_mod_supported(format, modifier);
>> + else if (INTEL_GEN(dev_priv) >= 4)
>> + return i965_mod_supported(format, modifier);
>> + else
>> + return i8xx_mod_supported(format, modifier);
>> +
>Nit: if you rewrite this as below, the control flow should be clearer.
>Thus the 'return false;' at the end, will be [more] obvious that it's
>unreachable ;-)
>
> if (INTEL_GEN(dev_priv) >= 9)
> return skl_mod_supported(format, modifier);
>
> if (INTEL_GEN(dev_priv) >= 4)
> return i965_mod_supported(format, modifier);
>
> return i8xx_mod_supported(format, modifier);
>
>
It's a good point, however many other blocks of code do the same thing, and I
think the nature of if/else if/else implies unreachable. I can add
unreachable()... In fact, I just did.
>> + return false;
>> +}
>> +
>
>
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>
>> +static bool intel_sprite_plane_format_mod_supported(struct drm_plane *plane,
>> + uint32_t format,
>> + uint64_t modifier)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
>> +
>> + if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
>> + return false;
>> +
>> + BUG_ON(plane->base.type != DRM_PLANE_TYPE_OVERLAY);
>> +
>> + switch (format) {
>> + case DRM_FORMAT_XBGR2101010:
>> + case DRM_FORMAT_ABGR2101010:
>> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> + return true;
>> + break;
>> + case DRM_FORMAT_RGB565:
>> + case DRM_FORMAT_ABGR8888:
>> + case DRM_FORMAT_ARGB8888:
>> + if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> + return true;
>> + break;
>> + case DRM_FORMAT_XBGR8888:
>> + if (INTEL_GEN(dev_priv) >= 6)
>> + return true;
>> + break;
>> + case DRM_FORMAT_XRGB8888:
>> + case DRM_FORMAT_YUYV:
>> + case DRM_FORMAT_YVYU:
>> + case DRM_FORMAT_UYVY:
>> + case DRM_FORMAT_VYUY:
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +const struct drm_plane_funcs intel_sprite_plane_funcs = {
>> + .update_plane = drm_atomic_helper_update_plane,
>> + .disable_plane = drm_atomic_helper_disable_plane,
>> + .destroy = intel_plane_destroy,
>> + .set_property = drm_atomic_helper_plane_set_property,
>> + .atomic_get_property = intel_plane_atomic_get_property,
>> + .atomic_set_property = intel_plane_atomic_set_property,
>> + .atomic_duplicate_state = intel_plane_duplicate_state,
>> + .atomic_destroy_state = intel_plane_destroy_state,
>> + .format_mod_supported = intel_sprite_plane_format_mod_supported,
>> +};
>> +
>Having a dull moment - is intel_sprite_plane_funcs (+
>intel_sprite_plane_format_mod_supported) unused or I'm seeing things?
>If one is to keep it, for future work, perhaps it's worth adding a 2-3
>word comment,
>
>Regards,
>Emil
Not sure how this happened, it is a mistake. Thanks for spotting it. Here's what
I got locally:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3519c10abcc3..a8f4c5783ec2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13479,7 +13479,7 @@ static bool intel_primary_plane_format_mod_supported(struct drm_plane *plane,
else
return i8xx_mod_supported(format, modifier);
- return false;
+ unreachable();
}
static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index e7c5c0a94bbd..1d582b436b59 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1233,7 +1233,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
if (INTEL_GEN(dev_priv) >= 9)
ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base,
- possible_crtcs, &intel_plane_funcs,
+ possible_crtcs, &intel_sprite_plane_funcs,
plane_formats, num_plane_formats,
modifiers,
DRM_PLANE_TYPE_OVERLAY,
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] drm: Plumb modifiers through plane init
2017-05-18 0:26 ` Ben Widawsky
@ 2017-05-18 8:45 ` Liviu Dudau
0 siblings, 0 replies; 16+ messages in thread
From: Liviu Dudau @ 2017-05-18 8:45 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Daniel Stone, DRI Development
On Wed, May 17, 2017 at 05:26:14PM -0700, Ben Widawsky wrote:
> On 17-05-17 11:17:57, Liviu Dudau wrote:
> > On Tue, May 16, 2017 at 02:31:24PM -0700, Ben Widawsky wrote:
> > > This is the plumbing for supporting fb modifiers on planes. Modifiers
> > > have already been introduced to some extent, but this series will extend
> > > this to allow querying modifiers per plane. Based on this, the client to
> > > enable optimal modifications for framebuffers.
> > >
> > > This patch simply allows the DRM drivers to initialize their list of
> > > supported modifiers upon initializing the plane.
> > >
> > > v2: A minor addition from Daniel
> > >
> > > v3: Updated commit message
> > > s/INVALID/DRM_FORMAT_MOD_INVALID (Liviu)
> > > Remove some excess newlines (Liviu)
> > > Update comment for > 64 modifiers (Liviu)
> > >
> > > Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Reviewed-by: Daniel Stone <daniels@collabora.com> (v2)
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >
> > Minor nits, see below, but otherwise:
> >
> > Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >
> > Thanks,
> > Liviu
> >
>
> Thank you. I took them all but the memcpy change, which I'm pretty sure is okay.
> Take a quick look again and let me know.
[snip]
>
> > > + * format is encoded as a bit and the current code only supports a u64.
> > > + */
> > > + if (WARN_ON(format_count > 64))
> > > + return -EINVAL;
> > > +
> > > + if (format_modifiers) {
> > > + const uint64_t *temp_modifiers = format_modifiers;
> > > + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> > > + format_modifier_count++;
> > > + }
> > > +
> > > + plane->modifier_count = format_modifier_count;
> > > + plane->modifiers = kmalloc_array(format_modifier_count,
> > > + sizeof(format_modifiers[0]),
> > > + GFP_KERNEL);
> > > +
> > > + if (format_modifier_count && !plane->modifiers) {
> > > + DRM_DEBUG_KMS("out of memory when allocating plane\n");
> > > + kfree(plane->format_types);
> > > + drm_mode_object_unregister(dev, &plane->base);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > if (name) {
> > > va_list ap;
> > >
> > > @@ -117,12 +145,15 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> > > }
> > > if (!plane->name) {
> > > kfree(plane->format_types);
> > > + kfree(plane->modifiers);
> > > drm_mode_object_unregister(dev, &plane->base);
> > > return -ENOMEM;
> > > }
> > >
> > > memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
> > > plane->format_count = format_count;
> > > + memcpy(plane->modifiers, format_modifiers,
> > > + format_modifier_count * sizeof(format_modifiers[0]));
> >
> > I'm still worried that we can reach the memcpy with a NULL format_modifiers and we are opening
> > a security hole here.
> >
>
> I didn't notice your feedback here before, I apologize. I'm pretty certain you
> can't get here with !format_modifiers (well you can, but then the 'n' for memcpy
> will be 0). format_modifier_count is only !0 if format_modifiers is !NULL.
That is a valid point. It then makes me ask why we even go through the dance of allocating
a 0 array for plane->modifiers, can we not skip the whole thing around plane->modifiers if
format_modifiers is NULL?
[snip]
> --
> Ben Widawsky, Intel Open Source Technology Center
Thanks for the effort,
Liviu
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-18 0:46 ` [Intel-gfx] " Ben Widawsky
@ 2017-05-18 9:02 ` Ville Syrjälä
2017-05-18 9:49 ` Emil Velikov
1 sibling, 0 replies; 16+ messages in thread
From: Ville Syrjälä @ 2017-05-18 9:02 UTC (permalink / raw)
To: Ben Widawsky
Cc: Kristian H . Kristensen, Intel GFX, Emil Velikov, Daniel Stone,
DRI Development
On Wed, May 17, 2017 at 05:46:18PM -0700, Ben Widawsky wrote:
> On 17-05-17 01:06:16, Emil Velikov wrote:
> >Hi Ben,
> >
> >On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
> >> Updated blob layout (Rob, Daniel, Kristian, xerpi)
> >>
> >> v2:
> >> * Removed __packed, and alignment (.+)
> >> * Fix indent in drm_format_modifier fields (Liviu)
> >> * Remove duplicated modifier > 64 check (Liviu)
> >> * Change comment about modifier (Liviu)
> >> * Remove arguments to blob creation, use plane instead (Liviu)
> >> * Fix data types (Ben)
> >> * Make the blob part of uapi (Daniel)
> >>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Daniel Stone <daniels@collabora.com>
> >> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> >> Cc: Liviu Dudau <liviu@dudau.co.uk>
> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >> Reviewed-by: Daniel Stone <daniels@collabora.com>
> >
> >I think this is almost perfect, barring the UAPI nitpick.
> >The rest is somewhat of a bikeshedding.
> >
> >With the UAPI resolved, regardless of the rest
> >Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> >
> >
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >
> >> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> >> +{
> >> + const struct drm_mode_config *config = &dev->mode_config;
> >> + const uint64_t *temp_modifiers = plane->modifiers;
> >> + unsigned int format_modifier_count = 0;
> >> + struct drm_property_blob *blob = NULL;
> >> + struct drm_format_modifier *mod;
> >> + size_t blob_size = 0, formats_size, modifiers_size;
> >There's no need to initialize blob and blob_size here.
> >
> >> + struct drm_format_modifier_blob *blob_data;
> >> + int i, j, ret = 0;
> >Make i and j unsigned to match format_modifier_count and
> >plane->format_count respectively.
> >Then expand ret in the only place where it's used?
> >
>
> Oh. ret has lost it's utility over the iterations of this patch. Make i and j
> unsigned and dropped ret.
Unsigned loop iterators will likely bite someone some day, especially
if they're called something like 'i', 'j', etc. IMO it's best to keep
them signed.
>
> >> +
> >> + if (plane->modifiers)
> >> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> >> + format_modifier_count++;
> >> +
> >> + formats_size = sizeof(*plane->format_types) * plane->format_count;
> >> + if (WARN_ON(!formats_size)) {
> >> + /* 0 formats are never expected */
> >> + return 0;
> >> + }
> >> +
> >> + modifiers_size =
> >> + sizeof(struct drm_format_modifier) * format_modifier_count;
> >> +
> >> + blob_size = sizeof(struct drm_format_modifier_blob);
> >> + blob_size += ALIGN(formats_size, 8);
> >Worth having the "Modifiers offset is a pointer..." comment moved/copied here?
> >
>
> Yes it is.
>
> >> + blob_size += modifiers_size;
> >> +
> >> + blob = drm_property_create_blob(dev, blob_size, NULL);
> >> + if (IS_ERR(blob))
> >> + return -1;
> >> +
> >Maybe propagate the exact error... Hmm we don't seem to check if
> >create_in_format_blob fails either so perhaps it's not worth it.
> >
> >
>
> In this case it's almost definitely ENOMEM. All values should be verified - I
> think the other errors are there for when userspace is utilizing blob creation.
>
> So I'll just leave it.
>
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -665,6 +665,56 @@ struct drm_mode_atomic {
> >> __u64 user_data;
> >> };
> >>
> >> +struct drm_format_modifier_blob {
> >> +#define FORMAT_BLOB_CURRENT 1
> >> + /* Version of this blob format */
> >> + u32 version;
> >> +
> >> + /* Flags */
> >> + u32 flags;
> >> +
> >> + /* Number of fourcc formats supported */
> >> + u32 count_formats;
> >> +
> >> + /* Where in this blob the formats exist (in bytes) */
> >> + u32 formats_offset;
> >> +
> >> + /* Number of drm_format_modifiers */
> >> + u32 count_modifiers;
> >> +
> >> + /* Where in this blob the modifiers exist (in bytes) */
> >> + u32 modifiers_offset;
> >> +
> >> + /* u32 formats[] */
> >> + /* struct drm_format_modifier modifiers[] */
> >> +};
> >> +
> >> +struct drm_format_modifier {
> >> + /* Bitmask of formats in get_plane format list this info applies to. The
> >> + * offset allows a sliding window of which 64 formats (bits).
> >> + *
> >> + * Some examples:
> >> + * In today's world with < 65 formats, and formats 0, and 2 are
> >> + * supported
> >> + * 0x0000000000000005
> >> + * ^-offset = 0, formats = 5
> >> + *
> >> + * If the number formats grew to 128, and formats 98-102 are
> >> + * supported with the modifier:
> G>> + *
> >> + * 0x0000003c00000000 0000000000000000
> >> + * ^
> >> + * |__offset = 64, formats = 0x3c00000000
> >> + *
> >> + */
> >> + __u64 formats;
> >> + __u32 offset;
> >> + __u32 pad;
> >> +
> >> + /* The modifier that applies to the >get_plane format list bitmask. */
> >> + __u64 modifier;
> >Please drop the leading __ from the type names in UAPI headers.
> >
>
> Many other structures have the __, can you explain why please (this has probably
> been beaten to death already; but I don't know)?
__ is the rigth choice for uapi headers, unless the rules have changed
recently.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-18 0:46 ` [Intel-gfx] " Ben Widawsky
2017-05-18 9:02 ` Ville Syrjälä
@ 2017-05-18 9:49 ` Emil Velikov
1 sibling, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2017-05-18 9:49 UTC (permalink / raw)
To: Ben Widawsky
Cc: Intel GFX, Kristian H . Kristensen, Daniel Stone, DRI Development
On 18 May 2017 at 01:46, Ben Widawsky <ben@bwidawsk.net> wrote:
>>> + blob_size += modifiers_size;
>>> +
>>> + blob = drm_property_create_blob(dev, blob_size, NULL);
>>> + if (IS_ERR(blob))
>>> + return -1;
>>> +
>>
>> Maybe propagate the exact error... Hmm we don't seem to check if
>> create_in_format_blob fails either so perhaps it's not worth it.
>>
>>
>
> In this case it's almost definitely ENOMEM. All values should be verified -
> I
> think the other errors are there for when userspace is utilizing blob
> creation.
>
> So I'll just leave it.
>
Ack, sure thing.
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -665,6 +665,56 @@ struct drm_mode_atomic {
>>> __u64 user_data;
>>> };
>>>
>>> +struct drm_format_modifier_blob {
>>> +#define FORMAT_BLOB_CURRENT 1
>>> + /* Version of this blob format */
>>> + u32 version;
>>> +
>>> + /* Flags */
>>> + u32 flags;
>>> +
>>> + /* Number of fourcc formats supported */
>>> + u32 count_formats;
>>> +
>>> + /* Where in this blob the formats exist (in bytes) */
>>> + u32 formats_offset;
>>> +
>>> + /* Number of drm_format_modifiers */
>>> + u32 count_modifiers;
>>> +
>>> + /* Where in this blob the modifiers exist (in bytes) */
>>> + u32 modifiers_offset;
>>> +
>>> + /* u32 formats[] */
>>> + /* struct drm_format_modifier modifiers[] */
>>> +};
>>> +
>>> +struct drm_format_modifier {
>>> + /* Bitmask of formats in get_plane format list this info applies
>>> to. The
>>> + * offset allows a sliding window of which 64 formats (bits).
>>> + *
>>> + * Some examples:
>>> + * In today's world with < 65 formats, and formats 0, and 2 are
>>> + * supported
>>> + * 0x0000000000000005
>>> + * ^-offset = 0, formats = 5
>>> + *
>>> + * If the number formats grew to 128, and formats 98-102 are
>>> + * supported with the modifier:
>
> G>> + *
>>>
>>> + * 0x0000003c00000000 0000000000000000
>>> + * ^
>>> + * |__offset = 64, formats = 0x3c00000000
>>> + *
>>> + */
>>> + __u64 formats;
>>> + __u32 offset;
>>> + __u32 pad;
>>> +
>>> + /* The modifier that applies to the >get_plane format list
>>> bitmask. */
>>> + __u64 modifier;
>>
>> Please drop the leading __ from the type names in UAPI headers.
>>
>
> Many other structures have the __, can you explain why please (this has
> probably
> been beaten to death already; but I don't know)?
>
Got confused there for a moment, apologies.
Using the __ types is correct. It is drm_format_modifier_blob that
should be updated to follow suit.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 3/3] drm/i915: Add format modifiers for Intel
2017-05-18 1:14 ` Ben Widawsky
@ 2017-05-18 9:55 ` Emil Velikov
0 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2017-05-18 9:55 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Kristian H . Kristensen, DRI Development
On 18 May 2017 at 02:14, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 17-05-17 01:20:50, Emil Velikov wrote:
>>
>> Hi Ben,
>>
>> A couple of small questions/suggestions that I hope you find useful.
>> Please don't block any of this work based on my comments.
>>
>> On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
>>
>>> +static bool intel_primary_plane_format_mod_supported(struct drm_plane
>>> *plane,
>>> + uint32_t format,
>>> + uint64_t modifier)
>>> +{
>>> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
>>> +
>>> + if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
>>> + return false;
>>> +
>>> + if (INTEL_GEN(dev_priv) >= 9)
>>> + return skl_mod_supported(format, modifier);
>>> + else if (INTEL_GEN(dev_priv) >= 4)
>>> + return i965_mod_supported(format, modifier);
>>> + else
>>> + return i8xx_mod_supported(format, modifier);
>>> +
>>
>> Nit: if you rewrite this as below, the control flow should be clearer.
>> Thus the 'return false;' at the end, will be [more] obvious that it's
>> unreachable ;-)
>>
>> if (INTEL_GEN(dev_priv) >= 9)
>> return skl_mod_supported(format, modifier);
>>
>> if (INTEL_GEN(dev_priv) >= 4)
>> return i965_mod_supported(format, modifier);
>>
>> return i8xx_mod_supported(format, modifier);
>>
>>
>
> It's a good point, however many other blocks of code do the same thing, and
> I
> think the nature of if/else if/else implies unreachable. I can add
> unreachable()... In fact, I just did.
>
>
The "implies" argument is a bit odd, but that's just bikesheding, so
feel free to ignore me.
>>> + return false;
>>> +}
>>> +
>>
>>
>>
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>
>>
>>> +static bool intel_sprite_plane_format_mod_supported(struct drm_plane
>>> *plane,
>>> + uint32_t format,
>>> + uint64_t modifier)
>>> +{
>>> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
>>> +
>>> + if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
>>> + return false;
>>> +
>>> + BUG_ON(plane->base.type != DRM_PLANE_TYPE_OVERLAY);
>>> +
>>> + switch (format) {
>>> + case DRM_FORMAT_XBGR2101010:
>>> + case DRM_FORMAT_ABGR2101010:
>>> + if (IS_VALLEYVIEW(dev_priv) ||
>>> IS_CHERRYVIEW(dev_priv))
>>> + return true;
>>> + break;
>>> + case DRM_FORMAT_RGB565:
>>> + case DRM_FORMAT_ABGR8888:
>>> + case DRM_FORMAT_ARGB8888:
>>> + if (INTEL_GEN(dev_priv) >= 9 ||
>>> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>> + return true;
>>> + break;
>>> + case DRM_FORMAT_XBGR8888:
>>> + if (INTEL_GEN(dev_priv) >= 6)
>>> + return true;
>>> + break;
>>> + case DRM_FORMAT_XRGB8888:
>>> + case DRM_FORMAT_YUYV:
>>> + case DRM_FORMAT_YVYU:
>>> + case DRM_FORMAT_UYVY:
>>> + case DRM_FORMAT_VYUY:
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> +const struct drm_plane_funcs intel_sprite_plane_funcs = {
>>> + .update_plane = drm_atomic_helper_update_plane,
>>> + .disable_plane = drm_atomic_helper_disable_plane,
>>> + .destroy = intel_plane_destroy,
>>> + .set_property = drm_atomic_helper_plane_set_property,
>>> + .atomic_get_property = intel_plane_atomic_get_property,
>>> + .atomic_set_property = intel_plane_atomic_set_property,
>>> + .atomic_duplicate_state = intel_plane_duplicate_state,
>>> + .atomic_destroy_state = intel_plane_destroy_state,
>>> + .format_mod_supported = intel_sprite_plane_format_mod_supported,
>>> +};
>>> +
>>
>> Having a dull moment - is intel_sprite_plane_funcs (+
>> intel_sprite_plane_format_mod_supported) unused or I'm seeing things?
>> If one is to keep it, for future work, perhaps it's worth adding a 2-3
>> word comment,
>>
>> Regards,
>> Emil
>
>
> Not sure how this happened, it is a mistake.
I've spotted an actual bug alongside my bikeshedding, Yay!
Fwiw
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/3] drm: Create a format/modifier blob
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
2017-05-16 23:24 ` Liviu Dudau
2017-05-17 0:06 ` Emil Velikov
@ 2017-05-23 16:39 ` Daniel Stone
2 siblings, 0 replies; 16+ messages in thread
From: Daniel Stone @ 2017-05-23 16:39 UTC (permalink / raw)
To: Ben Widawsky
Cc: Intel GFX, Kristian H . Kristensen, Daniel Stone, DRI Development
[-- Attachment #1: Type: text/plain, Size: 401 bytes --]
Hi Ben,
On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
If you take the attached fixup, this is:
Reviewed-by: Daniel Stone <daniels@collabora.com>
The rest of the series looks fine to me, so you can take my Acked-by,
but I'm currently off work sick and am away next week, so please don't
block on me for merging.
Cheers,
Daniel
[-- Attachment #2: fixup-bwidawsk-in_formats-blob.diff --]
[-- Type: text/plain, Size: 2339 bytes --]
commit 250f3b2c2a824516b18fe21623ddc86ccf31eddb
Author: Daniel Stone <daniels@collabora.com>
Date: Tue May 23 17:36:55 2017 +0100
fixup! drm: Create a format/modifier blob
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index f5b032b5f761..083231750583 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -77,18 +77,12 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
{
const struct drm_mode_config *config = &dev->mode_config;
- const uint64_t *temp_modifiers = plane->modifiers;
- unsigned int format_modifier_count = 0;
struct drm_property_blob *blob = NULL;
struct drm_format_modifier *mod;
size_t blob_size = 0, formats_size, modifiers_size;
struct drm_format_modifier_blob *blob_data;
int i, j, ret = 0;
- if (plane->modifiers)
- while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
- format_modifier_count++;
-
formats_size = sizeof(*plane->format_types) * plane->format_count;
if (WARN_ON(!formats_size)) {
/* 0 formats are never expected */
@@ -96,7 +90,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
}
modifiers_size =
- sizeof(struct drm_format_modifier) * format_modifier_count;
+ sizeof(struct drm_format_modifier) * plane->modifier_count;
blob_size = sizeof(struct drm_format_modifier_blob);
blob_size += ALIGN(formats_size, 8);
@@ -110,7 +104,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
blob_data->version = FORMAT_BLOB_CURRENT;
blob_data->count_formats = plane->format_count;
blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
- blob_data->count_modifiers = format_modifier_count;
+ blob_data->count_modifiers = plane->modifier_count;
/* Modifiers offset is a pointer to a struct with a 64 bit field so it
* should be naturally aligned to 8B.
@@ -125,7 +119,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
goto done;
mod = modifiers_ptr(blob_data);
- for (i = 0; i < format_modifier_count; i++) {
+ for (i = 0; i < plane->modifier_count; i++) {
for (j = 0; j < plane->format_count; j++) {
if (plane->funcs->format_mod_supported(plane,
plane->format_types[j],
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-05-23 16:39 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-16 21:31 [PATCH v3 1/3] drm: Plumb modifiers through plane init Ben Widawsky
2017-05-16 21:31 ` [PATCH v2 2/3] drm: Create a format/modifier blob Ben Widawsky
2017-05-16 23:24 ` Liviu Dudau
2017-05-17 0:06 ` Emil Velikov
2017-05-18 0:46 ` [Intel-gfx] " Ben Widawsky
2017-05-18 9:02 ` Ville Syrjälä
2017-05-18 9:49 ` Emil Velikov
2017-05-23 16:39 ` Daniel Stone
2017-05-16 21:31 ` [PATCH v5 3/3] drm/i915: Add format modifiers for Intel Ben Widawsky
2017-05-17 0:20 ` Emil Velikov
2017-05-18 1:14 ` Ben Widawsky
2017-05-18 9:55 ` Emil Velikov
2017-05-16 21:51 ` ✗ Fi.CI.BAT: failure for series starting with [v3,1/3] drm: Plumb modifiers through plane init Patchwork
2017-05-17 10:17 ` [PATCH v3 1/3] " Liviu Dudau
2017-05-18 0:26 ` Ben Widawsky
2017-05-18 8:45 ` Liviu Dudau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.