Re: [Intel-gfx] [PATCH 6/7] drm/i915: Toggle binder context ready status
Hi Nirmoy,
[...]
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index e5d693904123..07e95e9ab504 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -817,6 +817,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> i915_welcome_messages(i915);
>
> i915->do_release = true;
> + intel_gt_bind_context_set_ready(to_gt(i915), true);
>
> return 0;
>
> @@ -857,6 +858,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
> {
> intel_wakeref_t wakeref;
>
> + intel_gt_bind_context_set_ready(to_gt(i915), false);
> wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>
> i915_driver_unregister(i915);
> @@ -1079,6 +1081,8 @@ static int i915_drm_suspend(struct drm_device *dev)
> struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> pci_power_t opregion_target_state;
>
> + intel_gt_bind_context_set_ready(to_gt(dev_priv), false);
> +
> disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>
> /* We do a lot of poking in a lot of registers, make sure they work
> @@ -1266,6 +1270,7 @@ static int i915_drm_resume(struct drm_device *dev)
> intel_gvt_resume(dev_priv);
>
> enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> + intel_gt_bind_context_set_ready(to_gt(dev_priv), true);
would be nice if there were inline wrappers like:
static inline void intel_gt_bind_context_set_ready(struct intel_gt *gt)
{
__intel_gt_bind_context_set_ready(gt, true);
}
static inline void intel_gt_bind_context_set_unready(struct intel_gt *gt)
{
__intel_gt_bind_context_set_ready(gt, gt);
}
I always had a personal dislike of booleans in arguments as they
mean very little to whoever reads them. On the other hand, set
ready to "false" is more meaningful than set unready. Your
choice.
Reviewed-by: Andi Shyti
Andi
Re: [Intel-gfx] [PATCH 6/7] drm/i915: Toggle binder context ready status
On 9/15/2023 11:50 AM, Jani Nikula wrote:
On Fri, 15 Sep 2023, Nirmoy Das wrote:
Toggle binder context ready status when needed.
To issue gpu commands, the driver must be primed to receive
requests. Maintain binder-based GGTT update disablement until driver
probing completes. Moreover, implement a temporary disablement
of blitter prior to entering suspend, followed by re-enablement
post-resume. This is acceptable as those transition periods are
mostly single threaded.
Signed-off-by: Nirmoy Das
Signed-off-by: Oak Zeng
Not a fan of adding more gt (or gem, or display) specific details at the
top level driver hooks. The direction should be to remove stuff from
there, and move to lower levels.
The top level functions have become really hard to understand because we
don't know the ordering constraints. This adds more constraints, and
looks like very specific and complicated ones too.
Tried to hook this into gt suspend/resume but didn't work well . I will
try again.
Thanks,
Nirmoy
BR,
Jani.
---
drivers/gpu/drm/i915/i915_driver.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_driver.c
b/drivers/gpu/drm/i915/i915_driver.c
index f8dbee7a5af7..8cc289acdb39 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -815,6 +815,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
i915_welcome_messages(i915);
i915->do_release = true;
+ intel_gt_bind_context_set_ready(to_gt(i915), true);
return 0;
@@ -855,6 +856,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
{
intel_wakeref_t wakeref;
+ intel_gt_bind_context_set_ready(to_gt(i915), false);
wakeref = intel_runtime_pm_get(&i915->runtime_pm);
i915_driver_unregister(i915);
@@ -1077,6 +1079,8 @@ static int i915_drm_suspend(struct drm_device *dev)
struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
pci_power_t opregion_target_state;
+ intel_gt_bind_context_set_ready(to_gt(dev_priv), false);
+
disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
/* We do a lot of poking in a lot of registers, make sure they work
@@ -1264,6 +1268,7 @@ static int i915_drm_resume(struct drm_device *dev)
intel_gvt_resume(dev_priv);
enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
+ intel_gt_bind_context_set_ready(to_gt(dev_priv), true);
return 0;
}
Re: [Intel-gfx] [PATCH 6/7] drm/i915: Toggle binder context ready status
On Fri, 15 Sep 2023, Nirmoy Das wrote:
> Toggle binder context ready status when needed.
>
> To issue gpu commands, the driver must be primed to receive
> requests. Maintain binder-based GGTT update disablement until driver
> probing completes. Moreover, implement a temporary disablement
> of blitter prior to entering suspend, followed by re-enablement
> post-resume. This is acceptable as those transition periods are
> mostly single threaded.
>
> Signed-off-by: Nirmoy Das
> Signed-off-by: Oak Zeng
Not a fan of adding more gt (or gem, or display) specific details at the
top level driver hooks. The direction should be to remove stuff from
there, and move to lower levels.
The top level functions have become really hard to understand because we
don't know the ordering constraints. This adds more constraints, and
looks like very specific and complicated ones too.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/i915_driver.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index f8dbee7a5af7..8cc289acdb39 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -815,6 +815,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> i915_welcome_messages(i915);
>
> i915->do_release = true;
> + intel_gt_bind_context_set_ready(to_gt(i915), true);
>
> return 0;
>
> @@ -855,6 +856,7 @@ void i915_driver_remove(struct drm_i915_private *i915)
> {
> intel_wakeref_t wakeref;
>
> + intel_gt_bind_context_set_ready(to_gt(i915), false);
> wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>
> i915_driver_unregister(i915);
> @@ -1077,6 +1079,8 @@ static int i915_drm_suspend(struct drm_device *dev)
> struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> pci_power_t opregion_target_state;
>
> + intel_gt_bind_context_set_ready(to_gt(dev_priv), false);
> +
> disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>
> /* We do a lot of poking in a lot of registers, make sure they work
> @@ -1264,6 +1268,7 @@ static int i915_drm_resume(struct drm_device *dev)
> intel_gvt_resume(dev_priv);
>
> enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> + intel_gt_bind_context_set_ready(to_gt(dev_priv), true);
>
> return 0;
> }
--
Jani Nikula, Intel
