Message ID | 1445909006-15929-1-git-send-email-vivek.kasireddy@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 27/10/15 01:23, Vivek Kasireddy wrote: > While pinning a fb object to the display plane, only install a fence > if the object is using a normal view. This corresponds with the > behavior found in i915_gem_object_do_pin() where the fencability > criteria is determined only for objects with normal views. > > v2: > Look at the object's map_and_fenceable flag to determine whether to > install a fence or not (Chris). > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 52fb3f2..108c000 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > * framebuffer compression. For simplicity, we always install > * a fence as the cost is not that onerous. > */ > - ret = i915_gem_object_get_fence(obj); > + if (obj->map_and_fenceable) > + ret = i915_gem_object_get_fence(obj); > if (ret == -EDEADLK) { > /* > * -EDEADLK means there are no free fences > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: > While pinning a fb object to the display plane, only install a fence > if the object is using a normal view. This corresponds with the > behavior found in i915_gem_object_do_pin() where the fencability > criteria is determined only for objects with normal views. > > v2: > Look at the object's map_and_fenceable flag to determine whether to > install a fence or not (Chris). > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 52fb3f2..108c000 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > * framebuffer compression. For simplicity, we always install > * a fence as the cost is not that onerous. > */ > - ret = i915_gem_object_get_fence(obj); > + if (obj->map_and_fenceable) This will now get the fence and pin it for the 90/270 view as well, even though the fence doesn't even cover that particualr gtt mapping. > + ret = i915_gem_object_get_fence(obj); > if (ret == -EDEADLK) { > /* > * -EDEADLK means there are no free fences > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 27/10/15 12:51, Ville Syrjälä wrote: > On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: >> While pinning a fb object to the display plane, only install a fence >> if the object is using a normal view. This corresponds with the >> behavior found in i915_gem_object_do_pin() where the fencability >> criteria is determined only for objects with normal views. >> >> v2: >> Look at the object's map_and_fenceable flag to determine whether to >> install a fence or not (Chris). >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 52fb3f2..108c000 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, >> * framebuffer compression. For simplicity, we always install >> * a fence as the cost is not that onerous. >> */ >> - ret = i915_gem_object_get_fence(obj); >> + if (obj->map_and_fenceable) > > This will now get the fence and pin it for the 90/270 view as well, > even though the fence doesn't even cover that particualr gtt mapping. I don't follow. obj->map_and_fenceable will be true only when normal view exists, so this avoids setting up the fence when no normal view exists and so avoids the WARN_ON in i915_gem_object_get_fence. Regards, Tvrtko
On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: > > On 27/10/15 12:51, Ville Syrjälä wrote: > > On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: > >> While pinning a fb object to the display plane, only install a fence > >> if the object is using a normal view. This corresponds with the > >> behavior found in i915_gem_object_do_pin() where the fencability > >> criteria is determined only for objects with normal views. > >> > >> v2: > >> Look at the object's map_and_fenceable flag to determine whether to > >> install a fence or not (Chris). > >> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 52fb3f2..108c000 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > >> * framebuffer compression. For simplicity, we always install > >> * a fence as the cost is not that onerous. > >> */ > >> - ret = i915_gem_object_get_fence(obj); > >> + if (obj->map_and_fenceable) > > > > This will now get the fence and pin it for the 90/270 view as well, > > even though the fence doesn't even cover that particualr gtt mapping. > > I don't follow. obj->map_and_fenceable will be true only when normal > view exists, so this avoids setting up the fence when no normal view > exists and so avoids the WARN_ON in i915_gem_object_get_fence. Sure, but it's pointless to use up a fence if all we care about is the 90/270 mapping.
On 27/10/15 13:48, Ville Syrjälä wrote: > On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: >> >> On 27/10/15 12:51, Ville Syrjälä wrote: >>> On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: >>>> While pinning a fb object to the display plane, only install a fence >>>> if the object is using a normal view. This corresponds with the >>>> behavior found in i915_gem_object_do_pin() where the fencability >>>> criteria is determined only for objects with normal views. >>>> >>>> v2: >>>> Look at the object's map_and_fenceable flag to determine whether to >>>> install a fence or not (Chris). >>>> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 52fb3f2..108c000 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, >>>> * framebuffer compression. For simplicity, we always install >>>> * a fence as the cost is not that onerous. >>>> */ >>>> - ret = i915_gem_object_get_fence(obj); >>>> + if (obj->map_and_fenceable) >>> >>> This will now get the fence and pin it for the 90/270 view as well, >>> even though the fence doesn't even cover that particualr gtt mapping. >> >> I don't follow. obj->map_and_fenceable will be true only when normal >> view exists, so this avoids setting up the fence when no normal view >> exists and so avoids the WARN_ON in i915_gem_object_get_fence. > > Sure, but it's pointless to use up a fence if all we care about > is the 90/270 mapping. After a brief IRC discussion we agreed that the patch doesn't introduce any new negative behaviours. But in general it may be beneficial to revisit the approach of always installing fences. Especially as the number of pipes and planes grows and if number of fences remains static. Regards, Tvrtko
On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote: > > On 27/10/15 13:48, Ville Syrjälä wrote: > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: > >> > >>On 27/10/15 12:51, Ville Syrjälä wrote: > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: > >>>>While pinning a fb object to the display plane, only install a fence > >>>>if the object is using a normal view. This corresponds with the > >>>>behavior found in i915_gem_object_do_pin() where the fencability > >>>>criteria is determined only for objects with normal views. > >>>> > >>>>v2: > >>>>Look at the object's map_and_fenceable flag to determine whether to > >>>>install a fence or not (Chris). > >>>> > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>>Cc: Daniel Vetter <daniel@ffwll.ch> > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > >>>>--- > >>>> drivers/gpu/drm/i915/intel_display.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>>index 52fb3f2..108c000 100644 > >>>>--- a/drivers/gpu/drm/i915/intel_display.c > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > >>>> * framebuffer compression. For simplicity, we always install > >>>> * a fence as the cost is not that onerous. > >>>> */ > >>>>- ret = i915_gem_object_get_fence(obj); > >>>>+ if (obj->map_and_fenceable) > >>> > >>>This will now get the fence and pin it for the 90/270 view as well, > >>>even though the fence doesn't even cover that particualr gtt mapping. > >> > >>I don't follow. obj->map_and_fenceable will be true only when normal > >>view exists, so this avoids setting up the fence when no normal view > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence. > > > >Sure, but it's pointless to use up a fence if all we care about > >is the 90/270 mapping. > > After a brief IRC discussion we agreed that the patch doesn't > introduce any new negative behaviours. Urm, consider intel_unpin_fb_obj(): ... i915_gem_object_unpin_fence(intel_fb_obj(obj)); So if the fb is bound both in rotated and non-rotated modes, we will have a fence for the object and try to unpin it twice => WARN (Daniel is being too nice, once upon a time that was rightfully a BUG for a major screwup). We want to track the fence state on the vma and associated that vma with the plane_state so that the tracking doesn't get so easily confused. -Chris
On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote: > On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote: > > > > On 27/10/15 13:48, Ville Syrjälä wrote: > > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: > > >> > > >>On 27/10/15 12:51, Ville Syrjälä wrote: > > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: > > >>>>While pinning a fb object to the display plane, only install a fence > > >>>>if the object is using a normal view. This corresponds with the > > >>>>behavior found in i915_gem_object_do_pin() where the fencability > > >>>>criteria is determined only for objects with normal views. > > >>>> > > >>>>v2: > > >>>>Look at the object's map_and_fenceable flag to determine whether to > > >>>>install a fence or not (Chris). > > >>>> > > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > >>>>Cc: Daniel Vetter <daniel@ffwll.ch> > > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > >>>>--- > > >>>> drivers/gpu/drm/i915/intel_display.c | 3 ++- > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > >>>> > > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > >>>>index 52fb3f2..108c000 100644 > > >>>>--- a/drivers/gpu/drm/i915/intel_display.c > > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c > > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > > >>>> * framebuffer compression. For simplicity, we always install > > >>>> * a fence as the cost is not that onerous. > > >>>> */ > > >>>>- ret = i915_gem_object_get_fence(obj); > > >>>>+ if (obj->map_and_fenceable) > > >>> > > >>>This will now get the fence and pin it for the 90/270 view as well, > > >>>even though the fence doesn't even cover that particualr gtt mapping. > > >> > > >>I don't follow. obj->map_and_fenceable will be true only when normal > > >>view exists, so this avoids setting up the fence when no normal view > > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence. > > > > > >Sure, but it's pointless to use up a fence if all we care about > > >is the 90/270 mapping. > > > > After a brief IRC discussion we agreed that the patch doesn't > > introduce any new negative behaviours. > > Urm, consider > > intel_unpin_fb_obj(): > ... > i915_gem_object_unpin_fence(intel_fb_obj(obj)); We'll have (pointlessly) pinned the fence too, so I think it'll end up working. I would have just put in view==NORMAL checks myself as an interim solution to avoid that, but whatever. > > > So if the fb is bound both in rotated and non-rotated modes, we will > have a fence for the object and try to unpin it twice => WARN (Daniel is > being too nice, once upon a time that was rightfully a BUG for a major > screwup). > > We want to track the fence state on the vma and associated that vma with > the plane_state so that the tracking doesn't get so easily confused. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Tue, Oct 27, 2015 at 08:35:36PM +0200, Ville Syrjälä wrote: > On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote: > > On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote: > > > > > > On 27/10/15 13:48, Ville Syrjälä wrote: > > > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: > > > >> > > > >>On 27/10/15 12:51, Ville Syrjälä wrote: > > > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: > > > >>>>While pinning a fb object to the display plane, only install a fence > > > >>>>if the object is using a normal view. This corresponds with the > > > >>>>behavior found in i915_gem_object_do_pin() where the fencability > > > >>>>criteria is determined only for objects with normal views. > > > >>>> > > > >>>>v2: > > > >>>>Look at the object's map_and_fenceable flag to determine whether to > > > >>>>install a fence or not (Chris). > > > >>>> > > > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > >>>>Cc: Daniel Vetter <daniel@ffwll.ch> > > > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > >>>>--- > > > >>>> drivers/gpu/drm/i915/intel_display.c | 3 ++- > > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > > >>>> > > > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > >>>>index 52fb3f2..108c000 100644 > > > >>>>--- a/drivers/gpu/drm/i915/intel_display.c > > > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c > > > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > > > >>>> * framebuffer compression. For simplicity, we always install > > > >>>> * a fence as the cost is not that onerous. > > > >>>> */ > > > >>>>- ret = i915_gem_object_get_fence(obj); > > > >>>>+ if (obj->map_and_fenceable) > > > >>> > > > >>>This will now get the fence and pin it for the 90/270 view as well, > > > >>>even though the fence doesn't even cover that particualr gtt mapping. > > > >> > > > >>I don't follow. obj->map_and_fenceable will be true only when normal > > > >>view exists, so this avoids setting up the fence when no normal view > > > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence. > > > > > > > >Sure, but it's pointless to use up a fence if all we care about > > > >is the 90/270 mapping. > > > > > > After a brief IRC discussion we agreed that the patch doesn't > > > introduce any new negative behaviours. > > > > Urm, consider > > > > intel_unpin_fb_obj(): > > ... > > i915_gem_object_unpin_fence(intel_fb_obj(obj)); > > We'll have (pointlessly) pinned the fence too, so I think it'll end up > working. I would have just put in view==NORMAL checks myself as an > interim solution to avoid that, but whatever. No, it didn't. If we rotated first, we don't get a fence and so don't pin it. Then we attach an unrotated, grab a fence and pin it. Then we end up unpinning twice vs a single pin. -Chris
On Tue, Oct 27, 2015 at 06:47:19PM +0000, Chris Wilson wrote: > On Tue, Oct 27, 2015 at 08:35:36PM +0200, Ville Syrjälä wrote: > > On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote: > > > On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote: > > > > > > > > On 27/10/15 13:48, Ville Syrjälä wrote: > > > > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: > > > > >> > > > > >>On 27/10/15 12:51, Ville Syrjälä wrote: > > > > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: > > > > >>>>While pinning a fb object to the display plane, only install a fence > > > > >>>>if the object is using a normal view. This corresponds with the > > > > >>>>behavior found in i915_gem_object_do_pin() where the fencability > > > > >>>>criteria is determined only for objects with normal views. > > > > >>>> > > > > >>>>v2: > > > > >>>>Look at the object's map_and_fenceable flag to determine whether to > > > > >>>>install a fence or not (Chris). > > > > >>>> > > > > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > >>>>Cc: Daniel Vetter <daniel@ffwll.ch> > > > > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > > >>>>--- > > > > >>>> drivers/gpu/drm/i915/intel_display.c | 3 ++- > > > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > > > >>>> > > > > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > >>>>index 52fb3f2..108c000 100644 > > > > >>>>--- a/drivers/gpu/drm/i915/intel_display.c > > > > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c > > > > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > > > > >>>> * framebuffer compression. For simplicity, we always install > > > > >>>> * a fence as the cost is not that onerous. > > > > >>>> */ > > > > >>>>- ret = i915_gem_object_get_fence(obj); > > > > >>>>+ if (obj->map_and_fenceable) > > > > >>> > > > > >>>This will now get the fence and pin it for the 90/270 view as well, > > > > >>>even though the fence doesn't even cover that particualr gtt mapping. > > > > >> > > > > >>I don't follow. obj->map_and_fenceable will be true only when normal > > > > >>view exists, so this avoids setting up the fence when no normal view > > > > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence. > > > > > > > > > >Sure, but it's pointless to use up a fence if all we care about > > > > >is the 90/270 mapping. > > > > > > > > After a brief IRC discussion we agreed that the patch doesn't > > > > introduce any new negative behaviours. > > > > > > Urm, consider > > > > > > intel_unpin_fb_obj(): > > > ... > > > i915_gem_object_unpin_fence(intel_fb_obj(obj)); > > > > We'll have (pointlessly) pinned the fence too, so I think it'll end up > > working. I would have just put in view==NORMAL checks myself as an > > interim solution to avoid that, but whatever. > > No, it didn't. If we rotated first, we don't get a fence and so don't > pin it. Then we attach an unrotated, grab a fence and pin it. Then we > end up unpinning twice vs a single pin. Oh, map_and_fenceable on the _object_ depends whether the normal view is bound, and yeah it could get bound only after the rotated view got used for scanout. So view==NORMAL for now seems like good enough solution for now to me. Anything else means a bigger rework of the code, and I for one don't want to got there until I've managed to land my already pending stuff.
On 27/10/15 18:58, Ville Syrjälä wrote: > On Tue, Oct 27, 2015 at 06:47:19PM +0000, Chris Wilson wrote: >> On Tue, Oct 27, 2015 at 08:35:36PM +0200, Ville Syrjälä wrote: >>> On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote: >>>> On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote: >>>>> >>>>> On 27/10/15 13:48, Ville Syrjälä wrote: >>>>>> On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote: >>>>>>> >>>>>>> On 27/10/15 12:51, Ville Syrjälä wrote: >>>>>>>> On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote: >>>>>>>>> While pinning a fb object to the display plane, only install a fence >>>>>>>>> if the object is using a normal view. This corresponds with the >>>>>>>>> behavior found in i915_gem_object_do_pin() where the fencability >>>>>>>>> criteria is determined only for objects with normal views. >>>>>>>>> >>>>>>>>> v2: >>>>>>>>> Look at the object's map_and_fenceable flag to determine whether to >>>>>>>>> install a fence or not (Chris). >>>>>>>>> >>>>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>>>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/i915/intel_display.c | 3 ++- >>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>>>>> index 52fb3f2..108c000 100644 >>>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>>>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, >>>>>>>>> * framebuffer compression. For simplicity, we always install >>>>>>>>> * a fence as the cost is not that onerous. >>>>>>>>> */ >>>>>>>>> - ret = i915_gem_object_get_fence(obj); >>>>>>>>> + if (obj->map_and_fenceable) >>>>>>>> >>>>>>>> This will now get the fence and pin it for the 90/270 view as well, >>>>>>>> even though the fence doesn't even cover that particualr gtt mapping. >>>>>>> >>>>>>> I don't follow. obj->map_and_fenceable will be true only when normal >>>>>>> view exists, so this avoids setting up the fence when no normal view >>>>>>> exists and so avoids the WARN_ON in i915_gem_object_get_fence. >>>>>> >>>>>> Sure, but it's pointless to use up a fence if all we care about >>>>>> is the 90/270 mapping. >>>>> >>>>> After a brief IRC discussion we agreed that the patch doesn't >>>>> introduce any new negative behaviours. >>>> >>>> Urm, consider >>>> >>>> intel_unpin_fb_obj(): >>>> ... >>>> i915_gem_object_unpin_fence(intel_fb_obj(obj)); >>> >>> We'll have (pointlessly) pinned the fence too, so I think it'll end up >>> working. I would have just put in view==NORMAL checks myself as an >>> interim solution to avoid that, but whatever. >> >> No, it didn't. If we rotated first, we don't get a fence and so don't >> pin it. Then we attach an unrotated, grab a fence and pin it. Then we >> end up unpinning twice vs a single pin. > > Oh, map_and_fenceable on the _object_ depends whether the normal view is > bound, and yeah it could get bound only after the rotated view got used > for scanout. So view==NORMAL for now seems like good enough solution for > now to me. Anything else means a bigger rework of the code, and I for > one don't want to got there until I've managed to land my already pending > stuff. Right, so one WARN eliminated, one to go. :) Vivek would you have the time to add a new test case which will hit a WARN in i915_gem_object_unpin_fence with the v2 of your patch and post a v3 which fixes it? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 52fb3f2..108c000 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, * framebuffer compression. For simplicity, we always install * a fence as the cost is not that onerous. */ - ret = i915_gem_object_get_fence(obj); + if (obj->map_and_fenceable) + ret = i915_gem_object_get_fence(obj); if (ret == -EDEADLK) { /* * -EDEADLK means there are no free fences
While pinning a fb object to the display plane, only install a fence if the object is using a normal view. This corresponds with the behavior found in i915_gem_object_do_pin() where the fencability criteria is determined only for objects with normal views. v2: Look at the object's map_and_fenceable flag to determine whether to install a fence or not (Chris). Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)