| Message ID | 1431415928-21270-2-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 12, 2015 at 01:02:08PM +0530, Animesh Manna wrote: > Specifically csr mutex lock is to protect csr-related data structures > so declaration moved intel_csr structure. > > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 6 +++--- > drivers/gpu/drm/i915/intel_csr.c | 12 ++++++------ > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index a238889..78e6ae8 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -816,7 +816,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > spin_lock_init(&dev_priv->mmio_flip_lock); > mutex_init(&dev_priv->dpio_lock); > mutex_init(&dev_priv->modeset_restore_lock); > - mutex_init(&dev_priv->csr_lock); > + mutex_init(&dev_priv->csr.csr_lock); > > intel_pm_setup(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 136d42a..43011d7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -676,6 +676,9 @@ enum csr_state { > }; > > struct intel_csr { > + /* CSR protection, used to protect firmware loading status: csr_state */ > + struct mutex csr_lock; csr_ prefix is redundant. But the real trouble is that essentially your open-coding a completion using this mutex and csr->state. And that's just confusing compared to using completions directly. Can you please replace the usage of csr->state with a completion and remove the mutex entirely? -Daniel > + > const char *fw_path; > __be32 *dmc_payload; > uint32_t dmc_fw_size; > @@ -1592,9 +1595,6 @@ struct drm_i915_private { > > struct intel_csr csr; > > - /* Display CSR-related protection */ > - struct mutex csr_lock; > - > struct intel_gmbus gmbus[GMBUS_NUM_PINS]; > > /** gmbus_mutex protects against concurrent usage of the single hw gmbus > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 174106f..b03b9b3 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -195,9 +195,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) > { > enum csr_state state; > > - mutex_lock(&dev_priv->csr_lock); > + mutex_lock(&dev_priv->csr.csr_lock); > state = dev_priv->csr.state; > - mutex_unlock(&dev_priv->csr_lock); > + mutex_unlock(&dev_priv->csr.csr_lock); > > return state; > } > @@ -212,9 +212,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) > void intel_csr_load_status_set(struct drm_i915_private *dev_priv, > enum csr_state state) > { > - mutex_lock(&dev_priv->csr_lock); > + mutex_lock(&dev_priv->csr.csr_lock); > dev_priv->csr.state = state; > - mutex_unlock(&dev_priv->csr_lock); > + mutex_unlock(&dev_priv->csr.csr_lock); > } > > /** > @@ -236,7 +236,7 @@ void intel_csr_load_program(struct drm_device *dev) > return; > } > > - mutex_lock(&dev_priv->csr_lock); > + mutex_lock(&dev_priv->csr.csr_lock); > fw_size = dev_priv->csr.dmc_fw_size; > for (i = 0; i < fw_size; i++) > I915_WRITE(CSR_PROGRAM_BASE + i * 4, > @@ -248,7 +248,7 @@ void intel_csr_load_program(struct drm_device *dev) > } > > dev_priv->csr.state = FW_LOADED; > - mutex_unlock(&dev_priv->csr_lock); > + mutex_unlock(&dev_priv->csr.csr_lock); > } > > static void finish_csr_load(const struct firmware *fw, void *context) > -- > 2.0.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 5/12/2015 1:59 PM, Daniel Vetter wrote: > On Tue, May 12, 2015 at 01:02:08PM +0530, Animesh Manna wrote: >> Specifically csr mutex lock is to protect csr-related data structures >> so declaration moved intel_csr structure. >> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com> >> --- >> drivers/gpu/drm/i915/i915_dma.c | 2 +- >> drivers/gpu/drm/i915/i915_drv.h | 6 +++--- >> drivers/gpu/drm/i915/intel_csr.c | 12 ++++++------ >> 3 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >> index a238889..78e6ae8 100644 >> --- a/drivers/gpu/drm/i915/i915_dma.c >> +++ b/drivers/gpu/drm/i915/i915_dma.c >> @@ -816,7 +816,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) >> spin_lock_init(&dev_priv->mmio_flip_lock); >> mutex_init(&dev_priv->dpio_lock); >> mutex_init(&dev_priv->modeset_restore_lock); >> - mutex_init(&dev_priv->csr_lock); >> + mutex_init(&dev_priv->csr.csr_lock); >> >> intel_pm_setup(dev); >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 136d42a..43011d7 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -676,6 +676,9 @@ enum csr_state { >> }; >> >> struct intel_csr { >> + /* CSR protection, used to protect firmware loading status: csr_state */ >> + struct mutex csr_lock; > csr_ prefix is redundant. But the real trouble is that essentially your > open-coding a completion using this mutex and csr->state. And that's just > confusing compared to using completions directly. Can you please replace > the usage of csr->state with a completion and remove the mutex entirely? > -Daniel I will remove csr_ prefix. During initialization or suspend-resume scenario firmware data is written to CSR mmio registers and the completion success/failure need to capture which later is used to enable dc5/dc6. Not sure how to replace csr->state with completion (understood as returning true/false from a function). Agree as display initialization and suspend-resume is mutually exclusive mutex is not needed. Regards, Animesh > >> + >> const char *fw_path; >> __be32 *dmc_payload; >> uint32_t dmc_fw_size; >> @@ -1592,9 +1595,6 @@ struct drm_i915_private { >> >> struct intel_csr csr; >> >> - /* Display CSR-related protection */ >> - struct mutex csr_lock; >> - >> struct intel_gmbus gmbus[GMBUS_NUM_PINS]; >> >> /** gmbus_mutex protects against concurrent usage of the single hw gmbus >> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c >> index 174106f..b03b9b3 100644 >> --- a/drivers/gpu/drm/i915/intel_csr.c >> +++ b/drivers/gpu/drm/i915/intel_csr.c >> @@ -195,9 +195,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) >> { >> enum csr_state state; >> >> - mutex_lock(&dev_priv->csr_lock); >> + mutex_lock(&dev_priv->csr.csr_lock); >> state = dev_priv->csr.state; >> - mutex_unlock(&dev_priv->csr_lock); >> + mutex_unlock(&dev_priv->csr.csr_lock); >> >> return state; >> } >> @@ -212,9 +212,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) >> void intel_csr_load_status_set(struct drm_i915_private *dev_priv, >> enum csr_state state) >> { >> - mutex_lock(&dev_priv->csr_lock); >> + mutex_lock(&dev_priv->csr.csr_lock); >> dev_priv->csr.state = state; >> - mutex_unlock(&dev_priv->csr_lock); >> + mutex_unlock(&dev_priv->csr.csr_lock); >> } >> >> /** >> @@ -236,7 +236,7 @@ void intel_csr_load_program(struct drm_device *dev) >> return; >> } >> >> - mutex_lock(&dev_priv->csr_lock); >> + mutex_lock(&dev_priv->csr.csr_lock); >> fw_size = dev_priv->csr.dmc_fw_size; >> for (i = 0; i < fw_size; i++) >> I915_WRITE(CSR_PROGRAM_BASE + i * 4, >> @@ -248,7 +248,7 @@ void intel_csr_load_program(struct drm_device *dev) >> } >> >> dev_priv->csr.state = FW_LOADED; >> - mutex_unlock(&dev_priv->csr_lock); >> + mutex_unlock(&dev_priv->csr.csr_lock); >> } >> >> static void finish_csr_load(const struct firmware *fw, void *context) >> -- >> 2.0.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6385
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 302/302 302/302
SNB -1 314/314 313/314
IVB 338/338 338/338
BYT 286/286 286/286
BDW -1 320/320 319/320
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
SNB igt@pm_rpm@dpms-mode-unset-non-lpsp DMESG_WARN(13)PASS(1) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW igt@gem_gtt_hog PASS(4) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
On Wed, May 13, 2015 at 09:10:41PM +0530, Animesh Manna wrote: > > > On 5/12/2015 1:59 PM, Daniel Vetter wrote: > >On Tue, May 12, 2015 at 01:02:08PM +0530, Animesh Manna wrote: > >>Specifically csr mutex lock is to protect csr-related data structures > >>so declaration moved intel_csr structure. > >> > >>Signed-off-by: Animesh Manna <animesh.manna@intel.com> > >>Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_dma.c | 2 +- > >> drivers/gpu/drm/i915/i915_drv.h | 6 +++--- > >> drivers/gpu/drm/i915/intel_csr.c | 12 ++++++------ > >> 3 files changed, 10 insertions(+), 10 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > >>index a238889..78e6ae8 100644 > >>--- a/drivers/gpu/drm/i915/i915_dma.c > >>+++ b/drivers/gpu/drm/i915/i915_dma.c > >>@@ -816,7 +816,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > >> spin_lock_init(&dev_priv->mmio_flip_lock); > >> mutex_init(&dev_priv->dpio_lock); > >> mutex_init(&dev_priv->modeset_restore_lock); > >>- mutex_init(&dev_priv->csr_lock); > >>+ mutex_init(&dev_priv->csr.csr_lock); > >> intel_pm_setup(dev); > >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >>index 136d42a..43011d7 100644 > >>--- a/drivers/gpu/drm/i915/i915_drv.h > >>+++ b/drivers/gpu/drm/i915/i915_drv.h > >>@@ -676,6 +676,9 @@ enum csr_state { > >> }; > >> struct intel_csr { > >>+ /* CSR protection, used to protect firmware loading status: csr_state */ > >>+ struct mutex csr_lock; > >csr_ prefix is redundant. But the real trouble is that essentially your > >open-coding a completion using this mutex and csr->state. And that's just > >confusing compared to using completions directly. Can you please replace > >the usage of csr->state with a completion and remove the mutex entirely? > >-Daniel > > I will remove csr_ prefix. During initialization or suspend-resume scenario > firmware data is written to CSR mmio registers and the completion success/failure > need to capture which later is used to enable dc5/dc6. Not sure how to replace > csr->state with completion (understood as returning true/false from a function). > Agree as display initialization and suspend-resume is mutually exclusive mutex > is not needed. Ok instead of just looking at the mutex code I looked at the design overall and we need to replace it. The following /* TODO: wait for a completion event or * similar here instead of busy * waiting using wait_for function. */ wait_for((state = intel_csr_load_status_get(dev_priv)) != FW_UNINITIALIZED, 1000); from skl_set_power_well really is not how things should work. This would be the place that instead of the busys-loop would call wait_for_completion(). No need to for dev_priv->csr.state at all afaics. But digging deeper event the completion isn't necessary. The usual design in i915 for delaying runtime pm until everything is ready is to grab an additional reference for that power well until everything is set up. That way there's no need for checks, no need for completions and depencies become a lot simpler. I.e. 1) In the driver load code when you schedule the firmware loading work you also grab an appropropriate display power well (the one that will prevent going into dc5/6). 2) In the firmware load code you drop that power well reference _after_ everything has been set up. No locking needed at all, the power well code has it's own locks (power_domains->lock). 3) csr->state & csr_lock and all related code can be completely removed. Cheers, Daniel > > Regards, > Animesh > > > > >>+ > >> const char *fw_path; > >> __be32 *dmc_payload; > >> uint32_t dmc_fw_size; > >>@@ -1592,9 +1595,6 @@ struct drm_i915_private { > >> struct intel_csr csr; > >>- /* Display CSR-related protection */ > >>- struct mutex csr_lock; > >>- > >> struct intel_gmbus gmbus[GMBUS_NUM_PINS]; > >> /** gmbus_mutex protects against concurrent usage of the single hw gmbus > >>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > >>index 174106f..b03b9b3 100644 > >>--- a/drivers/gpu/drm/i915/intel_csr.c > >>+++ b/drivers/gpu/drm/i915/intel_csr.c > >>@@ -195,9 +195,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) > >> { > >> enum csr_state state; > >>- mutex_lock(&dev_priv->csr_lock); > >>+ mutex_lock(&dev_priv->csr.csr_lock); > >> state = dev_priv->csr.state; > >>- mutex_unlock(&dev_priv->csr_lock); > >>+ mutex_unlock(&dev_priv->csr.csr_lock); > >> return state; > >> } > >>@@ -212,9 +212,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) > >> void intel_csr_load_status_set(struct drm_i915_private *dev_priv, > >> enum csr_state state) > >> { > >>- mutex_lock(&dev_priv->csr_lock); > >>+ mutex_lock(&dev_priv->csr.csr_lock); > >> dev_priv->csr.state = state; > >>- mutex_unlock(&dev_priv->csr_lock); > >>+ mutex_unlock(&dev_priv->csr.csr_lock); > >> } > >> /** > >>@@ -236,7 +236,7 @@ void intel_csr_load_program(struct drm_device *dev) > >> return; > >> } > >>- mutex_lock(&dev_priv->csr_lock); > >>+ mutex_lock(&dev_priv->csr.csr_lock); > >> fw_size = dev_priv->csr.dmc_fw_size; > >> for (i = 0; i < fw_size; i++) > >> I915_WRITE(CSR_PROGRAM_BASE + i * 4, > >>@@ -248,7 +248,7 @@ void intel_csr_load_program(struct drm_device *dev) > >> } > >> dev_priv->csr.state = FW_LOADED; > >>- mutex_unlock(&dev_priv->csr_lock); > >>+ mutex_unlock(&dev_priv->csr.csr_lock); > >> } > >> static void finish_csr_load(const struct firmware *fw, void *context) > >>-- > >>2.0.2 > >> > >>_______________________________________________ > >>Intel-gfx mailing list > >>Intel-gfx@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a238889..78e6ae8 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -816,7 +816,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) spin_lock_init(&dev_priv->mmio_flip_lock); mutex_init(&dev_priv->dpio_lock); mutex_init(&dev_priv->modeset_restore_lock); - mutex_init(&dev_priv->csr_lock); + mutex_init(&dev_priv->csr.csr_lock); intel_pm_setup(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 136d42a..43011d7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -676,6 +676,9 @@ enum csr_state { }; struct intel_csr { + /* CSR protection, used to protect firmware loading status: csr_state */ + struct mutex csr_lock; + const char *fw_path; __be32 *dmc_payload; uint32_t dmc_fw_size; @@ -1592,9 +1595,6 @@ struct drm_i915_private { struct intel_csr csr; - /* Display CSR-related protection */ - struct mutex csr_lock; - struct intel_gmbus gmbus[GMBUS_NUM_PINS]; /** gmbus_mutex protects against concurrent usage of the single hw gmbus diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 174106f..b03b9b3 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -195,9 +195,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) { enum csr_state state; - mutex_lock(&dev_priv->csr_lock); + mutex_lock(&dev_priv->csr.csr_lock); state = dev_priv->csr.state; - mutex_unlock(&dev_priv->csr_lock); + mutex_unlock(&dev_priv->csr.csr_lock); return state; } @@ -212,9 +212,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv) void intel_csr_load_status_set(struct drm_i915_private *dev_priv, enum csr_state state) { - mutex_lock(&dev_priv->csr_lock); + mutex_lock(&dev_priv->csr.csr_lock); dev_priv->csr.state = state; - mutex_unlock(&dev_priv->csr_lock); + mutex_unlock(&dev_priv->csr.csr_lock); } /** @@ -236,7 +236,7 @@ void intel_csr_load_program(struct drm_device *dev) return; } - mutex_lock(&dev_priv->csr_lock); + mutex_lock(&dev_priv->csr.csr_lock); fw_size = dev_priv->csr.dmc_fw_size; for (i = 0; i < fw_size; i++) I915_WRITE(CSR_PROGRAM_BASE + i * 4, @@ -248,7 +248,7 @@ void intel_csr_load_program(struct drm_device *dev) } dev_priv->csr.state = FW_LOADED; - mutex_unlock(&dev_priv->csr_lock); + mutex_unlock(&dev_priv->csr.csr_lock); } static void finish_csr_load(const struct firmware *fw, void *context)