Re: [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
On Fri, Jul 22, 2022 at 05:32:11PM -0400, Jim Shargo wrote:
> This change supports multiple CRTCs, encoders, connectors instead of one
> of each per card.
>
> Since ConfigFS-based devices will support multiple crtcs, it's useful to
> move all of the writeback/composition data from being a per-output thing
> to being a per-CRTC thing.
>
> Since there's still only ever one CRTC, this should be a no-op refactor.
>
> Signed-off-by: Jim Shargo
> ---
Actual PATCH 3/5 review below :-)
> drivers/gpu/drm/vkms/vkms_composer.c | 28 +++
> drivers/gpu/drm/vkms/vkms_crtc.c | 91 +++--
> drivers/gpu/drm/vkms/vkms_drv.c | 3 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 66 ++--
> drivers/gpu/drm/vkms/vkms_output.c| 110 ++
> drivers/gpu/drm/vkms/vkms_plane.c | 39 ++---
> drivers/gpu/drm/vkms/vkms_writeback.c | 25 +++---
> 7 files changed, 225 insertions(+), 137 deletions(-)
>
/snip
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> b/drivers/gpu/drm/vkms/vkms_crtc.c
> index c1b632952532..d93678d984ae 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -11,35 +11,34 @@
>
> static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> {
> - struct vkms_output *output = container_of(timer, struct vkms_output,
> - vblank_hrtimer);
> - struct drm_crtc *crtc = >crtc;
> + struct vkms_crtc *vkms_crtc = timer_to_vkms_crtc(timer);
> + struct drm_crtc *crtc = _crtc->base;
> struct vkms_crtc_state *state;
> u64 ret_overrun;
> bool ret, fence_cookie;
>
> fence_cookie = dma_fence_begin_signalling();
>
> - ret_overrun = hrtimer_forward_now(>vblank_hrtimer,
> - output->period_ns);
> + ret_overrun = hrtimer_forward_now(_crtc->vblank_hrtimer,
> + vkms_crtc->period_ns);
> if (ret_overrun != 1)
> pr_warn("%s: vblank timer overrun\n", __func__);
>
> - spin_lock(>lock);
> + spin_lock(_crtc->lock);
> ret = drm_crtc_handle_vblank(crtc);
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> - state = output->composer_state;
> - spin_unlock(>lock);
> + state = vkms_crtc->composer_state;
> + spin_unlock(_crtc->lock);
>
> - if (state && output->composer_enabled) {
> + if (state && vkms_crtc->composer_enabled) {
> u64 frame = drm_crtc_accurate_vblank_count(crtc);
>
> /* update frame_start only if a queued vkms_composer_worker()
>* has read the data
>*/
> - spin_lock(>composer_lock);
> + spin_lock(_crtc->composer_lock);
> if (!state->crc_pending)
> state->frame_start = frame;
> else
> @@ -47,9 +46,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
> hrtimer *timer)
>state->frame_start, frame);
> state->frame_end = frame;
> state->crc_pending = true;
> - spin_unlock(>composer_lock);
> + spin_unlock(_crtc->composer_lock);
>
> - ret = queue_work(output->composer_workq, >composer_work);
> + ret = queue_work(vkms_crtc->composer_workq,
> >composer_work);
> if (!ret)
> DRM_DEBUG_DRIVER("Composer worker already queued\n");
> }
> @@ -62,28 +61,24 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
> hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *out = >output;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = >vblank[pipe];
>
> drm_calc_timestamping_constants(crtc, >mode);
>
> - hrtimer_init(>vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - out->vblank_hrtimer.function = _vblank_simulate;
> - out->period_ns = ktime_set(0, vblank->framedur_ns);
> - hrtimer_start(>vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
> + hrtimer_init(_crtc->vblank_hrtimer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL);
> + vkms_crtc->vblank_hrtimer.function = _vblank_simulate;
> + vkms_crtc->period_ns = ktime_set(0, vblank->framedur_ns);
> + hrtimer_start(_crtc->vblank_hrtimer, vkms_crtc->period_ns,
> HRTIMER_MODE_REL);
>
> return 0;
> }
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *out = >output;
> -
> - hrtimer_cancel(>vblank_hrtimer);
> + struct vkms_crtc *vkms_crtc =
Re: [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
On Fri, Aug 05, 2022 at 06:27:08PM +, Sean Paul wrote: > On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote: > > This is a small refactor to make ConfigFS support easier. > > > > We now store the vkms_device statically, and maintain a list of > > "cards", each representing a different virtual DRM driver. > > > > We also make it clear when a card is "default", that is created at > > initialization, and not. This is because, due to limitations on what we > > can do with the configfs API, the default card won't be reflected in > > configfs and is initialized in a unique way. > > > > Since we're only managing a single card, this should be a no-op. > > > > Signed-off-by: Jim Shargo /snip What a mess, I replied to the wrong patch. The review here is targeting PATCH 2/5 despite the title and reply-to.
Re: [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote:
> This is a small refactor to make ConfigFS support easier.
>
> We now store the vkms_device statically, and maintain a list of
> "cards", each representing a different virtual DRM driver.
>
> We also make it clear when a card is "default", that is created at
> initialization, and not. This is because, due to limitations on what we
> can do with the configfs API, the default card won't be reflected in
> configfs and is initialized in a unique way.
>
> Since we're only managing a single card, this should be a no-op.
>
> Signed-off-by: Jim Shargo
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 11 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 160 --
> drivers/gpu/drm/vkms/vkms_drv.h | 32 --
> drivers/gpu/drm/vkms/vkms_output.c| 25 ++--
> drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
> drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++--
> 6 files changed, 158 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 57bbd32e9beb..c1b632952532 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -62,9 +62,10 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
> hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = >output;
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = >vblank[pipe];
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>
> drm_calc_timestamping_constants(crtc, >mode);
>
> @@ -78,7 +79,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> + struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = >output;
>
> hrtimer_cancel(>vblank_hrtimer);
> }
> @@ -88,9 +91,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> bool in_vblank_irq)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *output = >output;
> unsigned int pipe = crtc->index;
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> - struct vkms_output *output = >output;
> struct drm_vblank_crtc *vblank = >vblank[pipe];
>
> if (!READ_ONCE(vblank->enabled)) {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 81ed9417e511..92fbade2199b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -9,9 +9,9 @@
> * the GPU in DRM API tests.
> */
>
> +#include
Unless you're using something directly from this header, it's probably best not
to include it as it pulls in a _lot_ of stuff.
> #include
> #include
> -#include
>
> #include
> #include
> @@ -37,7 +37,7 @@
> #define DRIVER_MAJOR 1
> #define DRIVER_MINOR 0
>
> -static struct vkms_device *vkms_device;
> +static struct vkms_device vkms_device;
>
> static bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> @@ -55,9 +55,9 @@ DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>
> static void vkms_release(struct drm_device *dev)
> {
> - struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
>
> - destroy_workqueue(vkms->output.composer_workq);
> + destroy_workqueue(card->output.composer_workq);
> }
>
> static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> @@ -91,9 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state
> *old_state)
>
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> - seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
> - seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
> - seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
> + seq_printf(m, "writeback=%d\n", vkms_device.config.writeback);
> + seq_printf(m, "cursor=%d\n", vkms_device.config.cursor);
> + seq_printf(m, "overlay=%d\n", vkms_device.config.overlay);
>
> return 0;
> }
> @@ -133,9 +133,9 @@ static const struct drm_mode_config_helper_funcs
> vkms_mode_config_helpers = {
> .atomic_commit_tail = vkms_atomic_commit_tail,
> };
>
> -static int vkms_modeset_init(struct vkms_device *vkmsdev)
> +static int vkms_modeset_init(struct vkms_card *card)
> {
> - struct drm_device *dev = >drm;
> + struct drm_device *dev = >drm;
>
> drm_mode_config_init(dev);
> dev->mode_config.funcs = _mode_funcs;
> @@ -151,89 +151,133 @@
