From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
Sean Paul <seanpaul@google.com>,
Haneen Mohammed <hamohammed.sa@gmail.com>
Subject: Re: [PATCH 1/3] drm/vkms: Add basic CRTC initialization
Date: Wed, 16 May 2018 12:49:06 -0300 [thread overview]
Message-ID: <20180516154906.gfmwndvzdhu2ycai@smtp.gmail.com> (raw)
In-Reply-To: <CAKMK7uHor9ooiT+xk7H8Hp4eUnZ2dQiUQL5auQpUHMbEv2i7qg@mail.gmail.com>
Now I got it! I will split the patch and apply your suggestions :)
Thanks
On 05/16, Daniel Vetter wrote:
> On Wed, May 16, 2018 at 5:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, May 16, 2018 at 4:51 PM, Rodrigo Siqueira
> > <rodrigosiqueiramelo@gmail.com> wrote:
> >> Hi Daniel,
> >>
> >> Thanks for the feedback :)
> >>
> >> You can find my comments below:
> >>
> >> On 05/16, Daniel Vetter wrote:
> >>> On Wed, May 16, 2018 at 12:06:54AM -0300, Rodrigo Siqueira wrote:
> >>> > This commit adds the essential infrastructure for managing CRTCs which
> >>> > is composed of: a new data struct for output data information, a
> >>> > function for basic modeset initialization, and the operation to create
> >>> > planes. Due to the introduction of a new initialization function,
> >>> > connectors were moved from vkms_drv.c to vkms_display.c.
> >>> >
> >>> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >>> > ---
> >>> > drivers/gpu/drm/vkms/Makefile | 2 +-
> >>> > drivers/gpu/drm/vkms/vkms_display.c | 108 ++++++++++++++++++++++++++++
> >>> > drivers/gpu/drm/vkms/vkms_drv.c | 41 +----------
> >>> > drivers/gpu/drm/vkms/vkms_drv.h | 26 ++++++-
> >>> > drivers/gpu/drm/vkms/vkms_plane.c | 46 ++++++++++++
> >>> > 5 files changed, 180 insertions(+), 43 deletions(-)
> >>> > create mode 100644 drivers/gpu/drm/vkms/vkms_display.c
> >>> > create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
> >>> >
> >>> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> >>> > index 2aef948d3a34..f747e2a3a6f4 100644
> >>> > --- a/drivers/gpu/drm/vkms/Makefile
> >>> > +++ b/drivers/gpu/drm/vkms/Makefile
> >>> > @@ -1,3 +1,3 @@
> >>> > -vkms-y := vkms_drv.o
> >>> > +vkms-y := vkms_drv.o vkms_plane.o vkms_display.o
> >>> >
> >>> > obj-$(CONFIG_DRM_VKMS) += vkms.o
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_display.c b/drivers/gpu/drm/vkms/vkms_display.c
> >>> > new file mode 100644
> >>> > index 000000000000..b20b41f9590b
> >>> > --- /dev/null
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_display.c
> >>> > @@ -0,0 +1,108 @@
> >>> > +// SPDX-License-Identifier: GPL-2.0
> >>> > +/*
> >>> > + * This program is free software; you can redistribute it and/or modify
> >>> > + * it under the terms of the GNU General Public License as published by
> >>> > + * the Free Software Foundation; either version 2 of the License, or
> >>> > + * (at your option) any later version.
> >>> > + */
> >>> > +
> >>> > +#include "vkms_drv.h"
> >>> > +#include <drm/drm_atomic_helper.h>
> >>> > +#include <drm/drm_crtc_helper.h>
> >>> > +
> >>> > +#define XRES_MIN 32
> >>> > +#define YRES_MIN 32
> >>> > +
> >>> > +#define XRES_DEF 1024
> >>> > +#define YRES_DEF 768
> >>>
> >>> These seem unused.
> >>
> >> I created these defines because the documentation says:
> >>
> >> "[..]Once done, mode configuration must be setup by initializing the
> >> following fields."
> >> (https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.drm_mode_config_init)
> >>
> >> Finally, I used them in "mode_config" fields from drm_device (code below).
> >>
> >> Is it ok to not setup these values on mode_config? I looked at virtio as
> >> an inspiration for this.
> >
> > XYRES_MIN and _MAX you need, but _DEF seems unused. That's why I
> > asked, sorry for not making this clear.
>
> btw could make sense to split this fix into a separate patch, since
> the min/max setup is indeed missing and required.
> -Daniel
>
> > -Daniel
> >
> >>
> >>> > +static const struct drm_mode_config_funcs vkms_mode_funcs = {
> >>> > + .atomic_check = drm_atomic_helper_check,
> >>> > + .atomic_commit = drm_atomic_helper_commit,
> >>> > +};
> >>> > +
> >>> > +static const struct drm_crtc_funcs vkms_crtc_funcs = {
> >>> > + .set_config = drm_atomic_helper_set_config,
> >>> > + .destroy = drm_crtc_cleanup,
> >>> > + .page_flip = drm_atomic_helper_page_flip,
> >>> > + .reset = drm_atomic_helper_crtc_reset,
> >>> > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> >>> > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> >>> > +};
> >>> > +
> >>> > +static void vkms_connector_destroy(struct drm_connector *connector)
> >>> > +{
> >>> > + drm_connector_unregister(connector);
> >>> > + drm_connector_cleanup(connector);
> >>> > +}
> >>> > +
> >>> > +static const struct drm_connector_funcs vkms_connector_funcs = {
> >>> > + .fill_modes = drm_helper_probe_single_connector_modes,
> >>> > + .destroy = vkms_connector_destroy,
> >>> > +};
> >>> > +
> >>> > +static int vkms_output_init(struct vkms_device *vkmsdev)
> >>> > +{
> >>> > + struct vkms_output *output = &vkmsdev->output;
> >>> > + struct drm_device *dev = &vkmsdev->drm;
> >>> > + struct drm_connector *connector = &output->connector;
> >>> > + struct drm_crtc *crtc = &output->crtc;
> >>> > + struct drm_plane *primary;
> >>> > + int ret;
> >>> > +
> >>> > + primary = vkms_plane_init(vkmsdev);
> >>> > + if (IS_ERR(primary))
> >>> > + return PTR_ERR(primary);
> >>> > +
> >>> > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL,
> >>> > + &vkms_crtc_funcs, NULL);
> >>> > + if (ret) {
> >>> > + DRM_ERROR("Failed to init CRTC\n");
> >>> > + goto err_crtc;
> >>> > + }
> >>> > + primary->crtc = crtc;
> >>>
> >>> If you want to split stuff up a bit, I think putting the crtc stuff into
> >>> vkms_crtc.c, and then renaming this file here to vkms_output.c would make
> >>> sense.
> >>
> >> Nice, make a lot of sense to me. I will do it on v2.
> >>
> >>> > +
> >>> > + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> >>> > + DRM_MODE_CONNECTOR_VIRTUAL);
> >>> > + if (ret) {
> >>> > + DRM_ERROR("Failed to init connector\n");
> >>> > + goto err_connector;
> >>> > + }
> >>> > +
> >>> > + ret = drm_connector_register(connector);
> >>> > + if (ret) {
> >>> > + DRM_ERROR("Failed to register connector\n");
> >>> > + goto err_connector_register;
> >>> > + }
> >>> > +
> >>> > + drm_mode_config_reset(dev);
> >>> > +
> >>> > + return 0;
> >>> > +
> >>> > +err_connector_register:
> >>> > + drm_connector_cleanup(connector);
> >>> > +
> >>> > +err_connector:
> >>> > + drm_crtc_cleanup(crtc);
> >>> > +
> >>> > +err_crtc:
> >>> > + drm_plane_cleanup(primary);
> >>> > + return ret;
> >>> > +}
> >>> > +
> >>> > +int vkms_modeset_init(struct vkms_device *vkmsdev)
> >>>
> >>> Plus keeping vkms_modeset_init in vkms_drv.c, vkms is 100% about
> >>> modesetting and nothing else, so splitting that out is a bit overkill imo.
> >>
> >> Ok, I will do it too for v2.
> >>
> >>> > +{
> >>> > + struct drm_device *dev = &vkmsdev->drm;
> >>> > +
> >>> > + drm_mode_config_init(dev);
> >>> > + dev->mode_config.funcs = &vkms_mode_funcs;
> >>> > + dev->mode_config.min_width = XRES_MIN;
> >>> > + dev->mode_config.min_height = YRES_MIN;
> >>> > + dev->mode_config.max_width = XRES_MAX;
> >>> > + dev->mode_config.max_height = YRES_MAX;
> >>> > +
> >>> > + return vkms_output_init(vkmsdev);
> >>> > +}
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> >>> > index 35517b09538e..11e0569807bd 100644
> >>> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> >>> > @@ -6,9 +6,7 @@
> >>> > */
> >>> >
> >>> > #include <linux/module.h>
> >>> > -#include <drm/drmP.h>
> >>> > #include <drm/drm_gem.h>
> >>> > -#include <drm/drm_crtc_helper.h>
> >>> > #include "vkms_drv.h"
> >>> >
> >>> > #define DRIVER_NAME "vkms"
> >>> > @@ -52,21 +50,6 @@ static struct drm_driver vkms_driver = {
> >>> > .minor = DRIVER_MINOR,
> >>> > };
> >>> >
> >>> > -static const u32 vkms_formats[] = {
> >>> > - DRM_FORMAT_XRGB8888,
> >>> > -};
> >>> > -
> >>> > -static void vkms_connector_destroy(struct drm_connector *connector)
> >>> > -{
> >>> > - drm_connector_unregister(connector);
> >>> > - drm_connector_cleanup(connector);
> >>> > -}
> >>> > -
> >>> > -static const struct drm_connector_funcs vkms_connector_funcs = {
> >>> > - .fill_modes = drm_helper_probe_single_connector_modes,
> >>> > - .destroy = vkms_connector_destroy,
> >>> > -};
> >>> > -
> >>> > static int __init vkms_init(void)
> >>> > {
> >>> > int ret;
> >>> > @@ -86,34 +69,14 @@ static int __init vkms_init(void)
> >>> > goto out_fini;
> >>> > }
> >>> >
> >>> > - drm_mode_config_init(&vkms_device->drm);
> >>> > -
> >>> > - ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
> >>> > - &vkms_connector_funcs,
> >>> > - DRM_MODE_CONNECTOR_VIRTUAL);
> >>> > - if (ret < 0) {
> >>> > - DRM_ERROR("Failed to init connector\n");
> >>> > - goto out_unregister;
> >>> > - }
> >>> > -
> >>> > - ret = drm_simple_display_pipe_init(&vkms_device->drm,
> >>> > - &vkms_device->pipe,
> >>> > - NULL,
> >>> > - vkms_formats,
> >>> > - ARRAY_SIZE(vkms_formats),
> >>> > - NULL,
> >>> > - &vkms_device->connector);
> >>> > - if (ret < 0) {
> >>> > - DRM_ERROR("Cannot setup simple display pipe\n");
> >>> > + ret = vkms_modeset_init(vkms_device);
> >>> > + if (ret)
> >>> > goto out_unregister;
> >>> > - }
> >>> >
> >>> > ret = drm_dev_register(&vkms_device->drm, 0);
> >>> > if (ret)
> >>> > goto out_unregister;
> >>> >
> >>> > - drm_connector_register(&vkms_device->connector);
> >>> > -
> >>> > return 0;
> >>> >
> >>> > out_unregister:
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >>> > index c77c5bf5032a..292bdea9c785 100644
> >>> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >>> > @@ -1,13 +1,33 @@
> >>> > #ifndef _VKMS_DRV_H_
> >>> > #define _VKMS_DRV_H_
> >>> >
> >>> > -#include <drm/drm_simple_kms_helper.h>
> >>> > +#include <drm/drmP.h>
> >>> > +#include <drm/drm.h>
> >>> > +
> >>> > +static const u32 vkms_formats[] = {
> >>> > + DRM_FORMAT_XRGB8888,
> >>> > + DRM_FORMAT_ARGB8888,
> >>> > + DRM_FORMAT_BGRX8888,
> >>> > + DRM_FORMAT_BGRA8888,
> >>> > + DRM_FORMAT_RGBX8888,
> >>> > + DRM_FORMAT_RGBA8888,
> >>> > + DRM_FORMAT_XBGR8888,
> >>> > + DRM_FORMAT_ABGR8888,
> >>>
> >>> Why all these varianst? Right now we support none of this anyway ... Until
> >>> we have more I think just limiting to XRBG8888 is good enough, we need to
> >>> flesh out the crc support first (and backing memory handling too).
> >>
> >> My bad, I don't have any good argument for using all of this variants. I
> >> will keep it simple and adds only XRBG8888.
> >>
> >> Finally, I will prepare the v2 with your recommendations and I will merge
> >> the second patch (encoder) with this one.
> >>
> >> Thanks
> >>
> >>> -Daniel
> >>>
> >>> > +};
> >>> > +
> >>> > +struct vkms_output {
> >>> > + struct drm_crtc crtc;
> >>> > + struct drm_connector connector;
> >>> > +};
> >>> >
> >>> > struct vkms_device {
> >>> > struct drm_device drm;
> >>> > struct platform_device *platform;
> >>> > - struct drm_simple_display_pipe pipe;
> >>> > - struct drm_connector connector;
> >>> > + struct vkms_output output;
> >>> > };
> >>> >
> >>> > +int vkms_modeset_init(struct vkms_device *vkmsdev);
> >>> > +
> >>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
> >>> > +
> >>> > #endif /* _VKMS_DRV_H_ */
> >>> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> >>> > new file mode 100644
> >>> > index 000000000000..2c25b1d6ab5b
> >>> > --- /dev/null
> >>> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> >>> > @@ -0,0 +1,46 @@
> >>> > +// SPDX-License-Identifier: GPL-2.0
> >>> > +/*
> >>> > + * This program is free software; you can redistribute it and/or modify
> >>> > + * it under the terms of the GNU General Public License as published by
> >>> > + * the Free Software Foundation; either version 2 of the License, or
> >>> > + * (at your option) any later version.
> >>> > + */
> >>> > +
> >>> > +#include "vkms_drv.h"
> >>> > +#include <drm/drm_plane_helper.h>
> >>> > +#include <drm/drm_atomic_helper.h>
> >>> > +
> >>> > +static const struct drm_plane_funcs vkms_plane_funcs = {
> >>> > + .update_plane = drm_atomic_helper_update_plane,
> >>> > + .disable_plane = drm_atomic_helper_disable_plane,
> >>> > + .destroy = drm_plane_cleanup,
> >>> > + .reset = drm_atomic_helper_plane_reset,
> >>> > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >>> > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> >>> > +};
> >>> > +
> >>> > +struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
> >>> > +{
> >>> > + struct drm_device *dev = &vkmsdev->drm;
> >>> > + struct drm_plane *plane;
> >>> > + const u32 *formats;
> >>> > + int ret, nformats;
> >>> > +
> >>> > + plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> >>> > + if (!plane)
> >>> > + return ERR_PTR(-ENOMEM);
> >>> > +
> >>> > + formats = vkms_formats;
> >>> > + nformats = ARRAY_SIZE(vkms_formats);
> >>> > +
> >>> > + ret = drm_universal_plane_init(dev, plane, 0,
> >>> > + &vkms_plane_funcs,
> >>> > + formats, nformats,
> >>> > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> >>> > + if (ret) {
> >>> > + kfree(plane);
> >>> > + return ERR_PTR(ret);
> >>> > + }
> >>> > +
> >>> > + return plane;
> >>> > +}
> >>> > --
> >>> > 2.17.0
> >>> >
> >>>
> >>> --
> >>> Daniel Vetter
> >>> Software Engineer, Intel Corporation
> >>> http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2018-05-16 15:49 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-16 3:06 [PATCH 0/3] Expanding the basic vkms features Rodrigo Siqueira
2018-05-16 3:06 ` [PATCH 1/3] drm/vkms: Add basic CRTC initialization Rodrigo Siqueira
2018-05-16 9:51 ` Daniel Vetter
2018-05-16 14:51 ` Rodrigo Siqueira
2018-05-16 15:40 ` Daniel Vetter
2018-05-16 15:41 ` Daniel Vetter
2018-05-16 15:49 ` Rodrigo Siqueira [this message]
2018-05-16 3:07 ` [PATCH 2/3] drm/vkms: Add encoder initialization Rodrigo Siqueira
2018-05-16 9:45 ` Daniel Vetter
2018-05-16 3:07 ` [PATCH 3/3] drm/vkms: Add extra information about vkms Rodrigo Siqueira
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180516154906.gfmwndvzdhu2ycai@smtp.gmail.com \
--to=rodrigosiqueiramelo@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hamohammed.sa@gmail.com \
--cc=seanpaul@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.