| Message ID | 20190606222751.32567-10-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: rework crc worker | expand |
On Thu, Jun 6, 2019 at 7:28 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > The crc computation worker needs to be able to get at some data > structures and framebuffer mappings, while potentially more atomic > updates are going on. The solution thus far is to copy relevant bits > around, but that's very tedious. > > Here's a new approach, which tries to be more clever, but relies on a > few not-so-obvious things: > - crtc_state is always updated when a plane_state changes. Therefore > we can just stuff plane_state pointers into a crtc_state. That > solves the problem of easily getting at the needed plane_states. Just for curiosity, where exactly the crct_state is updated? If possible, could you elaborate a little bit about this? > - with the flushing changes from previous patches the above also holds > without races due to the next atomic update being a bit eager with > cleaning up pending work - we always wait for all crc work items to > complete before unmapping framebuffers. > - we also need to make sure that the hrtimer fires off the right > worker. Keep a new distinct crc_state pointer, under the > vkms_output->lock protection for this. Note that crtc->state is > updated very early in the atomic commit, way before we arm the > vblank event - the vblank event should always match the buffers we > use to compute the crc. This also solves an issue in the hrtimer, > where we've accessed drm_crtc->state without holding the right locks > (we held none - oops). > - in the worker itself we can then just access the plane states we > need, again solving a bunch of ordering and locking issues. > Accessing plane->state requires locks, accessing the private > vkms_crtc_state->active_planes pointer only requires that the memory > doesn't get freed too early. > > The idea behind vkms_crtc_state->active_planes is that this would > contain all visible planes, in z-order, as a first step towards a more > generic blending implementation. > > Note that this patch also fixes races between prepare_fb/cleanup_fb > and the crc worker accessing ->vaddr. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/vkms/vkms_crc.c | 21 +++-------- > drivers/gpu/drm/vkms/vkms_crtc.c | 60 +++++++++++++++++++++++++++++--- > drivers/gpu/drm/vkms/vkms_drv.h | 9 ++++- > 3 files changed, 67 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > index 9d15e5e85830..0d31cfc32042 100644 > --- a/drivers/gpu/drm/vkms/vkms_crc.c > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > @@ -159,11 +159,8 @@ void vkms_crc_work_handle(struct work_struct *work) > crc_work); > struct drm_crtc *crtc = crtc_state->base.crtc; > struct vkms_output *out = drm_crtc_to_vkms_output(crtc); > - struct vkms_device *vdev = container_of(out, struct vkms_device, > - output); > struct vkms_crc_data *primary_crc = NULL; > struct vkms_crc_data *cursor_crc = NULL; > - struct drm_plane *plane; > u32 crc32 = 0; > u64 frame_start, frame_end; > bool crc_pending; > @@ -184,21 +181,11 @@ void vkms_crc_work_handle(struct work_struct *work) > if (!crc_pending) > return; > > - drm_for_each_plane(plane, &vdev->drm) { > - struct vkms_plane_state *vplane_state; > - struct vkms_crc_data *crc_data; > + if (crtc_state->num_active_planes >= 1) > + primary_crc = crtc_state->active_planes[0]->crc_data; > > - vplane_state = to_vkms_plane_state(plane->state); > - crc_data = vplane_state->crc_data; > - > - if (drm_framebuffer_read_refcount(&crc_data->fb) == 0) > - continue; > - > - if (plane->type == DRM_PLANE_TYPE_PRIMARY) > - primary_crc = crc_data; > - else > - cursor_crc = crc_data; > - } > + if (crtc_state->num_active_planes == 2) > + cursor_crc = crtc_state->active_planes[1]->crc_data; > > if (primary_crc) > crc32 = _vkms_get_crc(primary_crc, cursor_crc); > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > index 48a793ba4030..14156ed70415 100644 > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0+ > > #include "vkms_drv.h" > +#include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_probe_helper.h> > > @@ -9,7 +10,7 @@ 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 = &output->crtc; > - struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state); > + struct vkms_crtc_state *state; > u64 ret_overrun; > bool ret; > > @@ -23,6 +24,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > if (!ret) > DRM_ERROR("vkms failure on handling vblank"); > > + state = output->crc_state; > if (state && output->crc_enabled) { > u64 frame = drm_crtc_accurate_vblank_count(crtc); > > @@ -124,10 +126,9 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, > > __drm_atomic_helper_crtc_destroy_state(state); > > - if (vkms_state) { > - WARN_ON(work_pending(&vkms_state->crc_work)); > - kfree(vkms_state); > - } > + WARN_ON(work_pending(&vkms_state->crc_work)); > + kfree(vkms_state->active_planes); > + kfree(vkms_state); > } > > static void vkms_atomic_crtc_reset(struct drm_crtc *crtc) > @@ -156,6 +157,52 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { > .verify_crc_source = vkms_verify_crc_source, > }; > > +static int vkms_crtc_atomic_check(struct drm_crtc *crtc, > + struct drm_crtc_state *state) > +{ > + struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state); > + struct drm_plane *plane; > + struct drm_plane_state *plane_state; > + int i = 0, ret; > + > + if (vkms_state->active_planes) > + return 0; > + > + ret = drm_atomic_add_affected_planes(state->state, crtc); > + if (ret < 0) > + return ret; > + > + drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) { In the drm_for_each_plane_mask documentation says: “ Iterate over all planes specified by bitmask”. I did not understand what it means by “specified by bitmask” and the use of this macro in this context. I tried to replace it for drm_for_each_plane, but the test just break. Could you explain a little bit the magic behind drm_for_each_plane_mask? > + plane_state = drm_atomic_get_existing_plane_state(state->state, > + plane); > + WARN_ON(!plane_state); > + > + if (!plane_state->visible) > + continue; > + > + i++; > + } > + > + vkms_state->active_planes = kcalloc(i, sizeof(plane), GFP_KERNEL); > + if (!vkms_state->active_planes) > + return -ENOMEM; > + vkms_state->num_active_planes = i; > + > + i = 0; > + drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) { > + plane_state = drm_atomic_get_existing_plane_state(state->state, > + plane); > + > + if (!plane_state->visible) > + continue; > + > + vkms_state->active_planes[i++] = > + to_vkms_plane_state(plane_state); > + } > + > + return 0; > +} > + > static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > @@ -197,10 +244,13 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc, > crtc->state->event = NULL; > } > > + vkms_output->crc_state = to_vkms_crtc_state(crtc->state); > + > spin_unlock_irq(&vkms_output->lock); > } > > static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { > + .atomic_check = vkms_crtc_atomic_check, > .atomic_begin = vkms_crtc_atomic_begin, > .atomic_flush = vkms_crtc_atomic_flush, > .atomic_enable = vkms_crtc_atomic_enable, > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 2a35299bfb89..4e7450111d45 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -49,6 +49,10 @@ struct vkms_crtc_state { > struct drm_crtc_state base; > struct work_struct crc_work; > > + int num_active_planes; > + /* stack of active planes for crc computation, should be in z order */ > + struct vkms_plane_state **active_planes; > + > /* below three are protected by vkms_output.crc_lock */ > bool crc_pending; > u64 frame_start; > @@ -62,12 +66,15 @@ struct vkms_output { > struct hrtimer vblank_hrtimer; > ktime_t period_ns; > struct drm_pending_vblank_event *event; > - bool crc_enabled; > /* ordered wq for crc_work */ > struct workqueue_struct *crc_workq; > /* protects concurrent access to crc_data */ > spinlock_t lock; > > + /* protected by @lock */ > + bool crc_enabled; > + struct vkms_crtc_state *crc_state; > + > spinlock_t crc_lock; > }; > > -- > 2.20.1 >
On Wed, Jun 12, 2019 at 10:46:28AM -0300, Rodrigo Siqueira wrote: > On Thu, Jun 6, 2019 at 7:28 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > The crc computation worker needs to be able to get at some data > > structures and framebuffer mappings, while potentially more atomic > > updates are going on. The solution thus far is to copy relevant bits > > around, but that's very tedious. > > > > Here's a new approach, which tries to be more clever, but relies on a > > few not-so-obvious things: > > - crtc_state is always updated when a plane_state changes. Therefore > > we can just stuff plane_state pointers into a crtc_state. That > > solves the problem of easily getting at the needed plane_states. > > Just for curiosity, where exactly the crct_state is updated? If > possible, could you elaborate a little bit about this? drm_atomic_get_plane_state always gets the crtc_state too, which means if we duplicate a plane_state, we will _always_ duplicate the crtc state too. This is a guarantee by the atomic kms design. > > > - with the flushing changes from previous patches the above also holds > > without races due to the next atomic update being a bit eager with > > cleaning up pending work - we always wait for all crc work items to > > complete before unmapping framebuffers. > > - we also need to make sure that the hrtimer fires off the right > > worker. Keep a new distinct crc_state pointer, under the > > vkms_output->lock protection for this. Note that crtc->state is > > updated very early in the atomic commit, way before we arm the > > vblank event - the vblank event should always match the buffers we > > use to compute the crc. This also solves an issue in the hrtimer, > > where we've accessed drm_crtc->state without holding the right locks > > (we held none - oops). > > - in the worker itself we can then just access the plane states we > > need, again solving a bunch of ordering and locking issues. > > Accessing plane->state requires locks, accessing the private > > vkms_crtc_state->active_planes pointer only requires that the memory > > doesn't get freed too early. > > > > The idea behind vkms_crtc_state->active_planes is that this would > > contain all visible planes, in z-order, as a first step towards a more > > generic blending implementation. > > > > Note that this patch also fixes races between prepare_fb/cleanup_fb > > and the crc worker accessing ->vaddr. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/vkms/vkms_crc.c | 21 +++-------- > > drivers/gpu/drm/vkms/vkms_crtc.c | 60 +++++++++++++++++++++++++++++--- > > drivers/gpu/drm/vkms/vkms_drv.h | 9 ++++- > > 3 files changed, 67 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > > index 9d15e5e85830..0d31cfc32042 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > > @@ -159,11 +159,8 @@ void vkms_crc_work_handle(struct work_struct *work) > > crc_work); > > struct drm_crtc *crtc = crtc_state->base.crtc; > > struct vkms_output *out = drm_crtc_to_vkms_output(crtc); > > - struct vkms_device *vdev = container_of(out, struct vkms_device, > > - output); > > struct vkms_crc_data *primary_crc = NULL; > > struct vkms_crc_data *cursor_crc = NULL; > > - struct drm_plane *plane; > > u32 crc32 = 0; > > u64 frame_start, frame_end; > > bool crc_pending; > > @@ -184,21 +181,11 @@ void vkms_crc_work_handle(struct work_struct *work) > > if (!crc_pending) > > return; > > > > - drm_for_each_plane(plane, &vdev->drm) { > > - struct vkms_plane_state *vplane_state; > > - struct vkms_crc_data *crc_data; > > + if (crtc_state->num_active_planes >= 1) > > + primary_crc = crtc_state->active_planes[0]->crc_data; > > > > - vplane_state = to_vkms_plane_state(plane->state); > > - crc_data = vplane_state->crc_data; > > - > > - if (drm_framebuffer_read_refcount(&crc_data->fb) == 0) > > - continue; > > - > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY) > > - primary_crc = crc_data; > > - else > > - cursor_crc = crc_data; > > - } > > + if (crtc_state->num_active_planes == 2) > > + cursor_crc = crtc_state->active_planes[1]->crc_data; > > > > if (primary_crc) > > crc32 = _vkms_get_crc(primary_crc, cursor_crc); > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > > index 48a793ba4030..14156ed70415 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0+ > > > > #include "vkms_drv.h" > > +#include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_probe_helper.h> > > > > @@ -9,7 +10,7 @@ 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 = &output->crtc; > > - struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state); > > + struct vkms_crtc_state *state; > > u64 ret_overrun; > > bool ret; > > > > @@ -23,6 +24,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > if (!ret) > > DRM_ERROR("vkms failure on handling vblank"); > > > > + state = output->crc_state; > > if (state && output->crc_enabled) { > > u64 frame = drm_crtc_accurate_vblank_count(crtc); > > > > @@ -124,10 +126,9 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, > > > > __drm_atomic_helper_crtc_destroy_state(state); > > > > - if (vkms_state) { > > - WARN_ON(work_pending(&vkms_state->crc_work)); > > - kfree(vkms_state); > > - } > > + WARN_ON(work_pending(&vkms_state->crc_work)); > > + kfree(vkms_state->active_planes); > > + kfree(vkms_state); > > } > > > > static void vkms_atomic_crtc_reset(struct drm_crtc *crtc) > > @@ -156,6 +157,52 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { > > .verify_crc_source = vkms_verify_crc_source, > > }; > > > > +static int vkms_crtc_atomic_check(struct drm_crtc *crtc, > > + struct drm_crtc_state *state) > > +{ > > + struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state); > > + struct drm_plane *plane; > > + struct drm_plane_state *plane_state; > > + int i = 0, ret; > > + > > + if (vkms_state->active_planes) > > + return 0; > > + > > + ret = drm_atomic_add_affected_planes(state->state, crtc); > > + if (ret < 0) > > + return ret; > > + > > + drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) { > > In the drm_for_each_plane_mask documentation says: “ Iterate over all > planes specified by bitmask”. I did not understand what it means by > “specified by bitmask” and the use of this macro in this context. I state->plane_mask is the bitmask. Each bit in there represents a plane, so can be used as a very effective representation for a subset of planes (as long as we don't have more than 32 planes in total). > tried to replace it for drm_for_each_plane, but the test just break. Hm right now this should work, because we only have one crtc, so no problem with walking over planes we shouldn't even look at. > Could you explain a little bit the magic behind > drm_for_each_plane_mask? Hope the above quick summary of what we use the bitmask for helps to get you started. -Daniel > > > + plane_state = drm_atomic_get_existing_plane_state(state->state, > > + plane); > > + WARN_ON(!plane_state); > > + > > + if (!plane_state->visible) > > + continue; > > + > > + i++; > > + } > > + > > + vkms_state->active_planes = kcalloc(i, sizeof(plane), GFP_KERNEL); > > + if (!vkms_state->active_planes) > > + return -ENOMEM; > > + vkms_state->num_active_planes = i; > > + > > + i = 0; > > + drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) { > > + plane_state = drm_atomic_get_existing_plane_state(state->state, > > + plane); > > + > > + if (!plane_state->visible) > > + continue; > > + > > + vkms_state->active_planes[i++] = > > + to_vkms_plane_state(plane_state); > > + } > > + > > + return 0; > > +} > > + > > static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, > > struct drm_crtc_state *old_state) > > { > > @@ -197,10 +244,13 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc, > > crtc->state->event = NULL; > > } > > > > + vkms_output->crc_state = to_vkms_crtc_state(crtc->state); > > + > > spin_unlock_irq(&vkms_output->lock); > > } > > > > static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { > > + .atomic_check = vkms_crtc_atomic_check, > > .atomic_begin = vkms_crtc_atomic_begin, > > .atomic_flush = vkms_crtc_atomic_flush, > > .atomic_enable = vkms_crtc_atomic_enable, > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index 2a35299bfb89..4e7450111d45 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -49,6 +49,10 @@ struct vkms_crtc_state { > > struct drm_crtc_state base; > > struct work_struct crc_work; > > > > + int num_active_planes; > > + /* stack of active planes for crc computation, should be in z order */ > > + struct vkms_plane_state **active_planes; > > + > > /* below three are protected by vkms_output.crc_lock */ > > bool crc_pending; > > u64 frame_start; > > @@ -62,12 +66,15 @@ struct vkms_output { > > struct hrtimer vblank_hrtimer; > > ktime_t period_ns; > > struct drm_pending_vblank_event *event; > > - bool crc_enabled; > > /* ordered wq for crc_work */ > > struct workqueue_struct *crc_workq; > > /* protects concurrent access to crc_data */ > > spinlock_t lock; > > > > + /* protected by @lock */ > > + bool crc_enabled; > > + struct vkms_crtc_state *crc_state; > > + > > spinlock_t crc_lock; > > }; > > > > -- > > 2.20.1 > > > > > -- > > Rodrigo Siqueira > https://siqueira.tech
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c index 9d15e5e85830..0d31cfc32042 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -159,11 +159,8 @@ void vkms_crc_work_handle(struct work_struct *work) crc_work); struct drm_crtc *crtc = crtc_state->base.crtc; struct vkms_output *out = drm_crtc_to_vkms_output(crtc); - struct vkms_device *vdev = container_of(out, struct vkms_device, - output); struct vkms_crc_data *primary_crc = NULL; struct vkms_crc_data *cursor_crc = NULL; - struct drm_plane *plane; u32 crc32 = 0; u64 frame_start, frame_end; bool crc_pending; @@ -184,21 +181,11 @@ void vkms_crc_work_handle(struct work_struct *work) if (!crc_pending) return; - drm_for_each_plane(plane, &vdev->drm) { - struct vkms_plane_state *vplane_state; - struct vkms_crc_data *crc_data; + if (crtc_state->num_active_planes >= 1) + primary_crc = crtc_state->active_planes[0]->crc_data; - vplane_state = to_vkms_plane_state(plane->state); - crc_data = vplane_state->crc_data; - - if (drm_framebuffer_read_refcount(&crc_data->fb) == 0) - continue; - - if (plane->type == DRM_PLANE_TYPE_PRIMARY) - primary_crc = crc_data; - else - cursor_crc = crc_data; - } + if (crtc_state->num_active_planes == 2) + cursor_crc = crtc_state->active_planes[1]->crc_data; if (primary_crc) crc32 = _vkms_get_crc(primary_crc, cursor_crc); diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 48a793ba4030..14156ed70415 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+ #include "vkms_drv.h" +#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_probe_helper.h> @@ -9,7 +10,7 @@ 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 = &output->crtc; - struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state); + struct vkms_crtc_state *state; u64 ret_overrun; bool ret; @@ -23,6 +24,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) if (!ret) DRM_ERROR("vkms failure on handling vblank"); + state = output->crc_state; if (state && output->crc_enabled) { u64 frame = drm_crtc_accurate_vblank_count(crtc); @@ -124,10 +126,9 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, __drm_atomic_helper_crtc_destroy_state(state); - if (vkms_state) { - WARN_ON(work_pending(&vkms_state->crc_work)); - kfree(vkms_state); - } + WARN_ON(work_pending(&vkms_state->crc_work)); + kfree(vkms_state->active_planes); + kfree(vkms_state); } static void vkms_atomic_crtc_reset(struct drm_crtc *crtc) @@ -156,6 +157,52 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { .verify_crc_source = vkms_verify_crc_source, }; +static int vkms_crtc_atomic_check(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state); + struct drm_plane *plane; + struct drm_plane_state *plane_state; + int i = 0, ret; + + if (vkms_state->active_planes) + return 0; + + ret = drm_atomic_add_affected_planes(state->state, crtc); + if (ret < 0) + return ret; + + drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) { + plane_state = drm_atomic_get_existing_plane_state(state->state, + plane); + WARN_ON(!plane_state); + + if (!plane_state->visible) + continue; + + i++; + } + + vkms_state->active_planes = kcalloc(i, sizeof(plane), GFP_KERNEL); + if (!vkms_state->active_planes) + return -ENOMEM; + vkms_state->num_active_planes = i; + + i = 0; + drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) { + plane_state = drm_atomic_get_existing_plane_state(state->state, + plane); + + if (!plane_state->visible) + continue; + + vkms_state->active_planes[i++] = + to_vkms_plane_state(plane_state); + } + + return 0; +} + static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { @@ -197,10 +244,13 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc, crtc->state->event = NULL; } + vkms_output->crc_state = to_vkms_crtc_state(crtc->state); + spin_unlock_irq(&vkms_output->lock); } static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { + .atomic_check = vkms_crtc_atomic_check, .atomic_begin = vkms_crtc_atomic_begin, .atomic_flush = vkms_crtc_atomic_flush, .atomic_enable = vkms_crtc_atomic_enable, diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 2a35299bfb89..4e7450111d45 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -49,6 +49,10 @@ struct vkms_crtc_state { struct drm_crtc_state base; struct work_struct crc_work; + int num_active_planes; + /* stack of active planes for crc computation, should be in z order */ + struct vkms_plane_state **active_planes; + /* below three are protected by vkms_output.crc_lock */ bool crc_pending; u64 frame_start; @@ -62,12 +66,15 @@ struct vkms_output { struct hrtimer vblank_hrtimer; ktime_t period_ns; struct drm_pending_vblank_event *event; - bool crc_enabled; /* ordered wq for crc_work */ struct workqueue_struct *crc_workq; /* protects concurrent access to crc_data */ spinlock_t lock; + /* protected by @lock */ + bool crc_enabled; + struct vkms_crtc_state *crc_state; + spinlock_t crc_lock; };
The crc computation worker needs to be able to get at some data structures and framebuffer mappings, while potentially more atomic updates are going on. The solution thus far is to copy relevant bits around, but that's very tedious. Here's a new approach, which tries to be more clever, but relies on a few not-so-obvious things: - crtc_state is always updated when a plane_state changes. Therefore we can just stuff plane_state pointers into a crtc_state. That solves the problem of easily getting at the needed plane_states. - with the flushing changes from previous patches the above also holds without races due to the next atomic update being a bit eager with cleaning up pending work - we always wait for all crc work items to complete before unmapping framebuffers. - we also need to make sure that the hrtimer fires off the right worker. Keep a new distinct crc_state pointer, under the vkms_output->lock protection for this. Note that crtc->state is updated very early in the atomic commit, way before we arm the vblank event - the vblank event should always match the buffers we use to compute the crc. This also solves an issue in the hrtimer, where we've accessed drm_crtc->state without holding the right locks (we held none - oops). - in the worker itself we can then just access the plane states we need, again solving a bunch of ordering and locking issues. Accessing plane->state requires locks, accessing the private vkms_crtc_state->active_planes pointer only requires that the memory doesn't get freed too early. The idea behind vkms_crtc_state->active_planes is that this would contain all visible planes, in z-order, as a first step towards a more generic blending implementation. Note that this patch also fixes races between prepare_fb/cleanup_fb and the crc worker accessing ->vaddr. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Cc: Haneen Mohammed <hamohammed.sa@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/vkms/vkms_crc.c | 21 +++-------- drivers/gpu/drm/vkms/vkms_crtc.c | 60 +++++++++++++++++++++++++++++--- drivers/gpu/drm/vkms/vkms_drv.h | 9 ++++- 3 files changed, 67 insertions(+), 23 deletions(-)