Op 04-01-17 om 16:47 schreef Ville Syrjälä:
> On Wed, Jan 04, 2017 at 03:14:45PM +0000, Chris Wilson wrote:
>> On Wed, Jan 04, 2017 at 05:06:46PM +0200, Ville Syrjälä wrote:
>>> On Wed, Jan 04, 2017 at 02:31:26PM +0100, Maarten Lankhorst wrote:
>>>> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>>>
>>>> With atomic plane states we are able to track an allocation right from
>>>> preparation, during use and through to the final free after being
>>>> swapped out for a new plane. We can couple the VMA we pin for the
>>>> framebuffer (and its rotation) to this lifetime and avoid all the clumsy
>>>> lookups in between.
>>>>
>>>> v2: Remove residual vma on plane cleanup (Chris)
>>>> v3: Add a description for the vma destruction in
>>>> intel_plane_destroy_state (Maarten)
>>>>
>>>> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.h | 16 +---
>>>> drivers/gpu/drm/i915/intel_atomic_plane.c | 20 +++++
>>>> drivers/gpu/drm/i915/intel_display.c | 129 +++++++++++-------------------
>>>> drivers/gpu/drm/i915/intel_drv.h | 9 ++-
>>>> drivers/gpu/drm/i915/intel_fbc.c | 52 +++++-------
>>>> drivers/gpu/drm/i915/intel_fbdev.c | 4 +-
>>>> drivers/gpu/drm/i915/intel_sprite.c | 8 +-
>>>> 7 files changed, 103 insertions(+), 135 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 22d3f610212c..5369f5f9ce3a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1079,6 +1079,8 @@ struct intel_fbc {
>>>> struct work_struct underrun_work;
>>>>
>>>> struct intel_fbc_state_cache {
>>>> + struct i915_vma *vma;
>>>> +
>>>> struct {
>>>> unsigned int mode_flags;
>>>> uint32_t hsw_bdw_pixel_rate;
>>>> @@ -1092,15 +1094,14 @@ struct intel_fbc {
>>>> } plane;
>>>>
>>>> struct {
>>>> - u64 ilk_ggtt_offset;
>>>> const struct drm_format_info *format;
>>>> unsigned int stride;
>>>> - int fence_reg;
>>>> - unsigned int tiling_mode;
>>>> } fb;
>>>> } state_cache;
>>>>
>>>> struct intel_fbc_reg_params {
>>>> + struct i915_vma *vma;
>>>> +
>>>> struct {
>>>> enum pipe pipe;
>>>> enum plane plane;
>>>> @@ -1108,10 +1109,8 @@ struct intel_fbc {
>>>> } crtc;
>>>>
>>>> struct {
>>>> - u64 ggtt_offset;
>>>> const struct drm_format_info *format;
>>>> unsigned int stride;
>>>> - int fence_reg;
>>>> } fb;
>>>>
>>>> int cfb_size;
>>>> @@ -3406,13 +3405,6 @@ i915_gem_object_to_ggtt(struct drm_i915_gem_object *obj,
>>>> return i915_gem_obj_to_vma(obj, &to_i915(obj->base.dev)->ggtt.base, view);
>>>> }
>>>>
>>>> -static inline unsigned long
>>>> -i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
>>>> - const struct i915_ggtt_view *view)
>>>> -{
>>>> - return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
>>>> -}
>>>> -
>>>> /* i915_gem_fence_reg.c */
>>>> int __must_check i915_vma_get_fence(struct i915_vma *vma);
>>>> int __must_check i915_vma_put_fence(struct i915_vma *vma);
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> index 4612ffd555a7..41fd94e62d3c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> @@ -85,6 +85,8 @@ intel_plane_duplicate_state(struct drm_plane *plane)
>>>>
>>>> __drm_atomic_helper_plane_duplicate_state(plane, state);
>>>>
>>>> + intel_state->vma = NULL;
>>> Shouldn't we be doing vma_get() instead?
>> I went with NULL (dropping the copy) for simplicity. Before the plane
>> can be used it must be prepared, so vma will always be set before
>> commiting, and using NULL avoided having the reference counting dance.
>> It also allowed detection of when we didn't prepare the plane as
>> required.
> Hmm. Does that risk some kind of failure at prepare_fb time if the vma
> got nuked in the meantime? I guess that might ahve to involve
> suspend/resume or some other case where we duplicate the state and
> don't hang on to the old state.
I think the way this is done is sane, and a nice safeguard against forgetting to call prepare.
The old mapping is still around when prepare_plane_fb is called, and we keep the vma during suspend/resume
since we never unset the plane fb. We only set crtc_state->active = false (dpms off).
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
