Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[Ville Syrjälä]

On Tue, Mar 10, 2015 at 12:32:43PM +, Tvrtko Ursulin wrote:
 
 On 03/10/2015 12:19 PM, Chris Wilson wrote:
  On Tue, Mar 10, 2015 at 12:02:28PM +, Tvrtko Ursulin wrote:
  @@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device 
  *dev)
   NULL)) {
DRM_ERROR(failed to pin boot fb on pipe %d\n,
  to_intel_crtc(c)-pipe);
  - drm_framebuffer_unreference(c-primary-fb);
  - c-primary-fb = NULL;
  - update_state_fb(c-primary);
  + unused[n_unused++] = c-primary;
}
}
mutex_unlock(dev-struct_mutex);
 
  + while (n_unused--) {
  + struct drm_plane *p = unused[n_unused];
  + drm_framebuffer_unreference(p-fb);
  + p-fb = NULL;
  + update_state_fb(p);
  + }
  +
 
  For this one I am not sure. Should c-primary-fb = NULL remain
  under the locked loop? If not what is the the mutex protecting then?
 
  It's a dummy mutex that only exists to keep the WARNs quiet. This phase
  of initialisation is explicitly single-threaded.
 
 Would it be a simpler fix then to move the mutex only around 
 pin_and_fence_fb_obj?

That would be much nicer indeed.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[Chris Wilson]

On Tue, Mar 10, 2015 at 12:02:28PM +, Tvrtko Ursulin wrote:
 @@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
 NULL)) {
  DRM_ERROR(failed to pin boot fb on pipe %d\n,
to_intel_crtc(c)-pipe);
 -drm_framebuffer_unreference(c-primary-fb);
 -c-primary-fb = NULL;
 -update_state_fb(c-primary);
 +unused[n_unused++] = c-primary;
  }
  }
  mutex_unlock(dev-struct_mutex);
 
 +while (n_unused--) {
 +struct drm_plane *p = unused[n_unused];
 +drm_framebuffer_unreference(p-fb);
 +p-fb = NULL;
 +update_state_fb(p);
 +}
 +
 
 For this one I am not sure. Should c-primary-fb = NULL remain
 under the locked loop? If not what is the the mutex protecting then?

It's a dummy mutex that only exists to keep the WARNs quiet. This phase
of initialisation is explicitly single-threaded.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[Tvrtko Ursulin]

Hi,

On 02/16/2015 02:31 PM, Chris Wilson wrote:

intel_user_framebuffer_destroy() requires the struct_mutex for its
object bookkeeping, so this means that all calls to
drm_framebuffer_reference must be held without that lock.


Same comment on the commit message as 1/2.


References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
  drivers/gpu/drm/i915/intel_display.c | 13 ++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 6e1da7da5cca..aba36662d511 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13672,6 +13672,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_crtc *c;
struct drm_i915_gem_object *obj;
+   struct drm_plane *unused[I915_MAX_PIPES];
+   int n_unused = 0;

mutex_lock(dev-struct_mutex);
intel_init_gt_powersave(dev);
@@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
   NULL)) {
DRM_ERROR(failed to pin boot fb on pipe %d\n,
  to_intel_crtc(c)-pipe);
-   drm_framebuffer_unreference(c-primary-fb);
-   c-primary-fb = NULL;
-   update_state_fb(c-primary);
+   unused[n_unused++] = c-primary;
}
}
mutex_unlock(dev-struct_mutex);

+   while (n_unused--) {
+   struct drm_plane *p = unused[n_unused];
+   drm_framebuffer_unreference(p-fb);
+   p-fb = NULL;
+   update_state_fb(p);
+   }
+


For this one I am not sure. Should c-primary-fb = NULL remain under 
the locked loop? If not what is the the mutex protecting then?


Regards,

Tvrtko

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[Tvrtko Ursulin]

On 03/10/2015 12:19 PM, Chris Wilson wrote:

On Tue, Mar 10, 2015 at 12:02:28PM +, Tvrtko Ursulin wrote:

@@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
   NULL)) {
DRM_ERROR(failed to pin boot fb on pipe %d\n,
  to_intel_crtc(c)-pipe);
-   drm_framebuffer_unreference(c-primary-fb);
-   c-primary-fb = NULL;
-   update_state_fb(c-primary);
+   unused[n_unused++] = c-primary;
}
}
mutex_unlock(dev-struct_mutex);

+   while (n_unused--) {
+   struct drm_plane *p = unused[n_unused];
+   drm_framebuffer_unreference(p-fb);
+   p-fb = NULL;
+   update_state_fb(p);
+   }
+


For this one I am not sure. Should c-primary-fb = NULL remain
under the locked loop? If not what is the the mutex protecting then?


It's a dummy mutex that only exists to keep the WARNs quiet. This phase
of initialisation is explicitly single-threaded.


Would it be a simpler fix then to move the mutex only around 
pin_and_fence_fb_obj?


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[shuang . he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: 
shuang...@intel.com)
Task id: 5783
-Summary-
Platform  Delta  drm-intel-nightly  Series Applied
PNV -1  277/277  276/277
ILK  313/313  313/313
SNB -1  309/309  308/309
IVB  382/382  382/382
BYT  296/296  296/296
HSW  425/425  425/425
BDW -1  318/318  317/318
-Detailed-
Platform  Testdrm-intel-nightly  Series 
Applied
*PNV  igt_drm_vma_limiter_cached  NRUN(1)PASS(1)  NO_RESULT(1)PASS(1)
*SNB  igt_kms_plane_plane-position-covered-pipe-B-plane-1  PASS(2)  
DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog  PASS(5)  DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[Chris Wilson]

intel_user_framebuffer_destroy() requires the struct_mutex for its
object bookkeeping, so this means that all calls to
drm_framebuffer_reference must be held without that lock.

References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 6e1da7da5cca..aba36662d511 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13672,6 +13672,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_crtc *c;
struct drm_i915_gem_object *obj;
+   struct drm_plane *unused[I915_MAX_PIPES];
+   int n_unused = 0;
 
mutex_lock(dev-struct_mutex);
intel_init_gt_powersave(dev);
@@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
   NULL)) {
DRM_ERROR(failed to pin boot fb on pipe %d\n,
  to_intel_crtc(c)-pipe);
-   drm_framebuffer_unreference(c-primary-fb);
-   c-primary-fb = NULL;
-   update_state_fb(c-primary);
+   unused[n_unused++] = c-primary;
}
}
mutex_unlock(dev-struct_mutex);
 
+   while (n_unused--) {
+   struct drm_plane *p = unused[n_unused];
+   drm_framebuffer_unreference(p-fb);
+   p-fb = NULL;
+   update_state_fb(p);
+   }
+
intel_backlight_register(dev);
 }
 
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx