| Message ID | ad6b3fe82bd02af5d005337c6414d0e583172503.1526439491.git.rodrigosiqueiramelo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
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
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; +}
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