* [PATCH 0/3] Expanding the basic vkms features
@ 2018-05-16 3:06 Rodrigo Siqueira
2018-05-16 3:06 ` [PATCH 1/3] drm/vkms: Add basic CRTC initialization Rodrigo Siqueira
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-05-16 3:06 UTC (permalink / raw)
To: Gustavo Padovan, Daniel Vetter, Sean Paul, Haneen Mohammed; +Cc: dri-devel
This series of patches add a centralized initialization mechanism, a
single CRTC with a plane, an encoder, and extra module information.
Rodrigo Siqueira (3):
drm/vkms: Add basic CRTC initialization
drm/vkms: Add encoder initialization
drm/vkms: Add extra information about vkms
drivers/gpu/drm/Kconfig | 8 +-
drivers/gpu/drm/vkms/Makefile | 2 +-
drivers/gpu/drm/vkms/vkms_display.c | 133 ++++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_drv.c | 43 +--------
drivers/gpu/drm/vkms/vkms_drv.h | 28 +++++-
drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++
6 files changed, 215 insertions(+), 45 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] drm/vkms: Add basic CRTC initialization
2018-05-16 3:06 [PATCH 0/3] Expanding the basic vkms features Rodrigo Siqueira
@ 2018-05-16 3:06 ` Rodrigo Siqueira
2018-05-16 9:51 ` Daniel Vetter
2018-05-16 3:07 ` [PATCH 2/3] drm/vkms: Add encoder initialization Rodrigo Siqueira
2018-05-16 3:07 ` [PATCH 3/3] drm/vkms: Add extra information about vkms Rodrigo Siqueira
2 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-05-16 3:06 UTC (permalink / raw)
To: Gustavo Padovan, Daniel Vetter, Sean Paul, Haneen Mohammed; +Cc: dri-devel
This commit adds the essential infrastructure for managing CRTCs which
is composed of: a new data struct for output data information, a
function for basic modeset initialization, and the operation to create
planes. Due to the introduction of a new initialization function,
connectors were moved from vkms_drv.c to vkms_display.c.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/Makefile | 2 +-
drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
5 files changed, 180 insertions(+), 43 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 2aef948d3a34..f747e2a3a6f4 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
new file mode 100644
index 000000000000..b20b41f9590b
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_display.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+
+#define XRES_MIN 32
+#define YRES_MIN 32
+
+#define XRES_DEF 1024
+#define YRES_DEF 768
+
+#define XRES_MAX 8192
+#define YRES_MAX 8192
+
+static const struct drm_mode_config_funcs vkms_mode_funcs = {
+ .atomic_check = drm_atomic_helper_check,
+ .atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_crtc_funcs vkms_crtc_funcs = {
+ .set_config = drm_atomic_helper_set_config,
+ .destroy = drm_crtc_cleanup,
+ .page_flip = drm_atomic_helper_page_flip,
+ .reset = drm_atomic_helper_crtc_reset,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static void vkms_connector_destroy(struct drm_connector *connector)
+{
+ drm_connector_unregister(connector);
+ drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs vkms_connector_funcs = {
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = vkms_connector_destroy,
+};
+
+static int vkms_output_init(struct vkms_device *vkmsdev)
+{
+ struct vkms_output *output = &vkmsdev->output;
+ struct drm_device *dev = &vkmsdev->drm;
+ struct drm_connector *connector = &output->connector;
+ struct drm_crtc *crtc = &output->crtc;
+ struct drm_plane *primary;
+ int ret;
+
+ primary = vkms_plane_init(vkmsdev);
+ if (IS_ERR(primary))
+ return PTR_ERR(primary);
+
+ ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
+ &vkms_crtc_funcs, NULL);
+ if (ret) {
+ DRM_ERROR("Failed to init CRTC\n");
+ goto err_crtc;
+ }
+ primary->crtc = crtc;
+
+ ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
+ DRM_MODE_CONNECTOR_VIRTUAL);
+ if (ret) {
+ DRM_ERROR("Failed to init connector\n");
+ goto err_connector;
+ }
+
+ ret = drm_connector_register(connector);
+ if (ret) {
+ DRM_ERROR("Failed to register connector\n");
+ goto err_connector_register;
+ }
+
+ drm_mode_config_reset(dev);
+
+ return 0;
+
+err_connector_register:
+ drm_connector_cleanup(connector);
+
+err_connector:
+ drm_crtc_cleanup(crtc);
+
+err_crtc:
+ drm_plane_cleanup(primary);
+ return ret;
+}
+
+int vkms_modeset_init(struct vkms_device *vkmsdev)
+{
+ struct drm_device *dev = &vkmsdev->drm;
+
+ drm_mode_config_init(dev);
+ dev->mode_config.funcs = &vkms_mode_funcs;
+ dev->mode_config.min_width = XRES_MIN;
+ dev->mode_config.min_height = YRES_MIN;
+ dev->mode_config.max_width = XRES_MAX;
+ dev->mode_config.max_height = YRES_MAX;
+
+ return vkms_output_init(vkmsdev);
+}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 35517b09538e..11e0569807bd 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -6,9 +6,7 @@
*/
#include <linux/module.h>
-#include <drm/drmP.h>
#include <drm/drm_gem.h>
-#include <drm/drm_crtc_helper.h>
#include "vkms_drv.h"
#define DRIVER_NAME "vkms"
@@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
.minor = DRIVER_MINOR,
};
-static const u32 vkms_formats[] = {
- DRM_FORMAT_XRGB8888,
-};
-
-static void vkms_connector_destroy(struct drm_connector *connector)
-{
- drm_connector_unregister(connector);
- drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs vkms_connector_funcs = {
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = vkms_connector_destroy,
-};
-
static int __init vkms_init(void)
{
int ret;
@@ -86,34 +69,14 @@ static int __init vkms_init(void)
goto out_fini;
}
- drm_mode_config_init(&vkms_device->drm);
-
- ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
- &vkms_connector_funcs,
- DRM_MODE_CONNECTOR_VIRTUAL);
- if (ret < 0) {
- DRM_ERROR("Failed to init connector\n");
- goto out_unregister;
- }
-
- ret = drm_simple_display_pipe_init(&vkms_device->drm,
- &vkms_device->pipe,
- NULL,
- vkms_formats,
- ARRAY_SIZE(vkms_formats),
- NULL,
- &vkms_device->connector);
- if (ret < 0) {
- DRM_ERROR("Cannot setup simple display pipe\n");
+ ret = vkms_modeset_init(vkms_device);
+ if (ret)
goto out_unregister;
- }
ret = drm_dev_register(&vkms_device->drm, 0);
if (ret)
goto out_unregister;
- drm_connector_register(&vkms_device->connector);
-
return 0;
out_unregister:
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index c77c5bf5032a..292bdea9c785 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -1,13 +1,33 @@
#ifndef _VKMS_DRV_H_
#define _VKMS_DRV_H_
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drmP.h>
+#include <drm/drm.h>
+
+static const u32 vkms_formats[] = {
+ DRM_FORMAT_XRGB8888,
+ DRM_FORMAT_ARGB8888,
+ DRM_FORMAT_BGRX8888,
+ DRM_FORMAT_BGRA8888,
+ DRM_FORMAT_RGBX8888,
+ DRM_FORMAT_RGBA8888,
+ DRM_FORMAT_XBGR8888,
+ DRM_FORMAT_ABGR8888,
+};
+
+struct vkms_output {
+ struct drm_crtc crtc;
+ struct drm_connector connector;
+};
struct vkms_device {
struct drm_device drm;
struct platform_device *platform;
- struct drm_simple_display_pipe pipe;
- struct drm_connector connector;
+ struct vkms_output output;
};
+int vkms_modeset_init(struct vkms_device *vkmsdev);
+
+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
+
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
new file mode 100644
index 000000000000..2c25b1d6ab5b
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
+
+static const struct drm_plane_funcs vkms_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
+{
+ struct drm_device *dev = &vkmsdev->drm;
+ struct drm_plane *plane;
+ const u32 *formats;
+ int ret, nformats;
+
+ plane = kzalloc(sizeof(*plane), GFP_KERNEL);
+ if (!plane)
+ return ERR_PTR(-ENOMEM);
+
+ formats = vkms_formats;
+ nformats = ARRAY_SIZE(vkms_formats);
+
+ ret = drm_universal_plane_init(dev, plane, 0,
+ &vkms_plane_funcs,
+ formats, nformats,
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret) {
+ kfree(plane);
+ return ERR_PTR(ret);
+ }
+
+ return plane;
+}
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] drm/vkms: Add encoder initialization
2018-05-16 3:06 [PATCH 0/3] Expanding the basic vkms features Rodrigo Siqueira
2018-05-16 3:06 ` [PATCH 1/3] drm/vkms: Add basic CRTC initialization Rodrigo Siqueira
@ 2018-05-16 3:07 ` Rodrigo Siqueira
2018-05-16 9:45 ` Daniel Vetter
2018-05-16 3:07 ` [PATCH 3/3] drm/vkms: Add extra information about vkms Rodrigo Siqueira
2 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-05-16 3:07 UTC (permalink / raw)
To: Gustavo Padovan, Daniel Vetter, Sean Paul, Haneen Mohammed; +Cc: dri-devel
This commit adds a single and simple virtual encoder to VKMS.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/vkms_display.c | 25 +++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_drv.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
index b20b41f9590b..d6702128b5f7 100644
--- a/drivers/gpu/drm/vkms/vkms_display.c
+++ b/drivers/gpu/drm/vkms/vkms_display.c
@@ -44,11 +44,16 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
.destroy = vkms_connector_destroy,
};
+static const struct drm_encoder_funcs vkms_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static int vkms_output_init(struct vkms_device *vkmsdev)
{
struct vkms_output *output = &vkmsdev->output;
struct drm_device *dev = &vkmsdev->drm;
struct drm_connector *connector = &output->connector;
+ struct drm_encoder *encoder = &output->encoder;
struct drm_crtc *crtc = &output->crtc;
struct drm_plane *primary;
int ret;
@@ -78,10 +83,30 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
goto err_connector_register;
}
+ ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret) {
+ DRM_ERROR("Failed to init encoder\n");
+ goto err_encoder;
+ }
+ encoder->possible_crtcs = 1;
+
+ ret = drm_mode_connector_attach_encoder(connector, encoder);
+ if (ret) {
+ DRM_ERROR("Failed to attach connector to encoder\n");
+ goto err_attach;
+ }
+
drm_mode_config_reset(dev);
return 0;
+err_attach:
+ drm_encoder_cleanup(encoder);
+
+err_encoder:
+ drm_connector_unregister(connector);
+
err_connector_register:
drm_connector_cleanup(connector);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 292bdea9c785..933eedc5b25b 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -3,6 +3,7 @@
#include <drm/drmP.h>
#include <drm/drm.h>
+#include <drm/drm_encoder.h>
static const u32 vkms_formats[] = {
DRM_FORMAT_XRGB8888,
@@ -17,6 +18,7 @@ static const u32 vkms_formats[] = {
struct vkms_output {
struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector connector;
};
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm/vkms: Add extra information about vkms
2018-05-16 3:06 [PATCH 0/3] Expanding the basic vkms features Rodrigo Siqueira
2018-05-16 3:06 ` [PATCH 1/3] drm/vkms: Add basic CRTC initialization Rodrigo Siqueira
2018-05-16 3:07 ` [PATCH 2/3] drm/vkms: Add encoder initialization Rodrigo Siqueira
@ 2018-05-16 3:07 ` Rodrigo Siqueira
2 siblings, 0 replies; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-05-16 3:07 UTC (permalink / raw)
To: Gustavo Padovan, Daniel Vetter, Sean Paul, Haneen Mohammed; +Cc: dri-devel
Add the following additional information: authors and description in
Kconfig.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/Kconfig | 8 ++++++--
drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 0893b1d8ede7..a16efdea00fa 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -213,10 +213,14 @@ config DRM_VGEM
If M is selected the module will be called vgem.
config DRM_VKMS
- tristate "Virtual KMS"
+ tristate "Virtual KMS (EXPERIMENTAL)"
depends on DRM
+ default n
help
- Choose this option to get a virtual kernal mode-setting driver.
+ Virtual Kernel Mode-Setting (VKMS) is used for testing or for
+ running GPU in a headless machines. Choose this option to get
+ a VKMS.
+
If M is selected the module will be called vkms.
source "drivers/gpu/drm/exynos/Kconfig"
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 11e0569807bd..07f53f095562 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -105,5 +105,7 @@ static void __exit vkms_exit(void)
module_init(vkms_init);
module_exit(vkms_exit);
+MODULE_AUTHOR("Haneen Mohammed <hamohammed.sa@gmail.com>");
+MODULE_AUTHOR("Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>");
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/vkms: Add encoder initialization
2018-05-16 3:07 ` [PATCH 2/3] drm/vkms: Add encoder initialization Rodrigo Siqueira
@ 2018-05-16 9:45 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-05-16 9:45 UTC (permalink / raw)
To: Rodrigo Siqueira; +Cc: Haneen Mohammed, dri-devel, Sean Paul, Daniel Vetter
On Wed, May 16, 2018 at 12:07:12AM -0300, Rodrigo Siqueira wrote:
> This commit adds a single and simple virtual encoder to VKMS.
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Doesn't this break bisection, i.e. between patch 1&2 vkms looks broken
(because the encoder is missing)?
If so probably better to merge this into patch 1.
-Daniel
> ---
> drivers/gpu/drm/vkms/vkms_display.c | 25 +++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_drv.h | 2 ++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
> index b20b41f9590b..d6702128b5f7 100644
> --- a/drivers/gpu/drm/vkms/vkms_display.c
> +++ b/drivers/gpu/drm/vkms/vkms_display.c
> @@ -44,11 +44,16 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
> .destroy = vkms_connector_destroy,
> };
>
> +static const struct drm_encoder_funcs vkms_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> static int vkms_output_init(struct vkms_device *vkmsdev)
> {
> struct vkms_output *output = &vkmsdev->output;
> struct drm_device *dev = &vkmsdev->drm;
> struct drm_connector *connector = &output->connector;
> + struct drm_encoder *encoder = &output->encoder;
> struct drm_crtc *crtc = &output->crtc;
> struct drm_plane *primary;
> int ret;
> @@ -78,10 +83,30 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
> goto err_connector_register;
> }
>
> + ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> + DRM_MODE_ENCODER_VIRTUAL, NULL);
> + if (ret) {
> + DRM_ERROR("Failed to init encoder\n");
> + goto err_encoder;
> + }
> + encoder->possible_crtcs = 1;
> +
> + ret = drm_mode_connector_attach_encoder(connector, encoder);
> + if (ret) {
> + DRM_ERROR("Failed to attach connector to encoder\n");
> + goto err_attach;
> + }
> +
> drm_mode_config_reset(dev);
>
> return 0;
>
> +err_attach:
> + drm_encoder_cleanup(encoder);
> +
> +err_encoder:
> + drm_connector_unregister(connector);
> +
> err_connector_register:
> drm_connector_cleanup(connector);
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 292bdea9c785..933eedc5b25b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -3,6 +3,7 @@
>
> #include <drm/drmP.h>
> #include <drm/drm.h>
> +#include <drm/drm_encoder.h>
>
> static const u32 vkms_formats[] = {
> DRM_FORMAT_XRGB8888,
> @@ -17,6 +18,7 @@ static const u32 vkms_formats[] = {
>
> struct vkms_output {
> struct drm_crtc crtc;
> + struct drm_encoder encoder;
> struct drm_connector connector;
> };
>
> --
> 2.17.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
2018-05-16 3:06 ` [PATCH 1/3] drm/vkms: Add basic CRTC initialization Rodrigo Siqueira
@ 2018-05-16 9:51 ` Daniel Vetter
2018-05-16 14:51 ` Rodrigo Siqueira
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2018-05-16 9:51 UTC (permalink / raw)
To: Rodrigo Siqueira; +Cc: Haneen Mohammed, dri-devel, Sean Paul, Daniel Vetter
On Wed, May 16, 2018 at 12:06:54AM -0300, Rodrigo Siqueira wrote:
> This commit adds the essential infrastructure for managing CRTCs which
> is composed of: a new data struct for output data information, a
> function for basic modeset initialization, and the operation to create
> planes. Due to the introduction of a new initialization function,
> connectors were moved from vkms_drv.c to vkms_display.c.
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
> drivers/gpu/drm/vkms/Makefile | 2 +-
> drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
> drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
> drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
> 5 files changed, 180 insertions(+), 43 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
> create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
>
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 2aef948d3a34..f747e2a3a6f4 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,3 +1,3 @@
> -vkms-y := vkms_drv.o
> +vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
>
> obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
> new file mode 100644
> index 000000000000..b20b41f9590b
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_display.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +#define XRES_MIN 32
> +#define YRES_MIN 32
> +
> +#define XRES_DEF 1024
> +#define YRES_DEF 768
These seem unused.
> +#define XRES_MAX 8192
> +#define YRES_MAX 8192
> +
> +static const struct drm_mode_config_funcs vkms_mode_funcs = {
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static const struct drm_crtc_funcs vkms_crtc_funcs = {
> + .set_config = drm_atomic_helper_set_config,
> + .destroy = drm_crtc_cleanup,
> + .page_flip = drm_atomic_helper_page_flip,
> + .reset = drm_atomic_helper_crtc_reset,
> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +};
> +
> +static void vkms_connector_destroy(struct drm_connector *connector)
> +{
> + drm_connector_unregister(connector);
> + drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs vkms_connector_funcs = {
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = vkms_connector_destroy,
> +};
> +
> +static int vkms_output_init(struct vkms_device *vkmsdev)
> +{
> + struct vkms_output *output = &vkmsdev->output;
> + struct drm_device *dev = &vkmsdev->drm;
> + struct drm_connector *connector = &output->connector;
> + struct drm_crtc *crtc = &output->crtc;
> + struct drm_plane *primary;
> + int ret;
> +
> + primary = vkms_plane_init(vkmsdev);
> + if (IS_ERR(primary))
> + return PTR_ERR(primary);
> +
> + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
> + &vkms_crtc_funcs, NULL);
> + if (ret) {
> + DRM_ERROR("Failed to init CRTC\n");
> + goto err_crtc;
> + }
> + primary->crtc = crtc;
If you want to split stuff up a bit, I think putting the crtc stuff into
vkms_crtc.c, and then renaming this file here to vkms_output.c would make
sense.
> +
> + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> + DRM_MODE_CONNECTOR_VIRTUAL);
> + if (ret) {
> + DRM_ERROR("Failed to init connector\n");
> + goto err_connector;
> + }
> +
> + ret = drm_connector_register(connector);
> + if (ret) {
> + DRM_ERROR("Failed to register connector\n");
> + goto err_connector_register;
> + }
> +
> + drm_mode_config_reset(dev);
> +
> + return 0;
> +
> +err_connector_register:
> + drm_connector_cleanup(connector);
> +
> +err_connector:
> + drm_crtc_cleanup(crtc);
> +
> +err_crtc:
> + drm_plane_cleanup(primary);
> + return ret;
> +}
> +
> +int vkms_modeset_init(struct vkms_device *vkmsdev)
Plus keeping vkms_modeset_init in vkms_drv.c, vkms is 100% about
modesetting and nothing else, so splitting that out is a bit overkill imo.
> +{
> + struct drm_device *dev = &vkmsdev->drm;
> +
> + drm_mode_config_init(dev);
> + dev->mode_config.funcs = &vkms_mode_funcs;
> + dev->mode_config.min_width = XRES_MIN;
> + dev->mode_config.min_height = YRES_MIN;
> + dev->mode_config.max_width = XRES_MAX;
> + dev->mode_config.max_height = YRES_MAX;
> +
> + return vkms_output_init(vkmsdev);
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 35517b09538e..11e0569807bd 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -6,9 +6,7 @@
> */
>
> #include <linux/module.h>
> -#include <drm/drmP.h>
> #include <drm/drm_gem.h>
> -#include <drm/drm_crtc_helper.h>
> #include "vkms_drv.h"
>
> #define DRIVER_NAME "vkms"
> @@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
> .minor = DRIVER_MINOR,
> };
>
> -static const u32 vkms_formats[] = {
> - DRM_FORMAT_XRGB8888,
> -};
> -
> -static void vkms_connector_destroy(struct drm_connector *connector)
> -{
> - drm_connector_unregister(connector);
> - drm_connector_cleanup(connector);
> -}
> -
> -static const struct drm_connector_funcs vkms_connector_funcs = {
> - .fill_modes = drm_helper_probe_single_connector_modes,
> - .destroy = vkms_connector_destroy,
> -};
> -
> static int __init vkms_init(void)
> {
> int ret;
> @@ -86,34 +69,14 @@ static int __init vkms_init(void)
> goto out_fini;
> }
>
> - drm_mode_config_init(&vkms_device->drm);
> -
> - ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
> - &vkms_connector_funcs,
> - DRM_MODE_CONNECTOR_VIRTUAL);
> - if (ret < 0) {
> - DRM_ERROR("Failed to init connector\n");
> - goto out_unregister;
> - }
> -
> - ret = drm_simple_display_pipe_init(&vkms_device->drm,
> - &vkms_device->pipe,
> - NULL,
> - vkms_formats,
> - ARRAY_SIZE(vkms_formats),
> - NULL,
> - &vkms_device->connector);
> - if (ret < 0) {
> - DRM_ERROR("Cannot setup simple display pipe\n");
> + ret = vkms_modeset_init(vkms_device);
> + if (ret)
> goto out_unregister;
> - }
>
> ret = drm_dev_register(&vkms_device->drm, 0);
> if (ret)
> goto out_unregister;
>
> - drm_connector_register(&vkms_device->connector);
> -
> return 0;
>
> out_unregister:
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c77c5bf5032a..292bdea9c785 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -1,13 +1,33 @@
> #ifndef _VKMS_DRV_H_
> #define _VKMS_DRV_H_
>
> -#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drmP.h>
> +#include <drm/drm.h>
> +
> +static const u32 vkms_formats[] = {
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_BGRX8888,
> + DRM_FORMAT_BGRA8888,
> + DRM_FORMAT_RGBX8888,
> + DRM_FORMAT_RGBA8888,
> + DRM_FORMAT_XBGR8888,
> + DRM_FORMAT_ABGR8888,
Why all these varianst? Right now we support none of this anyway ... Until
we have more I think just limiting to XRBG8888 is good enough, we need to
flesh out the crc support first (and backing memory handling too).
-Daniel
> +};
> +
> +struct vkms_output {
> + struct drm_crtc crtc;
> + struct drm_connector connector;
> +};
>
> struct vkms_device {
> struct drm_device drm;
> struct platform_device *platform;
> - struct drm_simple_display_pipe pipe;
> - struct drm_connector connector;
> + struct vkms_output output;
> };
>
> +int vkms_modeset_init(struct vkms_device *vkmsdev);
> +
> +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> new file mode 100644
> index 000000000000..2c25b1d6ab5b
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +
> +static const struct drm_plane_funcs vkms_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = drm_plane_cleanup,
> + .reset = drm_atomic_helper_plane_reset,
> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +};
> +
> +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
> +{
> + struct drm_device *dev = &vkmsdev->drm;
> + struct drm_plane *plane;
> + const u32 *formats;
> + int ret, nformats;
> +
> + plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> + if (!plane)
> + return ERR_PTR(-ENOMEM);
> +
> + formats = vkms_formats;
> + nformats = ARRAY_SIZE(vkms_formats);
> +
> + ret = drm_universal_plane_init(dev, plane, 0,
> + &vkms_plane_funcs,
> + formats, nformats,
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret) {
> + kfree(plane);
> + return ERR_PTR(ret);
> + }
> +
> + return plane;
> +}
> --
> 2.17.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
2018-05-16 9:51 ` Daniel Vetter
@ 2018-05-16 14:51 ` Rodrigo Siqueira
2018-05-16 15:40 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-05-16 14:51 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Haneen Mohammed, dri-devel, Sean Paul, Daniel Vetter
Hi Daniel,
Thanks for the feedback :)
You can find my comments below:
On 05/16, Daniel Vetter wrote:
> On Wed, May 16, 2018 at 12:06:54AM -0300, Rodrigo Siqueira wrote:
> > This commit adds the essential infrastructure for managing CRTCs which
> > is composed of: a new data struct for output data information, a
> > function for basic modeset initialization, and the operation to create
> > planes. Due to the introduction of a new initialization function,
> > connectors were moved from vkms_drv.c to vkms_display.c.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> > drivers/gpu/drm/vkms/Makefile | 2 +-
> > drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
> > drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
> > drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
> > drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
> > 5 files changed, 180 insertions(+), 43 deletions(-)
> > create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
> > create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
> >
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 2aef948d3a34..f747e2a3a6f4 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,3 +1,3 @@
> > -vkms-y := vkms_drv.o
> > +vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
> >
> > obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
> > new file mode 100644
> > index 000000000000..b20b41f9590b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_display.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +#define XRES_MIN 32
> > +#define YRES_MIN 32
> > +
> > +#define XRES_DEF 1024
> > +#define YRES_DEF 768
>
> These seem unused.
I created these defines because the documentation says:
"[..]Once done, mode configuration must be setup by initializing the
following fields."
(https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.drm_mode_config_init)
Finally, I used them in "mode_config" fields from drm_device (code below).
Is it ok to not setup these values on mode_config? I looked at virtio as
an inspiration for this.
> > +static const struct drm_mode_config_funcs vkms_mode_funcs = {
> > + .atomic_check = drm_atomic_helper_check,
> > + .atomic_commit = drm_atomic_helper_commit,
> > +};
> > +
> > +static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > + .set_config = drm_atomic_helper_set_config,
> > + .destroy = drm_crtc_cleanup,
> > + .page_flip = drm_atomic_helper_page_flip,
> > + .reset = drm_atomic_helper_crtc_reset,
> > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> > +};
> > +
> > +static void vkms_connector_destroy(struct drm_connector *connector)
> > +{
> > + drm_connector_unregister(connector);
> > + drm_connector_cleanup(connector);
> > +}
> > +
> > +static const struct drm_connector_funcs vkms_connector_funcs = {
> > + .fill_modes = drm_helper_probe_single_connector_modes,
> > + .destroy = vkms_connector_destroy,
> > +};
> > +
> > +static int vkms_output_init(struct vkms_device *vkmsdev)
> > +{
> > + struct vkms_output *output = &vkmsdev->output;
> > + struct drm_device *dev = &vkmsdev->drm;
> > + struct drm_connector *connector = &output->connector;
> > + struct drm_crtc *crtc = &output->crtc;
> > + struct drm_plane *primary;
> > + int ret;
> > +
> > + primary = vkms_plane_init(vkmsdev);
> > + if (IS_ERR(primary))
> > + return PTR_ERR(primary);
> > +
> > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
> > + &vkms_crtc_funcs, NULL);
> > + if (ret) {
> > + DRM_ERROR("Failed to init CRTC\n");
> > + goto err_crtc;
> > + }
> > + primary->crtc = crtc;
>
> If you want to split stuff up a bit, I think putting the crtc stuff into
> vkms_crtc.c, and then renaming this file here to vkms_output.c would make
> sense.
Nice, make a lot of sense to me. I will do it on v2.
> > +
> > + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > + DRM_MODE_CONNECTOR_VIRTUAL);
> > + if (ret) {
> > + DRM_ERROR("Failed to init connector\n");
> > + goto err_connector;
> > + }
> > +
> > + ret = drm_connector_register(connector);
> > + if (ret) {
> > + DRM_ERROR("Failed to register connector\n");
> > + goto err_connector_register;
> > + }
> > +
> > + drm_mode_config_reset(dev);
> > +
> > + return 0;
> > +
> > +err_connector_register:
> > + drm_connector_cleanup(connector);
> > +
> > +err_connector:
> > + drm_crtc_cleanup(crtc);
> > +
> > +err_crtc:
> > + drm_plane_cleanup(primary);
> > + return ret;
> > +}
> > +
> > +int vkms_modeset_init(struct vkms_device *vkmsdev)
>
> Plus keeping vkms_modeset_init in vkms_drv.c, vkms is 100% about
> modesetting and nothing else, so splitting that out is a bit overkill imo.
Ok, I will do it too for v2.
> > +{
> > + struct drm_device *dev = &vkmsdev->drm;
> > +
> > + drm_mode_config_init(dev);
> > + dev->mode_config.funcs = &vkms_mode_funcs;
> > + dev->mode_config.min_width = XRES_MIN;
> > + dev->mode_config.min_height = YRES_MIN;
> > + dev->mode_config.max_width = XRES_MAX;
> > + dev->mode_config.max_height = YRES_MAX;
> > +
> > + return vkms_output_init(vkmsdev);
> > +}
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 35517b09538e..11e0569807bd 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -6,9 +6,7 @@
> > */
> >
> > #include <linux/module.h>
> > -#include <drm/drmP.h>
> > #include <drm/drm_gem.h>
> > -#include <drm/drm_crtc_helper.h>
> > #include "vkms_drv.h"
> >
> > #define DRIVER_NAME "vkms"
> > @@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
> > .minor = DRIVER_MINOR,
> > };
> >
> > -static const u32 vkms_formats[] = {
> > - DRM_FORMAT_XRGB8888,
> > -};
> > -
> > -static void vkms_connector_destroy(struct drm_connector *connector)
> > -{
> > - drm_connector_unregister(connector);
> > - drm_connector_cleanup(connector);
> > -}
> > -
> > -static const struct drm_connector_funcs vkms_connector_funcs = {
> > - .fill_modes = drm_helper_probe_single_connector_modes,
> > - .destroy = vkms_connector_destroy,
> > -};
> > -
> > static int __init vkms_init(void)
> > {
> > int ret;
> > @@ -86,34 +69,14 @@ static int __init vkms_init(void)
> > goto out_fini;
> > }
> >
> > - drm_mode_config_init(&vkms_device->drm);
> > -
> > - ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
> > - &vkms_connector_funcs,
> > - DRM_MODE_CONNECTOR_VIRTUAL);
> > - if (ret < 0) {
> > - DRM_ERROR("Failed to init connector\n");
> > - goto out_unregister;
> > - }
> > -
> > - ret = drm_simple_display_pipe_init(&vkms_device->drm,
> > - &vkms_device->pipe,
> > - NULL,
> > - vkms_formats,
> > - ARRAY_SIZE(vkms_formats),
> > - NULL,
> > - &vkms_device->connector);
> > - if (ret < 0) {
> > - DRM_ERROR("Cannot setup simple display pipe\n");
> > + ret = vkms_modeset_init(vkms_device);
> > + if (ret)
> > goto out_unregister;
> > - }
> >
> > ret = drm_dev_register(&vkms_device->drm, 0);
> > if (ret)
> > goto out_unregister;
> >
> > - drm_connector_register(&vkms_device->connector);
> > -
> > return 0;
> >
> > out_unregister:
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index c77c5bf5032a..292bdea9c785 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -1,13 +1,33 @@
> > #ifndef _VKMS_DRV_H_
> > #define _VKMS_DRV_H_
> >
> > -#include <drm/drm_simple_kms_helper.h>
> > +#include <drm/drmP.h>
> > +#include <drm/drm.h>
> > +
> > +static const u32 vkms_formats[] = {
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_ARGB8888,
> > + DRM_FORMAT_BGRX8888,
> > + DRM_FORMAT_BGRA8888,
> > + DRM_FORMAT_RGBX8888,
> > + DRM_FORMAT_RGBA8888,
> > + DRM_FORMAT_XBGR8888,
> > + DRM_FORMAT_ABGR8888,
>
> Why all these varianst? Right now we support none of this anyway ... Until
> we have more I think just limiting to XRBG8888 is good enough, we need to
> flesh out the crc support first (and backing memory handling too).
My bad, I don't have any good argument for using all of this variants. I
will keep it simple and adds only XRBG8888.
Finally, I will prepare the v2 with your recommendations and I will merge
the second patch (encoder) with this one.
Thanks
> -Daniel
>
> > +};
> > +
> > +struct vkms_output {
> > + struct drm_crtc crtc;
> > + struct drm_connector connector;
> > +};
> >
> > struct vkms_device {
> > struct drm_device drm;
> > struct platform_device *platform;
> > - struct drm_simple_display_pipe pipe;
> > - struct drm_connector connector;
> > + struct vkms_output output;
> > };
> >
> > +int vkms_modeset_init(struct vkms_device *vkmsdev);
> > +
> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
> > +
> > #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > new file mode 100644
> > index 000000000000..2c25b1d6ab5b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -0,0 +1,46 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_plane_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +
> > +static const struct drm_plane_funcs vkms_plane_funcs = {
> > + .update_plane = drm_atomic_helper_update_plane,
> > + .disable_plane = drm_atomic_helper_disable_plane,
> > + .destroy = drm_plane_cleanup,
> > + .reset = drm_atomic_helper_plane_reset,
> > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > +};
> > +
> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
> > +{
> > + struct drm_device *dev = &vkmsdev->drm;
> > + struct drm_plane *plane;
> > + const u32 *formats;
> > + int ret, nformats;
> > +
> > + plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> > + if (!plane)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + formats = vkms_formats;
> > + nformats = ARRAY_SIZE(vkms_formats);
> > +
> > + ret = drm_universal_plane_init(dev, plane, 0,
> > + &vkms_plane_funcs,
> > + formats, nformats,
> > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> > + if (ret) {
> > + kfree(plane);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + return plane;
> > +}
> > --
> > 2.17.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
2018-05-16 14:51 ` Rodrigo Siqueira
@ 2018-05-16 15:40 ` Daniel Vetter
2018-05-16 15:41 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2018-05-16 15:40 UTC (permalink / raw)
To: Rodrigo Siqueira; +Cc: dri-devel, Sean Paul, Haneen Mohammed
On Wed, May 16, 2018 at 4:51 PM, Rodrigo Siqueira
<rodrigosiqueiramelo@gmail.com> wrote:
> Hi Daniel,
>
> Thanks for the feedback :)
>
> You can find my comments below:
>
> On 05/16, Daniel Vetter wrote:
>> On Wed, May 16, 2018 at 12:06:54AM -0300, Rodrigo Siqueira wrote:
>> > This commit adds the essential infrastructure for managing CRTCs which
>> > is composed of: a new data struct for output data information, a
>> > function for basic modeset initialization, and the operation to create
>> > planes. Due to the introduction of a new initialization function,
>> > connectors were moved from vkms_drv.c to vkms_display.c.
>> >
>> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>> > ---
>> > drivers/gpu/drm/vkms/Makefile | 2 +-
>> > drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
>> > drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
>> > drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
>> > drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
>> > 5 files changed, 180 insertions(+), 43 deletions(-)
>> > create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
>> > create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
>> >
>> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
>> > index 2aef948d3a34..f747e2a3a6f4 100644
>> > --- a/drivers/gpu/drm/vkms/Makefile
>> > +++ b/drivers/gpu/drm/vkms/Makefile
>> > @@ -1,3 +1,3 @@
>> > -vkms-y := vkms_drv.o
>> > +vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
>> >
>> > obj-$(CONFIG_DRM_VKMS) += vkms.o
>> > diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
>> > new file mode 100644
>> > index 000000000000..b20b41f9590b
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/vkms/vkms_display.c
>> > @@ -0,0 +1,108 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/*
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + */
>> > +
>> > +#include "vkms_drv.h"
>> > +#include <drm/drm_atomic_helper.h>
>> > +#include <drm/drm_crtc_helper.h>
>> > +
>> > +#define XRES_MIN 32
>> > +#define YRES_MIN 32
>> > +
>> > +#define XRES_DEF 1024
>> > +#define YRES_DEF 768
>>
>> These seem unused.
>
> I created these defines because the documentation says:
>
> "[..]Once done, mode configuration must be setup by initializing the
> following fields."
> (https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.drm_mode_config_init)
>
> Finally, I used them in "mode_config" fields from drm_device (code below).
>
> Is it ok to not setup these values on mode_config? I looked at virtio as
> an inspiration for this.
XYRES_MIN and _MAX you need, but _DEF seems unused. That's why I
asked, sorry for not making this clear.
-Daniel
>
>> > +static const struct drm_mode_config_funcs vkms_mode_funcs = {
>> > + .atomic_check = drm_atomic_helper_check,
>> > + .atomic_commit = drm_atomic_helper_commit,
>> > +};
>> > +
>> > +static const struct drm_crtc_funcs vkms_crtc_funcs = {
>> > + .set_config = drm_atomic_helper_set_config,
>> > + .destroy = drm_crtc_cleanup,
>> > + .page_flip = drm_atomic_helper_page_flip,
>> > + .reset = drm_atomic_helper_crtc_reset,
>> > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>> > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> > +};
>> > +
>> > +static void vkms_connector_destroy(struct drm_connector *connector)
>> > +{
>> > + drm_connector_unregister(connector);
>> > + drm_connector_cleanup(connector);
>> > +}
>> > +
>> > +static const struct drm_connector_funcs vkms_connector_funcs = {
>> > + .fill_modes = drm_helper_probe_single_connector_modes,
>> > + .destroy = vkms_connector_destroy,
>> > +};
>> > +
>> > +static int vkms_output_init(struct vkms_device *vkmsdev)
>> > +{
>> > + struct vkms_output *output = &vkmsdev->output;
>> > + struct drm_device *dev = &vkmsdev->drm;
>> > + struct drm_connector *connector = &output->connector;
>> > + struct drm_crtc *crtc = &output->crtc;
>> > + struct drm_plane *primary;
>> > + int ret;
>> > +
>> > + primary = vkms_plane_init(vkmsdev);
>> > + if (IS_ERR(primary))
>> > + return PTR_ERR(primary);
>> > +
>> > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
>> > + &vkms_crtc_funcs, NULL);
>> > + if (ret) {
>> > + DRM_ERROR("Failed to init CRTC\n");
>> > + goto err_crtc;
>> > + }
>> > + primary->crtc = crtc;
>>
>> If you want to split stuff up a bit, I think putting the crtc stuff into
>> vkms_crtc.c, and then renaming this file here to vkms_output.c would make
>> sense.
>
> Nice, make a lot of sense to me. I will do it on v2.
>
>> > +
>> > + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
>> > + DRM_MODE_CONNECTOR_VIRTUAL);
>> > + if (ret) {
>> > + DRM_ERROR("Failed to init connector\n");
>> > + goto err_connector;
>> > + }
>> > +
>> > + ret = drm_connector_register(connector);
>> > + if (ret) {
>> > + DRM_ERROR("Failed to register connector\n");
>> > + goto err_connector_register;
>> > + }
>> > +
>> > + drm_mode_config_reset(dev);
>> > +
>> > + return 0;
>> > +
>> > +err_connector_register:
>> > + drm_connector_cleanup(connector);
>> > +
>> > +err_connector:
>> > + drm_crtc_cleanup(crtc);
>> > +
>> > +err_crtc:
>> > + drm_plane_cleanup(primary);
>> > + return ret;
>> > +}
>> > +
>> > +int vkms_modeset_init(struct vkms_device *vkmsdev)
>>
>> Plus keeping vkms_modeset_init in vkms_drv.c, vkms is 100% about
>> modesetting and nothing else, so splitting that out is a bit overkill imo.
>
> Ok, I will do it too for v2.
>
>> > +{
>> > + struct drm_device *dev = &vkmsdev->drm;
>> > +
>> > + drm_mode_config_init(dev);
>> > + dev->mode_config.funcs = &vkms_mode_funcs;
>> > + dev->mode_config.min_width = XRES_MIN;
>> > + dev->mode_config.min_height = YRES_MIN;
>> > + dev->mode_config.max_width = XRES_MAX;
>> > + dev->mode_config.max_height = YRES_MAX;
>> > +
>> > + return vkms_output_init(vkmsdev);
>> > +}
>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
>> > index 35517b09538e..11e0569807bd 100644
>> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>> > @@ -6,9 +6,7 @@
>> > */
>> >
>> > #include <linux/module.h>
>> > -#include <drm/drmP.h>
>> > #include <drm/drm_gem.h>
>> > -#include <drm/drm_crtc_helper.h>
>> > #include "vkms_drv.h"
>> >
>> > #define DRIVER_NAME "vkms"
>> > @@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
>> > .minor = DRIVER_MINOR,
>> > };
>> >
>> > -static const u32 vkms_formats[] = {
>> > - DRM_FORMAT_XRGB8888,
>> > -};
>> > -
>> > -static void vkms_connector_destroy(struct drm_connector *connector)
>> > -{
>> > - drm_connector_unregister(connector);
>> > - drm_connector_cleanup(connector);
>> > -}
>> > -
>> > -static const struct drm_connector_funcs vkms_connector_funcs = {
>> > - .fill_modes = drm_helper_probe_single_connector_modes,
>> > - .destroy = vkms_connector_destroy,
>> > -};
>> > -
>> > static int __init vkms_init(void)
>> > {
>> > int ret;
>> > @@ -86,34 +69,14 @@ static int __init vkms_init(void)
>> > goto out_fini;
>> > }
>> >
>> > - drm_mode_config_init(&vkms_device->drm);
>> > -
>> > - ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
>> > - &vkms_connector_funcs,
>> > - DRM_MODE_CONNECTOR_VIRTUAL);
>> > - if (ret < 0) {
>> > - DRM_ERROR("Failed to init connector\n");
>> > - goto out_unregister;
>> > - }
>> > -
>> > - ret = drm_simple_display_pipe_init(&vkms_device->drm,
>> > - &vkms_device->pipe,
>> > - NULL,
>> > - vkms_formats,
>> > - ARRAY_SIZE(vkms_formats),
>> > - NULL,
>> > - &vkms_device->connector);
>> > - if (ret < 0) {
>> > - DRM_ERROR("Cannot setup simple display pipe\n");
>> > + ret = vkms_modeset_init(vkms_device);
>> > + if (ret)
>> > goto out_unregister;
>> > - }
>> >
>> > ret = drm_dev_register(&vkms_device->drm, 0);
>> > if (ret)
>> > goto out_unregister;
>> >
>> > - drm_connector_register(&vkms_device->connector);
>> > -
>> > return 0;
>> >
>> > out_unregister:
>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>> > index c77c5bf5032a..292bdea9c785 100644
>> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>> > @@ -1,13 +1,33 @@
>> > #ifndef _VKMS_DRV_H_
>> > #define _VKMS_DRV_H_
>> >
>> > -#include <drm/drm_simple_kms_helper.h>
>> > +#include <drm/drmP.h>
>> > +#include <drm/drm.h>
>> > +
>> > +static const u32 vkms_formats[] = {
>> > + DRM_FORMAT_XRGB8888,
>> > + DRM_FORMAT_ARGB8888,
>> > + DRM_FORMAT_BGRX8888,
>> > + DRM_FORMAT_BGRA8888,
>> > + DRM_FORMAT_RGBX8888,
>> > + DRM_FORMAT_RGBA8888,
>> > + DRM_FORMAT_XBGR8888,
>> > + DRM_FORMAT_ABGR8888,
>>
>> Why all these varianst? Right now we support none of this anyway ... Until
>> we have more I think just limiting to XRBG8888 is good enough, we need to
>> flesh out the crc support first (and backing memory handling too).
>
> My bad, I don't have any good argument for using all of this variants. I
> will keep it simple and adds only XRBG8888.
>
> Finally, I will prepare the v2 with your recommendations and I will merge
> the second patch (encoder) with this one.
>
> Thanks
>
>> -Daniel
>>
>> > +};
>> > +
>> > +struct vkms_output {
>> > + struct drm_crtc crtc;
>> > + struct drm_connector connector;
>> > +};
>> >
>> > struct vkms_device {
>> > struct drm_device drm;
>> > struct platform_device *platform;
>> > - struct drm_simple_display_pipe pipe;
>> > - struct drm_connector connector;
>> > + struct vkms_output output;
>> > };
>> >
>> > +int vkms_modeset_init(struct vkms_device *vkmsdev);
>> > +
>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
>> > +
>> > #endif /* _VKMS_DRV_H_ */
>> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>> > new file mode 100644
>> > index 000000000000..2c25b1d6ab5b
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>> > @@ -0,0 +1,46 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/*
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + */
>> > +
>> > +#include "vkms_drv.h"
>> > +#include <drm/drm_plane_helper.h>
>> > +#include <drm/drm_atomic_helper.h>
>> > +
>> > +static const struct drm_plane_funcs vkms_plane_funcs = {
>> > + .update_plane = drm_atomic_helper_update_plane,
>> > + .disable_plane = drm_atomic_helper_disable_plane,
>> > + .destroy = drm_plane_cleanup,
>> > + .reset = drm_atomic_helper_plane_reset,
>> > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> > +};
>> > +
>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
>> > +{
>> > + struct drm_device *dev = &vkmsdev->drm;
>> > + struct drm_plane *plane;
>> > + const u32 *formats;
>> > + int ret, nformats;
>> > +
>> > + plane = kzalloc(sizeof(*plane), GFP_KERNEL);
>> > + if (!plane)
>> > + return ERR_PTR(-ENOMEM);
>> > +
>> > + formats = vkms_formats;
>> > + nformats = ARRAY_SIZE(vkms_formats);
>> > +
>> > + ret = drm_universal_plane_init(dev, plane, 0,
>> > + &vkms_plane_funcs,
>> > + formats, nformats,
>> > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> > + if (ret) {
>> > + kfree(plane);
>> > + return ERR_PTR(ret);
>> > + }
>> > +
>> > + return plane;
>> > +}
>> > --
>> > 2.17.0
>> >
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
2018-05-16 15:40 ` Daniel Vetter
@ 2018-05-16 15:41 ` Daniel Vetter
2018-05-16 15:49 ` Rodrigo Siqueira
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2018-05-16 15:41 UTC (permalink / raw)
To: Rodrigo Siqueira; +Cc: dri-devel, Sean Paul, Haneen Mohammed
On Wed, May 16, 2018 at 5:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, May 16, 2018 at 4:51 PM, Rodrigo Siqueira
> <rodrigosiqueiramelo@gmail.com> wrote:
>> Hi Daniel,
>>
>> Thanks for the feedback :)
>>
>> You can find my comments below:
>>
>> On 05/16, Daniel Vetter wrote:
>>> On Wed, May 16, 2018 at 12:06:54AM -0300, Rodrigo Siqueira wrote:
>>> > This commit adds the essential infrastructure for managing CRTCs which
>>> > is composed of: a new data struct for output data information, a
>>> > function for basic modeset initialization, and the operation to create
>>> > planes. Due to the introduction of a new initialization function,
>>> > connectors were moved from vkms_drv.c to vkms_display.c.
>>> >
>>> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>>> > ---
>>> > drivers/gpu/drm/vkms/Makefile | 2 +-
>>> > drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
>>> > drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
>>> > drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
>>> > drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
>>> > 5 files changed, 180 insertions(+), 43 deletions(-)
>>> > create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
>>> > create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
>>> >
>>> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
>>> > index 2aef948d3a34..f747e2a3a6f4 100644
>>> > --- a/drivers/gpu/drm/vkms/Makefile
>>> > +++ b/drivers/gpu/drm/vkms/Makefile
>>> > @@ -1,3 +1,3 @@
>>> > -vkms-y := vkms_drv.o
>>> > +vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
>>> >
>>> > obj-$(CONFIG_DRM_VKMS) += vkms.o
>>> > diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
>>> > new file mode 100644
>>> > index 000000000000..b20b41f9590b
>>> > --- /dev/null
>>> > +++ b/drivers/gpu/drm/vkms/vkms_display.c
>>> > @@ -0,0 +1,108 @@
>>> > +// SPDX-License-Identifier: GPL-2.0
>>> > +/*
>>> > + * This program is free software; you can redistribute it and/or modify
>>> > + * it under the terms of the GNU General Public License as published by
>>> > + * the Free Software Foundation; either version 2 of the License, or
>>> > + * (at your option) any later version.
>>> > + */
>>> > +
>>> > +#include "vkms_drv.h"
>>> > +#include <drm/drm_atomic_helper.h>
>>> > +#include <drm/drm_crtc_helper.h>
>>> > +
>>> > +#define XRES_MIN 32
>>> > +#define YRES_MIN 32
>>> > +
>>> > +#define XRES_DEF 1024
>>> > +#define YRES_DEF 768
>>>
>>> These seem unused.
>>
>> I created these defines because the documentation says:
>>
>> "[..]Once done, mode configuration must be setup by initializing the
>> following fields."
>> (https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.drm_mode_config_init)
>>
>> Finally, I used them in "mode_config" fields from drm_device (code below).
>>
>> Is it ok to not setup these values on mode_config? I looked at virtio as
>> an inspiration for this.
>
> XYRES_MIN and _MAX you need, but _DEF seems unused. That's why I
> asked, sorry for not making this clear.
btw could make sense to split this fix into a separate patch, since
the min/max setup is indeed missing and required.
-Daniel
> -Daniel
>
>>
>>> > +static const struct drm_mode_config_funcs vkms_mode_funcs = {
>>> > + .atomic_check = drm_atomic_helper_check,
>>> > + .atomic_commit = drm_atomic_helper_commit,
>>> > +};
>>> > +
>>> > +static const struct drm_crtc_funcs vkms_crtc_funcs = {
>>> > + .set_config = drm_atomic_helper_set_config,
>>> > + .destroy = drm_crtc_cleanup,
>>> > + .page_flip = drm_atomic_helper_page_flip,
>>> > + .reset = drm_atomic_helper_crtc_reset,
>>> > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>> > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> > +};
>>> > +
>>> > +static void vkms_connector_destroy(struct drm_connector *connector)
>>> > +{
>>> > + drm_connector_unregister(connector);
>>> > + drm_connector_cleanup(connector);
>>> > +}
>>> > +
>>> > +static const struct drm_connector_funcs vkms_connector_funcs = {
>>> > + .fill_modes = drm_helper_probe_single_connector_modes,
>>> > + .destroy = vkms_connector_destroy,
>>> > +};
>>> > +
>>> > +static int vkms_output_init(struct vkms_device *vkmsdev)
>>> > +{
>>> > + struct vkms_output *output = &vkmsdev->output;
>>> > + struct drm_device *dev = &vkmsdev->drm;
>>> > + struct drm_connector *connector = &output->connector;
>>> > + struct drm_crtc *crtc = &output->crtc;
>>> > + struct drm_plane *primary;
>>> > + int ret;
>>> > +
>>> > + primary = vkms_plane_init(vkmsdev);
>>> > + if (IS_ERR(primary))
>>> > + return PTR_ERR(primary);
>>> > +
>>> > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
>>> > + &vkms_crtc_funcs, NULL);
>>> > + if (ret) {
>>> > + DRM_ERROR("Failed to init CRTC\n");
>>> > + goto err_crtc;
>>> > + }
>>> > + primary->crtc = crtc;
>>>
>>> If you want to split stuff up a bit, I think putting the crtc stuff into
>>> vkms_crtc.c, and then renaming this file here to vkms_output.c would make
>>> sense.
>>
>> Nice, make a lot of sense to me. I will do it on v2.
>>
>>> > +
>>> > + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
>>> > + DRM_MODE_CONNECTOR_VIRTUAL);
>>> > + if (ret) {
>>> > + DRM_ERROR("Failed to init connector\n");
>>> > + goto err_connector;
>>> > + }
>>> > +
>>> > + ret = drm_connector_register(connector);
>>> > + if (ret) {
>>> > + DRM_ERROR("Failed to register connector\n");
>>> > + goto err_connector_register;
>>> > + }
>>> > +
>>> > + drm_mode_config_reset(dev);
>>> > +
>>> > + return 0;
>>> > +
>>> > +err_connector_register:
>>> > + drm_connector_cleanup(connector);
>>> > +
>>> > +err_connector:
>>> > + drm_crtc_cleanup(crtc);
>>> > +
>>> > +err_crtc:
>>> > + drm_plane_cleanup(primary);
>>> > + return ret;
>>> > +}
>>> > +
>>> > +int vkms_modeset_init(struct vkms_device *vkmsdev)
>>>
>>> Plus keeping vkms_modeset_init in vkms_drv.c, vkms is 100% about
>>> modesetting and nothing else, so splitting that out is a bit overkill imo.
>>
>> Ok, I will do it too for v2.
>>
>>> > +{
>>> > + struct drm_device *dev = &vkmsdev->drm;
>>> > +
>>> > + drm_mode_config_init(dev);
>>> > + dev->mode_config.funcs = &vkms_mode_funcs;
>>> > + dev->mode_config.min_width = XRES_MIN;
>>> > + dev->mode_config.min_height = YRES_MIN;
>>> > + dev->mode_config.max_width = XRES_MAX;
>>> > + dev->mode_config.max_height = YRES_MAX;
>>> > +
>>> > + return vkms_output_init(vkmsdev);
>>> > +}
>>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
>>> > index 35517b09538e..11e0569807bd 100644
>>> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
>>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>>> > @@ -6,9 +6,7 @@
>>> > */
>>> >
>>> > #include <linux/module.h>
>>> > -#include <drm/drmP.h>
>>> > #include <drm/drm_gem.h>
>>> > -#include <drm/drm_crtc_helper.h>
>>> > #include "vkms_drv.h"
>>> >
>>> > #define DRIVER_NAME "vkms"
>>> > @@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
>>> > .minor = DRIVER_MINOR,
>>> > };
>>> >
>>> > -static const u32 vkms_formats[] = {
>>> > - DRM_FORMAT_XRGB8888,
>>> > -};
>>> > -
>>> > -static void vkms_connector_destroy(struct drm_connector *connector)
>>> > -{
>>> > - drm_connector_unregister(connector);
>>> > - drm_connector_cleanup(connector);
>>> > -}
>>> > -
>>> > -static const struct drm_connector_funcs vkms_connector_funcs = {
>>> > - .fill_modes = drm_helper_probe_single_connector_modes,
>>> > - .destroy = vkms_connector_destroy,
>>> > -};
>>> > -
>>> > static int __init vkms_init(void)
>>> > {
>>> > int ret;
>>> > @@ -86,34 +69,14 @@ static int __init vkms_init(void)
>>> > goto out_fini;
>>> > }
>>> >
>>> > - drm_mode_config_init(&vkms_device->drm);
>>> > -
>>> > - ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
>>> > - &vkms_connector_funcs,
>>> > - DRM_MODE_CONNECTOR_VIRTUAL);
>>> > - if (ret < 0) {
>>> > - DRM_ERROR("Failed to init connector\n");
>>> > - goto out_unregister;
>>> > - }
>>> > -
>>> > - ret = drm_simple_display_pipe_init(&vkms_device->drm,
>>> > - &vkms_device->pipe,
>>> > - NULL,
>>> > - vkms_formats,
>>> > - ARRAY_SIZE(vkms_formats),
>>> > - NULL,
>>> > - &vkms_device->connector);
>>> > - if (ret < 0) {
>>> > - DRM_ERROR("Cannot setup simple display pipe\n");
>>> > + ret = vkms_modeset_init(vkms_device);
>>> > + if (ret)
>>> > goto out_unregister;
>>> > - }
>>> >
>>> > ret = drm_dev_register(&vkms_device->drm, 0);
>>> > if (ret)
>>> > goto out_unregister;
>>> >
>>> > - drm_connector_register(&vkms_device->connector);
>>> > -
>>> > return 0;
>>> >
>>> > out_unregister:
>>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>>> > index c77c5bf5032a..292bdea9c785 100644
>>> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
>>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>>> > @@ -1,13 +1,33 @@
>>> > #ifndef _VKMS_DRV_H_
>>> > #define _VKMS_DRV_H_
>>> >
>>> > -#include <drm/drm_simple_kms_helper.h>
>>> > +#include <drm/drmP.h>
>>> > +#include <drm/drm.h>
>>> > +
>>> > +static const u32 vkms_formats[] = {
>>> > + DRM_FORMAT_XRGB8888,
>>> > + DRM_FORMAT_ARGB8888,
>>> > + DRM_FORMAT_BGRX8888,
>>> > + DRM_FORMAT_BGRA8888,
>>> > + DRM_FORMAT_RGBX8888,
>>> > + DRM_FORMAT_RGBA8888,
>>> > + DRM_FORMAT_XBGR8888,
>>> > + DRM_FORMAT_ABGR8888,
>>>
>>> Why all these varianst? Right now we support none of this anyway ... Until
>>> we have more I think just limiting to XRBG8888 is good enough, we need to
>>> flesh out the crc support first (and backing memory handling too).
>>
>> My bad, I don't have any good argument for using all of this variants. I
>> will keep it simple and adds only XRBG8888.
>>
>> Finally, I will prepare the v2 with your recommendations and I will merge
>> the second patch (encoder) with this one.
>>
>> Thanks
>>
>>> -Daniel
>>>
>>> > +};
>>> > +
>>> > +struct vkms_output {
>>> > + struct drm_crtc crtc;
>>> > + struct drm_connector connector;
>>> > +};
>>> >
>>> > struct vkms_device {
>>> > struct drm_device drm;
>>> > struct platform_device *platform;
>>> > - struct drm_simple_display_pipe pipe;
>>> > - struct drm_connector connector;
>>> > + struct vkms_output output;
>>> > };
>>> >
>>> > +int vkms_modeset_init(struct vkms_device *vkmsdev);
>>> > +
>>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
>>> > +
>>> > #endif /* _VKMS_DRV_H_ */
>>> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>> > new file mode 100644
>>> > index 000000000000..2c25b1d6ab5b
>>> > --- /dev/null
>>> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>> > @@ -0,0 +1,46 @@
>>> > +// SPDX-License-Identifier: GPL-2.0
>>> > +/*
>>> > + * This program is free software; you can redistribute it and/or modify
>>> > + * it under the terms of the GNU General Public License as published by
>>> > + * the Free Software Foundation; either version 2 of the License, or
>>> > + * (at your option) any later version.
>>> > + */
>>> > +
>>> > +#include "vkms_drv.h"
>>> > +#include <drm/drm_plane_helper.h>
>>> > +#include <drm/drm_atomic_helper.h>
>>> > +
>>> > +static const struct drm_plane_funcs vkms_plane_funcs = {
>>> > + .update_plane = drm_atomic_helper_update_plane,
>>> > + .disable_plane = drm_atomic_helper_disable_plane,
>>> > + .destroy = drm_plane_cleanup,
>>> > + .reset = drm_atomic_helper_plane_reset,
>>> > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>> > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>>> > +};
>>> > +
>>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
>>> > +{
>>> > + struct drm_device *dev = &vkmsdev->drm;
>>> > + struct drm_plane *plane;
>>> > + const u32 *formats;
>>> > + int ret, nformats;
>>> > +
>>> > + plane = kzalloc(sizeof(*plane), GFP_KERNEL);
>>> > + if (!plane)
>>> > + return ERR_PTR(-ENOMEM);
>>> > +
>>> > + formats = vkms_formats;
>>> > + nformats = ARRAY_SIZE(vkms_formats);
>>> > +
>>> > + ret = drm_universal_plane_init(dev, plane, 0,
>>> > + &vkms_plane_funcs,
>>> > + formats, nformats,
>>> > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>>> > + if (ret) {
>>> > + kfree(plane);
>>> > + return ERR_PTR(ret);
>>> > + }
>>> > +
>>> > + return plane;
>>> > +}
>>> > --
>>> > 2.17.0
>>> >
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
2018-05-16 15:41 ` Daniel Vetter
@ 2018-05-16 15:49 ` Rodrigo Siqueira
0 siblings, 0 replies; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-05-16 15:49 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel, Sean Paul, Haneen Mohammed
Now I got it! I will split the patch and apply your suggestions :)
Thanks
On 05/16, Daniel Vetter wrote:
> On Wed, May 16, 2018 at 5:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, May 16, 2018 at 4:51 PM, Rodrigo Siqueira
> > <rodrigosiqueiramelo@gmail.com> wrote:
> >> Hi Daniel,
> >>
> >> Thanks for the feedback :)
> >>
> >> You can find my comments below:
> >>
> >> On 05/16, Daniel Vetter wrote:
> >>> On Wed, May 16, 2018 at 12:06:54AM -0300, Rodrigo Siqueira wrote:
> >>> > This commit adds the essential infrastructure for managing CRTCs which
> >>> > is composed of: a new data struct for output data information, a
> >>> > function for basic modeset initialization, and the operation to create
> >>> > planes. Due to the introduction of a new initialization function,
> >>> > connectors were moved from vkms_drv.c to vkms_display.c.
> >>> >
> >>> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >>> > ---
> >>> > drivers/gpu/drm/vkms/Makefile | 2 +-
> >>> > drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
> >>> > drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
> >>> > drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
> >>> > drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
> >>> > 5 files changed, 180 insertions(+), 43 deletions(-)
> >>> > create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
> >>> > create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
> >>> >
> >>> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> >>> > index 2aef948d3a34..f747e2a3a6f4 100644
> >>> > --- a/drivers/gpu/drm/vkms/Makefile
> >>> > +++ b/drivers/gpu/drm/vkms/Makefile
> >>> > @@ -1,3 +1,3 @@
> >>> > -vkms-y := vkms_drv.o
> >>> > +vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
> >>> >
> >>> > obj-$(CONFIG_DRM_VKMS) += vkms.o
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
> >>> > new file mode 100644
> >>> > index 000000000000..b20b41f9590b
> >>> > --- /dev/null
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_display.c
> >>> > @@ -0,0 +1,108 @@
> >>> > +// SPDX-License-Identifier: GPL-2.0
> >>> > +/*
> >>> > + * This program is free software; you can redistribute it and/or modify
> >>> > + * it under the terms of the GNU General Public License as published by
> >>> > + * the Free Software Foundation; either version 2 of the License, or
> >>> > + * (at your option) any later version.
> >>> > + */
> >>> > +
> >>> > +#include "vkms_drv.h"
> >>> > +#include <drm/drm_atomic_helper.h>
> >>> > +#include <drm/drm_crtc_helper.h>
> >>> > +
> >>> > +#define XRES_MIN 32
> >>> > +#define YRES_MIN 32
> >>> > +
> >>> > +#define XRES_DEF 1024
> >>> > +#define YRES_DEF 768
> >>>
> >>> These seem unused.
> >>
> >> I created these defines because the documentation says:
> >>
> >> "[..]Once done, mode configuration must be setup by initializing the
> >> following fields."
> >> (https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.drm_mode_config_init)
> >>
> >> Finally, I used them in "mode_config" fields from drm_device (code below).
> >>
> >> Is it ok to not setup these values on mode_config? I looked at virtio as
> >> an inspiration for this.
> >
> > XYRES_MIN and _MAX you need, but _DEF seems unused. That's why I
> > asked, sorry for not making this clear.
>
> btw could make sense to split this fix into a separate patch, since
> the min/max setup is indeed missing and required.
> -Daniel
>
> > -Daniel
> >
> >>
> >>> > +static const struct drm_mode_config_funcs vkms_mode_funcs = {
> >>> > + .atomic_check = drm_atomic_helper_check,
> >>> > + .atomic_commit = drm_atomic_helper_commit,
> >>> > +};
> >>> > +
> >>> > +static const struct drm_crtc_funcs vkms_crtc_funcs = {
> >>> > + .set_config = drm_atomic_helper_set_config,
> >>> > + .destroy = drm_crtc_cleanup,
> >>> > + .page_flip = drm_atomic_helper_page_flip,
> >>> > + .reset = drm_atomic_helper_crtc_reset,
> >>> > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> >>> > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> >>> > +};
> >>> > +
> >>> > +static void vkms_connector_destroy(struct drm_connector *connector)
> >>> > +{
> >>> > + drm_connector_unregister(connector);
> >>> > + drm_connector_cleanup(connector);
> >>> > +}
> >>> > +
> >>> > +static const struct drm_connector_funcs vkms_connector_funcs = {
> >>> > + .fill_modes = drm_helper_probe_single_connector_modes,
> >>> > + .destroy = vkms_connector_destroy,
> >>> > +};
> >>> > +
> >>> > +static int vkms_output_init(struct vkms_device *vkmsdev)
> >>> > +{
> >>> > + struct vkms_output *output = &vkmsdev->output;
> >>> > + struct drm_device *dev = &vkmsdev->drm;
> >>> > + struct drm_connector *connector = &output->connector;
> >>> > + struct drm_crtc *crtc = &output->crtc;
> >>> > + struct drm_plane *primary;
> >>> > + int ret;
> >>> > +
> >>> > + primary = vkms_plane_init(vkmsdev);
> >>> > + if (IS_ERR(primary))
> >>> > + return PTR_ERR(primary);
> >>> > +
> >>> > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
> >>> > + &vkms_crtc_funcs, NULL);
> >>> > + if (ret) {
> >>> > + DRM_ERROR("Failed to init CRTC\n");
> >>> > + goto err_crtc;
> >>> > + }
> >>> > + primary->crtc = crtc;
> >>>
> >>> If you want to split stuff up a bit, I think putting the crtc stuff into
> >>> vkms_crtc.c, and then renaming this file here to vkms_output.c would make
> >>> sense.
> >>
> >> Nice, make a lot of sense to me. I will do it on v2.
> >>
> >>> > +
> >>> > + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> >>> > + DRM_MODE_CONNECTOR_VIRTUAL);
> >>> > + if (ret) {
> >>> > + DRM_ERROR("Failed to init connector\n");
> >>> > + goto err_connector;
> >>> > + }
> >>> > +
> >>> > + ret = drm_connector_register(connector);
> >>> > + if (ret) {
> >>> > + DRM_ERROR("Failed to register connector\n");
> >>> > + goto err_connector_register;
> >>> > + }
> >>> > +
> >>> > + drm_mode_config_reset(dev);
> >>> > +
> >>> > + return 0;
> >>> > +
> >>> > +err_connector_register:
> >>> > + drm_connector_cleanup(connector);
> >>> > +
> >>> > +err_connector:
> >>> > + drm_crtc_cleanup(crtc);
> >>> > +
> >>> > +err_crtc:
> >>> > + drm_plane_cleanup(primary);
> >>> > + return ret;
> >>> > +}
> >>> > +
> >>> > +int vkms_modeset_init(struct vkms_device *vkmsdev)
> >>>
> >>> Plus keeping vkms_modeset_init in vkms_drv.c, vkms is 100% about
> >>> modesetting and nothing else, so splitting that out is a bit overkill imo.
> >>
> >> Ok, I will do it too for v2.
> >>
> >>> > +{
> >>> > + struct drm_device *dev = &vkmsdev->drm;
> >>> > +
> >>> > + drm_mode_config_init(dev);
> >>> > + dev->mode_config.funcs = &vkms_mode_funcs;
> >>> > + dev->mode_config.min_width = XRES_MIN;
> >>> > + dev->mode_config.min_height = YRES_MIN;
> >>> > + dev->mode_config.max_width = XRES_MAX;
> >>> > + dev->mode_config.max_height = YRES_MAX;
> >>> > +
> >>> > + return vkms_output_init(vkmsdev);
> >>> > +}
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> >>> > index 35517b09538e..11e0569807bd 100644
> >>> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> >>> > @@ -6,9 +6,7 @@
> >>> > */
> >>> >
> >>> > #include <linux/module.h>
> >>> > -#include <drm/drmP.h>
> >>> > #include <drm/drm_gem.h>
> >>> > -#include <drm/drm_crtc_helper.h>
> >>> > #include "vkms_drv.h"
> >>> >
> >>> > #define DRIVER_NAME "vkms"
> >>> > @@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
> >>> > .minor = DRIVER_MINOR,
> >>> > };
> >>> >
> >>> > -static const u32 vkms_formats[] = {
> >>> > - DRM_FORMAT_XRGB8888,
> >>> > -};
> >>> > -
> >>> > -static void vkms_connector_destroy(struct drm_connector *connector)
> >>> > -{
> >>> > - drm_connector_unregister(connector);
> >>> > - drm_connector_cleanup(connector);
> >>> > -}
> >>> > -
> >>> > -static const struct drm_connector_funcs vkms_connector_funcs = {
> >>> > - .fill_modes = drm_helper_probe_single_connector_modes,
> >>> > - .destroy = vkms_connector_destroy,
> >>> > -};
> >>> > -
> >>> > static int __init vkms_init(void)
> >>> > {
> >>> > int ret;
> >>> > @@ -86,34 +69,14 @@ static int __init vkms_init(void)
> >>> > goto out_fini;
> >>> > }
> >>> >
> >>> > - drm_mode_config_init(&vkms_device->drm);
> >>> > -
> >>> > - ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
> >>> > - &vkms_connector_funcs,
> >>> > - DRM_MODE_CONNECTOR_VIRTUAL);
> >>> > - if (ret < 0) {
> >>> > - DRM_ERROR("Failed to init connector\n");
> >>> > - goto out_unregister;
> >>> > - }
> >>> > -
> >>> > - ret = drm_simple_display_pipe_init(&vkms_device->drm,
> >>> > - &vkms_device->pipe,
> >>> > - NULL,
> >>> > - vkms_formats,
> >>> > - ARRAY_SIZE(vkms_formats),
> >>> > - NULL,
> >>> > - &vkms_device->connector);
> >>> > - if (ret < 0) {
> >>> > - DRM_ERROR("Cannot setup simple display pipe\n");
> >>> > + ret = vkms_modeset_init(vkms_device);
> >>> > + if (ret)
> >>> > goto out_unregister;
> >>> > - }
> >>> >
> >>> > ret = drm_dev_register(&vkms_device->drm, 0);
> >>> > if (ret)
> >>> > goto out_unregister;
> >>> >
> >>> > - drm_connector_register(&vkms_device->connector);
> >>> > -
> >>> > return 0;
> >>> >
> >>> > out_unregister:
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >>> > index c77c5bf5032a..292bdea9c785 100644
> >>> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >>> > @@ -1,13 +1,33 @@
> >>> > #ifndef _VKMS_DRV_H_
> >>> > #define _VKMS_DRV_H_
> >>> >
> >>> > -#include <drm/drm_simple_kms_helper.h>
> >>> > +#include <drm/drmP.h>
> >>> > +#include <drm/drm.h>
> >>> > +
> >>> > +static const u32 vkms_formats[] = {
> >>> > + DRM_FORMAT_XRGB8888,
> >>> > + DRM_FORMAT_ARGB8888,
> >>> > + DRM_FORMAT_BGRX8888,
> >>> > + DRM_FORMAT_BGRA8888,
> >>> > + DRM_FORMAT_RGBX8888,
> >>> > + DRM_FORMAT_RGBA8888,
> >>> > + DRM_FORMAT_XBGR8888,
> >>> > + DRM_FORMAT_ABGR8888,
> >>>
> >>> Why all these varianst? Right now we support none of this anyway ... Until
> >>> we have more I think just limiting to XRBG8888 is good enough, we need to
> >>> flesh out the crc support first (and backing memory handling too).
> >>
> >> My bad, I don't have any good argument for using all of this variants. I
> >> will keep it simple and adds only XRBG8888.
> >>
> >> Finally, I will prepare the v2 with your recommendations and I will merge
> >> the second patch (encoder) with this one.
> >>
> >> Thanks
> >>
> >>> -Daniel
> >>>
> >>> > +};
> >>> > +
> >>> > +struct vkms_output {
> >>> > + struct drm_crtc crtc;
> >>> > + struct drm_connector connector;
> >>> > +};
> >>> >
> >>> > struct vkms_device {
> >>> > struct drm_device drm;
> >>> > struct platform_device *platform;
> >>> > - struct drm_simple_display_pipe pipe;
> >>> > - struct drm_connector connector;
> >>> > + struct vkms_output output;
> >>> > };
> >>> >
> >>> > +int vkms_modeset_init(struct vkms_device *vkmsdev);
> >>> > +
> >>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
> >>> > +
> >>> > #endif /* _VKMS_DRV_H_ */
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> >>> > new file mode 100644
> >>> > index 000000000000..2c25b1d6ab5b
> >>> > --- /dev/null
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> >>> > @@ -0,0 +1,46 @@
> >>> > +// SPDX-License-Identifier: GPL-2.0
> >>> > +/*
> >>> > + * This program is free software; you can redistribute it and/or modify
> >>> > + * it under the terms of the GNU General Public License as published by
> >>> > + * the Free Software Foundation; either version 2 of the License, or
> >>> > + * (at your option) any later version.
> >>> > + */
> >>> > +
> >>> > +#include "vkms_drv.h"
> >>> > +#include <drm/drm_plane_helper.h>
> >>> > +#include <drm/drm_atomic_helper.h>
> >>> > +
> >>> > +static const struct drm_plane_funcs vkms_plane_funcs = {
> >>> > + .update_plane = drm_atomic_helper_update_plane,
> >>> > + .disable_plane = drm_atomic_helper_disable_plane,
> >>> > + .destroy = drm_plane_cleanup,
> >>> > + .reset = drm_atomic_helper_plane_reset,
> >>> > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >>> > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> >>> > +};
> >>> > +
> >>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
> >>> > +{
> >>> > + struct drm_device *dev = &vkmsdev->drm;
> >>> > + struct drm_plane *plane;
> >>> > + const u32 *formats;
> >>> > + int ret, nformats;
> >>> > +
> >>> > + plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> >>> > + if (!plane)
> >>> > + return ERR_PTR(-ENOMEM);
> >>> > +
> >>> > + formats = vkms_formats;
> >>> > + nformats = ARRAY_SIZE(vkms_formats);
> >>> > +
> >>> > + ret = drm_universal_plane_init(dev, plane, 0,
> >>> > + &vkms_plane_funcs,
> >>> > + formats, nformats,
> >>> > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> >>> > + if (ret) {
> >>> > + kfree(plane);
> >>> > + return ERR_PTR(ret);
> >>> > + }
> >>> > +
> >>> > + return plane;
> >>> > +}
> >>> > --
> >>> > 2.17.0
> >>> >
> >>>
> >>> --
> >>> Daniel Vetter
> >>> Software Engineer, Intel Corporation
> >>> http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-16 15:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-16 3:06 [PATCH 0/3] Expanding the basic vkms features Rodrigo Siqueira
2018-05-16 3:06 ` [PATCH 1/3] drm/vkms: Add basic CRTC initialization Rodrigo Siqueira
2018-05-16 9:51 ` Daniel Vetter
2018-05-16 14:51 ` Rodrigo Siqueira
2018-05-16 15:40 ` Daniel Vetter
2018-05-16 15:41 ` Daniel Vetter
2018-05-16 15:49 ` Rodrigo Siqueira
2018-05-16 3:07 ` [PATCH 2/3] drm/vkms: Add encoder initialization Rodrigo Siqueira
2018-05-16 9:45 ` Daniel Vetter
2018-05-16 3:07 ` [PATCH 3/3] drm/vkms: Add extra information about vkms Rodrigo Siqueira
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.