| Message ID | 20250224164849.3746751-6-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VRAM Self Refresh | expand |
Could the subject line say something about what this patch does? On Mon, Feb 24, 2025 at 10:18:48PM +0530, Anshuman Gupta wrote: > Trade-off D3Cold target state based upon current vram usages. > if vram usages is greater then vram_d3cold_threshold and GPU > has display connected then target D3Cold state is D3Cold-VRSR > otherwise target state is D3COLD-Off. s/if vram usages/If vram usage/ (capitalize sentence and fix "usages") I dunno what "D3Cold-VRSR" means. I suppose GPU folks do.
On Mon, Feb 24, 2025 at 10:18:48PM +0530, Anshuman Gupta wrote: > Trade-off D3Cold target state based upon current vram usages. > if vram usages is greater then vram_d3cold_threshold and GPU > has display connected Why would anyone care about displays being connected or not? > then target D3Cold state is D3Cold-VRSR > otherwise target state is D3COLD-Off. > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++ > drivers/gpu/drm/xe/display/xe_display.h | 1 + > drivers/gpu/drm/xe/xe_pm.c | 12 ++++++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c > index 02a413a07382..140a43d6b1b6 100644 > --- a/drivers/gpu/drm/xe/display/xe_display.c > +++ b/drivers/gpu/drm/xe/display/xe_display.c > @@ -548,3 +548,25 @@ int xe_display_probe(struct xe_device *xe) > unset_display_features(xe); > return 0; > } > + > +bool xe_display_connected(struct xe_device *xe) > +{ > + struct drm_connector *list_connector; > + struct drm_connector_list_iter iter; > + bool ret = false; > + > + mutex_lock(&xe->drm.mode_config.mutex); > + drm_connector_list_iter_begin(&xe->drm, &iter); > + > + drm_for_each_connector_iter(list_connector, &iter) { > + if (list_connector->status == connector_status_connected) { > + ret = true; > + break; > + } > + } > + > + drm_connector_list_iter_end(&iter); > + mutex_unlock(&xe->drm.mode_config.mutex); > + > + return ret; > +} > diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h > index 685dc74402fb..c6bc54323084 100644 > --- a/drivers/gpu/drm/xe/display/xe_display.h > +++ b/drivers/gpu/drm/xe/display/xe_display.h > @@ -40,6 +40,7 @@ void xe_display_pm_resume(struct xe_device *xe); > void xe_display_pm_runtime_suspend(struct xe_device *xe); > void xe_display_pm_runtime_suspend_late(struct xe_device *xe); > void xe_display_pm_runtime_resume(struct xe_device *xe); > +bool xe_display_connected(struct xe_device *xe); > > #else > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > index 81e67b5693dc..6d28aedcb062 100644 > --- a/drivers/gpu/drm/xe/xe_pm.c > +++ b/drivers/gpu/drm/xe/xe_pm.c > @@ -198,6 +198,14 @@ static void xe_rpm_lockmap_release(const struct xe_device *xe) > &xe_pm_runtime_d3cold_map); > } > > +static void xe_pm_suspend_prepare(struct xe_device *xe) > +{ > + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE) > + xe_pm_d3cold_allowed_toggle(xe); > + else > + xe->d3cold.allowed = XE_D3COLD_OFF; > +} > + > /** > * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle > * @xe: xe device instance > @@ -213,6 +221,8 @@ int xe_pm_suspend(struct xe_device *xe) > drm_dbg(&xe->drm, "Suspending device\n"); > trace_xe_pm_suspend(xe, __builtin_return_address(0)); > > + xe_pm_suspend_prepare(xe); > + > err = xe_pxp_pm_suspend(xe->pxp); > if (err) > goto err; > @@ -875,6 +885,8 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe) > > if (total_vram_used_mb < xe->d3cold.vram_threshold) > xe->d3cold.allowed = XE_D3COLD_OFF; > + else if (xe->d3cold.vrsr_capable && xe_display_connected(xe)) > + xe->d3cold.allowed = XE_D3COLD_VRSR; > else > xe->d3cold.allowed = XE_D3HOT; > > -- > 2.34.1
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Tuesday, February 25, 2025 11:20 PM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: intel-xe@lists.freedesktop.org; linux-acpi@vger.kernel.org; linux- > pci@vger.kernel.org; rafael@kernel.org; lenb@kernel.org; > bhelgaas@google.com; ilpo.jarvinen@linux.intel.com; De Marchi, Lucas > <lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Nilawar, > Badal <badal.nilawar@intel.com>; Nasim, Kam <kam.nasim@intel.com> > Subject: Re: [RFC 5/6] drm/xe/pm: D3Cold target state > > On Mon, Feb 24, 2025 at 10:18:48PM +0530, Anshuman Gupta wrote: > > Trade-off D3Cold target state based upon current vram usages. > > if vram usages is greater than vram_d3cold_threshold and GPU has > > display connected > > Why would anyone care about displays being connected or not? As per specs we got to enable vrsr only when there is display connected, We can check that in probe but a drm connector status can change after completion of probe. That is the reason to put a check for display connected in idle callback. Thanks, Anshuman > > > then target D3Cold state is D3Cold-VRSR otherwise target state is > > D3COLD-Off. > > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++ > > drivers/gpu/drm/xe/display/xe_display.h | 1 + > > drivers/gpu/drm/xe/xe_pm.c | 12 ++++++++++++ > > 3 files changed, 35 insertions(+) > > > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c > > b/drivers/gpu/drm/xe/display/xe_display.c > > index 02a413a07382..140a43d6b1b6 100644 > > --- a/drivers/gpu/drm/xe/display/xe_display.c > > +++ b/drivers/gpu/drm/xe/display/xe_display.c > > @@ -548,3 +548,25 @@ int xe_display_probe(struct xe_device *xe) > > unset_display_features(xe); > > return 0; > > } > > + > > +bool xe_display_connected(struct xe_device *xe) { > > + struct drm_connector *list_connector; > > + struct drm_connector_list_iter iter; > > + bool ret = false; > > + > > + mutex_lock(&xe->drm.mode_config.mutex); > > + drm_connector_list_iter_begin(&xe->drm, &iter); > > + > > + drm_for_each_connector_iter(list_connector, &iter) { > > + if (list_connector->status == connector_status_connected) { > > + ret = true; > > + break; > > + } > > + } > > + > > + drm_connector_list_iter_end(&iter); > > + mutex_unlock(&xe->drm.mode_config.mutex); > > + > > + return ret; > > +} > > diff --git a/drivers/gpu/drm/xe/display/xe_display.h > > b/drivers/gpu/drm/xe/display/xe_display.h > > index 685dc74402fb..c6bc54323084 100644 > > --- a/drivers/gpu/drm/xe/display/xe_display.h > > +++ b/drivers/gpu/drm/xe/display/xe_display.h > > @@ -40,6 +40,7 @@ void xe_display_pm_resume(struct xe_device *xe); > > void xe_display_pm_runtime_suspend(struct xe_device *xe); void > > xe_display_pm_runtime_suspend_late(struct xe_device *xe); void > > xe_display_pm_runtime_resume(struct xe_device *xe); > > +bool xe_display_connected(struct xe_device *xe); > > > > #else > > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > > index 81e67b5693dc..6d28aedcb062 100644 > > --- a/drivers/gpu/drm/xe/xe_pm.c > > +++ b/drivers/gpu/drm/xe/xe_pm.c > > @@ -198,6 +198,14 @@ static void xe_rpm_lockmap_release(const struct > xe_device *xe) > > &xe_pm_runtime_d3cold_map); > > } > > > > +static void xe_pm_suspend_prepare(struct xe_device *xe) { > > + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE) > > + xe_pm_d3cold_allowed_toggle(xe); > > + else > > + xe->d3cold.allowed = XE_D3COLD_OFF; } > > + > > /** > > * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle > > * @xe: xe device instance > > @@ -213,6 +221,8 @@ int xe_pm_suspend(struct xe_device *xe) > > drm_dbg(&xe->drm, "Suspending device\n"); > > trace_xe_pm_suspend(xe, __builtin_return_address(0)); > > > > + xe_pm_suspend_prepare(xe); > > + > > err = xe_pxp_pm_suspend(xe->pxp); > > if (err) > > goto err; > > @@ -875,6 +885,8 @@ void xe_pm_d3cold_allowed_toggle(struct > xe_device > > *xe) > > > > if (total_vram_used_mb < xe->d3cold.vram_threshold) > > xe->d3cold.allowed = XE_D3COLD_OFF; > > + else if (xe->d3cold.vrsr_capable && xe_display_connected(xe)) > > + xe->d3cold.allowed = XE_D3COLD_VRSR; > > else > > xe->d3cold.allowed = XE_D3HOT; > > > > -- > > 2.34.1 > > -- > Ville Syrjälä > Intel
On Tue, Feb 25, 2025 at 06:00:04PM +0000, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Tuesday, February 25, 2025 11:20 PM > > To: Gupta, Anshuman <anshuman.gupta@intel.com> > > Cc: intel-xe@lists.freedesktop.org; linux-acpi@vger.kernel.org; linux- > > pci@vger.kernel.org; rafael@kernel.org; lenb@kernel.org; > > bhelgaas@google.com; ilpo.jarvinen@linux.intel.com; De Marchi, Lucas > > <lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Nilawar, > > Badal <badal.nilawar@intel.com>; Nasim, Kam <kam.nasim@intel.com> > > Subject: Re: [RFC 5/6] drm/xe/pm: D3Cold target state > > > > On Mon, Feb 24, 2025 at 10:18:48PM +0530, Anshuman Gupta wrote: > > > Trade-off D3Cold target state based upon current vram usages. > > > if vram usages is greater than vram_d3cold_threshold and GPU has > > > display connected > > > > Why would anyone care about displays being connected or not? > As per specs we got to enable vrsr only when there is display connected, What specs? And why do they say connected displays should be a factor here? I think the only thing that makes any sense for this kind of stuff would be sysfs power/ knobs that allow the system administrator to tune the behaviour for their specific use case. And if no such knobs exist currently then perhaps they should be added. > We can check that in probe but a drm connector status can change after completion of probe. That is the reason to put a check for display connected in idle callback. > Thanks, > Anshuman > > > > > then target D3Cold state is D3Cold-VRSR otherwise target state is > > > D3COLD-Off. > > > > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > > --- > > > drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++ > > > drivers/gpu/drm/xe/display/xe_display.h | 1 + > > > drivers/gpu/drm/xe/xe_pm.c | 12 ++++++++++++ > > > 3 files changed, 35 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c > > > b/drivers/gpu/drm/xe/display/xe_display.c > > > index 02a413a07382..140a43d6b1b6 100644 > > > --- a/drivers/gpu/drm/xe/display/xe_display.c > > > +++ b/drivers/gpu/drm/xe/display/xe_display.c > > > @@ -548,3 +548,25 @@ int xe_display_probe(struct xe_device *xe) > > > unset_display_features(xe); > > > return 0; > > > } > > > + > > > +bool xe_display_connected(struct xe_device *xe) { > > > + struct drm_connector *list_connector; > > > + struct drm_connector_list_iter iter; > > > + bool ret = false; > > > + > > > + mutex_lock(&xe->drm.mode_config.mutex); > > > + drm_connector_list_iter_begin(&xe->drm, &iter); > > > + > > > + drm_for_each_connector_iter(list_connector, &iter) { > > > + if (list_connector->status == connector_status_connected) { > > > + ret = true; > > > + break; > > > + } > > > + } > > > + > > > + drm_connector_list_iter_end(&iter); > > > + mutex_unlock(&xe->drm.mode_config.mutex); > > > + > > > + return ret; > > > +} > > > diff --git a/drivers/gpu/drm/xe/display/xe_display.h > > > b/drivers/gpu/drm/xe/display/xe_display.h > > > index 685dc74402fb..c6bc54323084 100644 > > > --- a/drivers/gpu/drm/xe/display/xe_display.h > > > +++ b/drivers/gpu/drm/xe/display/xe_display.h > > > @@ -40,6 +40,7 @@ void xe_display_pm_resume(struct xe_device *xe); > > > void xe_display_pm_runtime_suspend(struct xe_device *xe); void > > > xe_display_pm_runtime_suspend_late(struct xe_device *xe); void > > > xe_display_pm_runtime_resume(struct xe_device *xe); > > > +bool xe_display_connected(struct xe_device *xe); > > > > > > #else > > > > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c > > > index 81e67b5693dc..6d28aedcb062 100644 > > > --- a/drivers/gpu/drm/xe/xe_pm.c > > > +++ b/drivers/gpu/drm/xe/xe_pm.c > > > @@ -198,6 +198,14 @@ static void xe_rpm_lockmap_release(const struct > > xe_device *xe) > > > &xe_pm_runtime_d3cold_map); > > > } > > > > > > +static void xe_pm_suspend_prepare(struct xe_device *xe) { > > > + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE) > > > + xe_pm_d3cold_allowed_toggle(xe); > > > + else > > > + xe->d3cold.allowed = XE_D3COLD_OFF; } > > > + > > > /** > > > * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle > > > * @xe: xe device instance > > > @@ -213,6 +221,8 @@ int xe_pm_suspend(struct xe_device *xe) > > > drm_dbg(&xe->drm, "Suspending device\n"); > > > trace_xe_pm_suspend(xe, __builtin_return_address(0)); > > > > > > + xe_pm_suspend_prepare(xe); > > > + > > > err = xe_pxp_pm_suspend(xe->pxp); > > > if (err) > > > goto err; > > > @@ -875,6 +885,8 @@ void xe_pm_d3cold_allowed_toggle(struct > > xe_device > > > *xe) > > > > > > if (total_vram_used_mb < xe->d3cold.vram_threshold) > > > xe->d3cold.allowed = XE_D3COLD_OFF; > > > + else if (xe->d3cold.vrsr_capable && xe_display_connected(xe)) > > > + xe->d3cold.allowed = XE_D3COLD_VRSR; > > > else > > > xe->d3cold.allowed = XE_D3HOT; > > > > > > -- > > > 2.34.1 > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c index 02a413a07382..140a43d6b1b6 100644 --- a/drivers/gpu/drm/xe/display/xe_display.c +++ b/drivers/gpu/drm/xe/display/xe_display.c @@ -548,3 +548,25 @@ int xe_display_probe(struct xe_device *xe) unset_display_features(xe); return 0; } + +bool xe_display_connected(struct xe_device *xe) +{ + struct drm_connector *list_connector; + struct drm_connector_list_iter iter; + bool ret = false; + + mutex_lock(&xe->drm.mode_config.mutex); + drm_connector_list_iter_begin(&xe->drm, &iter); + + drm_for_each_connector_iter(list_connector, &iter) { + if (list_connector->status == connector_status_connected) { + ret = true; + break; + } + } + + drm_connector_list_iter_end(&iter); + mutex_unlock(&xe->drm.mode_config.mutex); + + return ret; +} diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h index 685dc74402fb..c6bc54323084 100644 --- a/drivers/gpu/drm/xe/display/xe_display.h +++ b/drivers/gpu/drm/xe/display/xe_display.h @@ -40,6 +40,7 @@ void xe_display_pm_resume(struct xe_device *xe); void xe_display_pm_runtime_suspend(struct xe_device *xe); void xe_display_pm_runtime_suspend_late(struct xe_device *xe); void xe_display_pm_runtime_resume(struct xe_device *xe); +bool xe_display_connected(struct xe_device *xe); #else diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c index 81e67b5693dc..6d28aedcb062 100644 --- a/drivers/gpu/drm/xe/xe_pm.c +++ b/drivers/gpu/drm/xe/xe_pm.c @@ -198,6 +198,14 @@ static void xe_rpm_lockmap_release(const struct xe_device *xe) &xe_pm_runtime_d3cold_map); } +static void xe_pm_suspend_prepare(struct xe_device *xe) +{ + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE) + xe_pm_d3cold_allowed_toggle(xe); + else + xe->d3cold.allowed = XE_D3COLD_OFF; +} + /** * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle * @xe: xe device instance @@ -213,6 +221,8 @@ int xe_pm_suspend(struct xe_device *xe) drm_dbg(&xe->drm, "Suspending device\n"); trace_xe_pm_suspend(xe, __builtin_return_address(0)); + xe_pm_suspend_prepare(xe); + err = xe_pxp_pm_suspend(xe->pxp); if (err) goto err; @@ -875,6 +885,8 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe) if (total_vram_used_mb < xe->d3cold.vram_threshold) xe->d3cold.allowed = XE_D3COLD_OFF; + else if (xe->d3cold.vrsr_capable && xe_display_connected(xe)) + xe->d3cold.allowed = XE_D3COLD_VRSR; else xe->d3cold.allowed = XE_D3HOT;
Trade-off D3Cold target state based upon current vram usages. if vram usages is greater then vram_d3cold_threshold and GPU has display connected then target D3Cold state is D3Cold-VRSR otherwise target state is D3COLD-Off. Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/xe/display/xe_display.h | 1 + drivers/gpu/drm/xe/xe_pm.c | 12 ++++++++++++ 3 files changed, 35 insertions(+)