Expanding the basic vkms features
cover Expanding the basic vkms features
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
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/3 drm/vkms: Add basic CRTC initialization
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 <[email protected]>
--- 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
drivers/gpu/drm/vkms/Makefile +1 -1
@@ -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
drivers/gpu/drm/vkms/vkms_display.c new file +108 -0
108 lines not on this page · raw message
drivers/gpu/drm/vkms/vkms_drv.c +2 -39
66 lines not on this page · raw message
drivers/gpu/drm/vkms/vkms_drv.h +23 -3
@@ -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_ */
drivers/gpu/drm/vkms/vkms_plane.c new file +46 -0
46 lines not on this page · raw message
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
Quoted 39 lines · Rodrigo Siqueira
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.ctovkms_display.c.Signed-off-by Rodrigo Siqueira <[email protected]>--- 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.cdrivers/gpu/drm/vkms/Makefile +1
-1@@ -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.odrivers/gpu/drm/vkms/vkms_display.c new file +7
-0@@ -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. +*/
drivers/gpu/drm/vkms/vkms_display.c line 17
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_display.c new file +10
-0+ +#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.
Quoted 38 lines
drivers/gpu/drm/vkms/vkms_display.c new file +38
-0+#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);
drivers/gpu/drm/vkms/vkms_display.c line 66
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_display.c new file +10
-0+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.
Quoted 20 lines
drivers/gpu/drm/vkms/vkms_display.c new file +20
-0+ + 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);
drivers/gpu/drm/vkms/vkms_display.c line 96
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_display.c new file +10
-0+ +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.
Quoted 96 lines
drivers/gpu/drm/vkms/vkms_display.c new file +12
-0+{ +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); +}drivers/gpu/drm/vkms/vkms_drv.c +2
-39@@ -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:drivers/gpu/drm/vkms/vkms_drv.h +2
-1@@ -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>
drivers/gpu/drm/vkms/vkms_drv.h line 15
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_drv.h +10
-0+ +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
Quoted 75 lines
drivers/gpu/drm/vkms/vkms_drv.h +11
-2+}; + +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_ */ drivers/gpu/drm/vkms/vkms_plane.c new file +46
-0@@ -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; +}Signature · 1 line
2.17.0
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
Hi Daniel,
Thanks for the feedback :)
You can find my comments below:
Quoted 44 lines · Daniel Vetter
Quoted 43 lines · Rodrigo Siqueira
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.ctovkms_display.c.Signed-off-by Rodrigo Siqueira <[email protected]>--- 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.cdrivers/gpu/drm/vkms/Makefile +1
-1@@ -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.odrivers/gpu/drm/vkms/vkms_display.c new file +11
-0@@ -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>
Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 17
Quoted 6 lines
drivers/gpu/drm/vkms/vkms_display.c new file +6
-0+ +#define XRES_MIN 32 +#define YRES_MIN 32 + +#define XRES_DEF 1024 +#define YRES_DEF 768These 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.
Quoted 41 lines
Quoted 41 lines
drivers/gpu/drm/vkms/vkms_display.c new file +41
-0+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) {
Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 66
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_display.c new file +4
-0+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 tovkms_output.cwould make sense.
Nice, make a lot of sense to me. I will do it on v2.
Quoted 25 lines
Quoted 25 lines
drivers/gpu/drm/vkms/vkms_display.c new file +25
-0+ + 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:
Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 96
Quoted 5 lines
drivers/gpu/drm/vkms/vkms_display.c new file +5
-0+drm_plane_cleanup (primary); +return ret; +} + +int vkms_modeset_init (struct vkms_device *vkmsdev)Plus keeping
vkms_modeset_initinvkms_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.
Quoted 102 lines
Quoted 102 lines
drivers/gpu/drm/vkms/vkms_display.c new file +12
-0+{ +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); +}drivers/gpu/drm/vkms/vkms_drv.c +2
-39@@ -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:drivers/gpu/drm/vkms/vkms_drv.h +8
-1@@ -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,
Quoted 8 lines
drivers/gpu/drm/vkms/vkms_drv.h line 15
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_drv.h +4
-0+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
Quoted 69 lines
-Daniel
Quoted 67 lines
drivers/gpu/drm/vkms/vkms_drv.h +11
-2+}; + +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_ */ drivers/gpu/drm/vkms/vkms_plane.c new file +41
-0@@ -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);
Quoted 13 lines
Quoted 8 lines
drivers/gpu/drm/vkms/vkms_plane.c new file +5
-0+return ERR_PTR (ret); + } + +return plane; +}Signature · 1 line
2.17.0Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
Quoted 62 lines · Rodrigo Siqueira
Hi Daniel,
Thanks for the feedback :)
You can find my comments below:
Quoted 44 lines · Daniel Vetter
Quoted 43 lines · Rodrigo Siqueira
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.ctovkms_display.c.Signed-off-by Rodrigo Siqueira <[email protected]>--- 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.cdrivers/gpu/drm/vkms/Makefile +1
-1@@ -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.odrivers/gpu/drm/vkms/vkms_display.c new file +11
-0@@ -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> Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 17
Quoted 6 lines
drivers/gpu/drm/vkms/vkms_display.c new file +6
-0+ +#define XRES_MIN 32 +#define YRES_MIN 32 + +#define XRES_DEF 1024 +#define YRES_DEF 768These seem unused.
I created these defines because the documentation says:
Quoted 8 lines
"[..]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 fromdrm_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
Quoted 290 lines
Quoted 42 lines
Quoted 42 lines
drivers/gpu/drm/vkms/vkms_display.c new file +41
-0+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) {Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 66
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_display.c new file +4
-0+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 tovkms_output.cwould make sense.Nice, make a lot of sense to me. I will do it on v2.
Quoted 25 lines
Quoted 25 lines
drivers/gpu/drm/vkms/vkms_display.c new file +25
-0+ + 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:Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 96
Quoted 5 lines
drivers/gpu/drm/vkms/vkms_display.c new file +5
-0+drm_plane_cleanup (primary); +return ret; +} + +int vkms_modeset_init (struct vkms_device *vkmsdev)Plus keeping
vkms_modeset_initinvkms_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.
Quoted 102 lines
Quoted 102 lines
drivers/gpu/drm/vkms/vkms_display.c new file +12
-0+{ +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); +}drivers/gpu/drm/vkms/vkms_drv.c +2
-39@@ -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:drivers/gpu/drm/vkms/vkms_drv.h +8
-1@@ -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,Quoted 8 lines
drivers/gpu/drm/vkms/vkms_drv.h line 15
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_drv.h +4
-0+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
Quoted 82 lines
-Daniel
Quoted 73 lines
drivers/gpu/drm/vkms/vkms_drv.h +11
-2+}; + +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_ */ drivers/gpu/drm/vkms/vkms_plane.c new file +46
-1@@ -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; +} --Quoted 2 lines
2.17.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
Quoted 68 lines · Daniel Vetter
Quoted 66 lines · Rodrigo Siqueira
Hi Daniel,
Thanks for the feedback :)
You can find my comments below:
Quoted 44 lines · Daniel Vetter
Quoted 43 lines · Rodrigo Siqueira
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.ctovkms_display.c.Signed-off-by Rodrigo Siqueira <[email protected]>--- 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.cdrivers/gpu/drm/vkms/Makefile +1
-1@@ -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.odrivers/gpu/drm/vkms/vkms_display.c new file +11
-0@@ -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> Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 17
Quoted 6 lines
drivers/gpu/drm/vkms/vkms_display.c new file +6
-0+ +#define XRES_MIN 32 +#define YRES_MIN 32 + +#define XRES_DEF 1024 +#define YRES_DEF 768These 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)
Quoted 7 lines
Quoted 4 lines
Finally, I used them in "
mode_config" fields fromdrm_device(code below).Is it ok to not setup these values on
mode_config? I looked at virtio as an inspiration for this.
XYRES_MINand_MAXyou need, but_DEFseems 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
Quoted 299 lines
-Daniel
Quoted 200 lines
Quoted 42 lines
Quoted 42 lines
drivers/gpu/drm/vkms/vkms_display.c new file +41
-0+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) {Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 66
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_display.c new file +4
-0+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 tovkms_output.cwould make sense.Nice, make a lot of sense to me. I will do it on v2.
Quoted 25 lines
Quoted 25 lines
drivers/gpu/drm/vkms/vkms_display.c new file +25
-0+ + 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:Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 96
Quoted 5 lines
drivers/gpu/drm/vkms/vkms_display.c new file +5
-0+drm_plane_cleanup (primary); +return ret; +} + +int vkms_modeset_init (struct vkms_device *vkmsdev)Plus keeping
vkms_modeset_initinvkms_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.
Quoted 102 lines
Quoted 102 lines
drivers/gpu/drm/vkms/vkms_display.c new file +12
-0+{ +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); +}drivers/gpu/drm/vkms/vkms_drv.c +2
-39@@ -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:drivers/gpu/drm/vkms/vkms_drv.h +8
-1@@ -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,Quoted 8 lines
drivers/gpu/drm/vkms/vkms_drv.h line 15
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_drv.h +4
-0+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).
Quoted 90 lines
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
Quoted 82 lines
-Daniel
Quoted 73 lines
drivers/gpu/drm/vkms/vkms_drv.h +11
-2+}; + +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_ */ drivers/gpu/drm/vkms/vkms_plane.c new file +46
-1@@ -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; +} --Quoted 2 lines
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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
Now I got it! I will split the patch and apply your suggestions :) Thanks
Quoted 376 lines · Daniel Vetter
Quoted 68 lines · Daniel Vetter
Quoted 66 lines · Rodrigo Siqueira
Hi Daniel,
Thanks for the feedback :)
You can find my comments below:
Quoted 44 lines · Daniel Vetter
Quoted 43 lines · Rodrigo Siqueira
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.ctovkms_display.c.Signed-off-by Rodrigo Siqueira <[email protected]>--- 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.cdrivers/gpu/drm/vkms/Makefile +1
-1@@ -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.odrivers/gpu/drm/vkms/vkms_display.c new file +11
-0@@ -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> Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 17
Quoted 6 lines
drivers/gpu/drm/vkms/vkms_display.c new file +6
-0+ +#define XRES_MIN 32 +#define YRES_MIN 32 + +#define XRES_DEF 1024 +#define YRES_DEF 768These 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)Quoted 7 lines
Quoted 4 lines
Finally, I used them in "
mode_config" fields fromdrm_device(code below).Is it ok to not setup these values on
mode_config? I looked at virtio as an inspiration for this.
XYRES_MINand_MAXyou need, but_DEFseems 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
Quoted 295 lines
-Daniel
Quoted 290 lines
Quoted 42 lines
Quoted 42 lines
drivers/gpu/drm/vkms/vkms_display.c new file +41
-0+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) {Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 66
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_display.c new file +4
-0+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 tovkms_output.cwould make sense.Nice, make a lot of sense to me. I will do it on v2.
Quoted 25 lines
Quoted 25 lines
drivers/gpu/drm/vkms/vkms_display.c new file +25
-0+ + 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:Quoted 8 lines
drivers/gpu/drm/vkms/vkms_display.c line 96
Quoted 5 lines
drivers/gpu/drm/vkms/vkms_display.c new file +5
-0+drm_plane_cleanup (primary); +return ret; +} + +int vkms_modeset_init (struct vkms_device *vkmsdev)Plus keeping
vkms_modeset_initinvkms_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.
Quoted 102 lines
Quoted 102 lines
drivers/gpu/drm/vkms/vkms_display.c new file +12
-0+{ +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); +}drivers/gpu/drm/vkms/vkms_drv.c +2
-39@@ -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:drivers/gpu/drm/vkms/vkms_drv.h +8
-1@@ -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,Quoted 8 lines
drivers/gpu/drm/vkms/vkms_drv.h line 15
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_drv.h +4
-0+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
Quoted 82 lines
-Daniel
Quoted 73 lines
drivers/gpu/drm/vkms/vkms_drv.h +11
-2+}; + +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_ */ drivers/gpu/drm/vkms/vkms_plane.c new file +46
-1@@ -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; +} --Quoted 2 lines
2.17.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Quoted 11 lines
Quoted 4 lines
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.chSignature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/3 drm/vkms: Add encoder initialization
In reply to Rodrigo SiqueiraThis commit adds a single and simple virtual encoder to VKMS.
Signed-off-by Rodrigo Siqueira <[email protected]>
--- drivers/gpu/drm/vkms/vkms_display.c | 25 +++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.h | 2 ++ 2 files changed, 27 insertions(+)
drivers/gpu/drm/vkms/vkms_display.c +25 -0
@@ -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);
drivers/gpu/drm/vkms/vkms_drv.h +2 -0
@@ -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; };
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/vkms: Add encoder initialization
Quoted 3 lines · Rodrigo Siqueira
This commit adds a single and simple virtual encoder to VKMS.
Signed-off-by Rodrigo Siqueira <[email protected]>
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
Quoted 80 lines
--- drivers/gpu/drm/vkms/vkms_display.c | 25 +++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.h | 2 ++ 2 files changed, 27 insertions(+)drivers/gpu/drm/vkms/vkms_display.c +25
-0@@ -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);drivers/gpu/drm/vkms/vkms_drv.h +2
-0@@ -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; };Signature · 1 line
2.17.0
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 3/3 drm/vkms: Add extra information about vkms
In reply to Rodrigo SiqueiraAdd the following additional information: authors and description in Kconfig.
Signed-off-by Rodrigo Siqueira <[email protected]>
--- drivers/gpu/drm/Kconfig | 8 ++++++-- drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-)
drivers/gpu/drm/Kconfig +6 -2
@@ -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"
drivers/gpu/drm/vkms/vkms_drv.c +2 -0
@@ -105,5 +105,7 @@ static void __exit vkms_exit(void)module_init (vkms_init);module_exit (vkms_exit); +MODULE_AUTHOR ("Haneen Mohammed <[email protected]>" ); +MODULE_AUTHOR ("Rodrigo Siqueira <[email protected]>" );MODULE_DESCRIPTION (DRIVER_DESC );MODULE_LICENSE ("GPL" );
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel