| Message ID | 20240611133344.30673-9-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsb: DSB prep stuff | expand |
On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > As we extend the use of DSB for critical pipe/plane register > programming, it'll be nice to have an escape valve at hand, > in case things go very poorly. To that end, add a i915.enable_dsb > modparam by which we can force the driver to take the pure mmio > path instead. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ > 3 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c > index aebdb7b59dbf..449a31767791 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, > intel_display_param_named_unsafe(enable_dpt, bool, 0400, > "Enable display page table (DPT) (default: true)"); > > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, > + "Enable display state buffer (DSB) (default: true)"); Not much point in leaving the module param 0600, is there? BR, Jani. > + > intel_display_param_named_unsafe(enable_sagv, bool, 0400, > "Enable system agent voltage/frequency scaling (SAGV) (default: true)"); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h > index 1208a62c16d2..48c29c55c939 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_params.h > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h > @@ -31,6 +31,7 @@ struct drm_i915_private; > param(int, vbt_sdvo_panel_type, -1, 0400) \ > param(int, enable_dc, -1, 0400) \ > param(bool, enable_dpt, true, 0400) \ > + param(bool, enable_dsb, true, 0600) \ > param(bool, enable_sagv, true, 0600) \ > param(int, disable_power_well, -1, 0400) \ > param(bool, enable_ips, true, 0600) \ > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c > index bee48ac419ce..2ab3765f6c06 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > @@ -460,6 +460,9 @@ struct intel_dsb *intel_dsb_prepare(struct intel_atomic_state *state, > if (!HAS_DSB(i915)) > return NULL; > > + if (!i915->display.params.enable_dsb) > + return NULL; > + > /* TODO: DSB is broken in Xe KMD, so disabling it until fixed */ > if (!IS_ENABLED(I915)) > return NULL;
On Tue, Jun 18, 2024 at 02:07:56PM +0300, Jani Nikula wrote: > On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > As we extend the use of DSB for critical pipe/plane register > > programming, it'll be nice to have an escape valve at hand, > > in case things go very poorly. To that end, add a i915.enable_dsb > > modparam by which we can force the driver to take the pure mmio > > path instead. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ > > 3 files changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c > > index aebdb7b59dbf..449a31767791 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, > > intel_display_param_named_unsafe(enable_dpt, bool, 0400, > > "Enable display page table (DPT) (default: true)"); > > > > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, > > + "Enable display state buffer (DSB) (default: true)"); > > Not much point in leaving the module param 0600, is there? It'll let you try both dsb and mmio paths at runtime without having to do a full reboot/reload. > > BR, > Jani. > > > + > > intel_display_param_named_unsafe(enable_sagv, bool, 0400, > > "Enable system agent voltage/frequency scaling (SAGV) (default: true)"); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h > > index 1208a62c16d2..48c29c55c939 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_params.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h > > @@ -31,6 +31,7 @@ struct drm_i915_private; > > param(int, vbt_sdvo_panel_type, -1, 0400) \ > > param(int, enable_dc, -1, 0400) \ > > param(bool, enable_dpt, true, 0400) \ > > + param(bool, enable_dsb, true, 0600) \ > > param(bool, enable_sagv, true, 0600) \ > > param(int, disable_power_well, -1, 0400) \ > > param(bool, enable_ips, true, 0600) \ > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c > > index bee48ac419ce..2ab3765f6c06 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > > @@ -460,6 +460,9 @@ struct intel_dsb *intel_dsb_prepare(struct intel_atomic_state *state, > > if (!HAS_DSB(i915)) > > return NULL; > > > > + if (!i915->display.params.enable_dsb) > > + return NULL; > > + > > /* TODO: DSB is broken in Xe KMD, so disabling it until fixed */ > > if (!IS_ENABLED(I915)) > > return NULL; > > -- > Jani Nikula, Intel
On Wed, 19 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Jun 18, 2024 at 02:07:56PM +0300, Jani Nikula wrote: >> On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > As we extend the use of DSB for critical pipe/plane register >> > programming, it'll be nice to have an escape valve at hand, >> > in case things go very poorly. To that end, add a i915.enable_dsb >> > modparam by which we can force the driver to take the pure mmio >> > path instead. >> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ >> > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + >> > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ >> > 3 files changed, 7 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c >> > index aebdb7b59dbf..449a31767791 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c >> > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, >> > intel_display_param_named_unsafe(enable_dpt, bool, 0400, >> > "Enable display page table (DPT) (default: true)"); >> > >> > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, >> > + "Enable display state buffer (DSB) (default: true)"); >> >> Not much point in leaving the module param 0600, is there? > > It'll let you try both dsb and mmio paths at runtime without > having to do a full reboot/reload. I mean does any code actually look at the *module* parameter runtime? It's only the initial value for the device param? BR, Jani. > >> >> BR, >> Jani. >> >> > + >> > intel_display_param_named_unsafe(enable_sagv, bool, 0400, >> > "Enable system agent voltage/frequency scaling (SAGV) (default: true)"); >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h >> > index 1208a62c16d2..48c29c55c939 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display_params.h >> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h >> > @@ -31,6 +31,7 @@ struct drm_i915_private; >> > param(int, vbt_sdvo_panel_type, -1, 0400) \ >> > param(int, enable_dc, -1, 0400) \ >> > param(bool, enable_dpt, true, 0400) \ >> > + param(bool, enable_dsb, true, 0600) \ >> > param(bool, enable_sagv, true, 0600) \ >> > param(int, disable_power_well, -1, 0400) \ >> > param(bool, enable_ips, true, 0600) \ >> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c >> > index bee48ac419ce..2ab3765f6c06 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >> > @@ -460,6 +460,9 @@ struct intel_dsb *intel_dsb_prepare(struct intel_atomic_state *state, >> > if (!HAS_DSB(i915)) >> > return NULL; >> > >> > + if (!i915->display.params.enable_dsb) >> > + return NULL; >> > + >> > /* TODO: DSB is broken in Xe KMD, so disabling it until fixed */ >> > if (!IS_ENABLED(I915)) >> > return NULL; >> >> -- >> Jani Nikula, Intel
On Wed, Jun 19, 2024 at 04:11:08PM +0300, Jani Nikula wrote: > On Wed, 19 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, Jun 18, 2024 at 02:07:56PM +0300, Jani Nikula wrote: > >> On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > > >> > As we extend the use of DSB for critical pipe/plane register > >> > programming, it'll be nice to have an escape valve at hand, > >> > in case things go very poorly. To that end, add a i915.enable_dsb > >> > modparam by which we can force the driver to take the pure mmio > >> > path instead. > >> > > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > >> > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > >> > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ > >> > 3 files changed, 7 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c > >> > index aebdb7b59dbf..449a31767791 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > >> > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, > >> > intel_display_param_named_unsafe(enable_dpt, bool, 0400, > >> > "Enable display page table (DPT) (default: true)"); > >> > > >> > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, > >> > + "Enable display state buffer (DSB) (default: true)"); > >> > >> Not much point in leaving the module param 0600, is there? > > > > It'll let you try both dsb and mmio paths at runtime without > > having to do a full reboot/reload. > > I mean does any code actually look at the *module* parameter runtime? > It's only the initial value for the device param? You can change it via the debugfs i915_params/* thing.
On Wed, Jun 19, 2024 at 04:24:16PM +0300, Ville Syrjälä wrote: > On Wed, Jun 19, 2024 at 04:11:08PM +0300, Jani Nikula wrote: > > On Wed, 19 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Tue, Jun 18, 2024 at 02:07:56PM +0300, Jani Nikula wrote: > > >> On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> > > > >> > As we extend the use of DSB for critical pipe/plane register > > >> > programming, it'll be nice to have an escape valve at hand, > > >> > in case things go very poorly. To that end, add a i915.enable_dsb > > >> > modparam by which we can force the driver to take the pure mmio > > >> > path instead. > > >> > > > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> > --- > > >> > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > > >> > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > > >> > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ > > >> > 3 files changed, 7 insertions(+) > > >> > > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c > > >> > index aebdb7b59dbf..449a31767791 100644 > > >> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > > >> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > > >> > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, > > >> > intel_display_param_named_unsafe(enable_dpt, bool, 0400, > > >> > "Enable display page table (DPT) (default: true)"); > > >> > > > >> > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, > > >> > + "Enable display state buffer (DSB) (default: true)"); > > >> > > >> Not much point in leaving the module param 0600, is there? > > > > > > It'll let you try both dsb and mmio paths at runtime without > > > having to do a full reboot/reload. > > > > I mean does any code actually look at the *module* parameter runtime? > > It's only the initial value for the device param? > > You can change it via the debugfs i915_params/* thing. Apparently the modparam vs. debugfs permissions are specified in two different places. This is rather confusing. Is there no way to put them in the same place? Or can we just nuke the permission stuff from the modparam macro entirely so it won't end up confusing me again? Looks like there is exactly one (gem related) modparam that uses 0600, everything else seems to be 0400.
On Wed, Jun 19, 2024 at 05:44:08PM +0300, Ville Syrjälä wrote: > On Wed, Jun 19, 2024 at 04:24:16PM +0300, Ville Syrjälä wrote: > > On Wed, Jun 19, 2024 at 04:11:08PM +0300, Jani Nikula wrote: > > > On Wed, 19 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > On Tue, Jun 18, 2024 at 02:07:56PM +0300, Jani Nikula wrote: > > > >> On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >> > > > > >> > As we extend the use of DSB for critical pipe/plane register > > > >> > programming, it'll be nice to have an escape valve at hand, > > > >> > in case things go very poorly. To that end, add a i915.enable_dsb > > > >> > modparam by which we can force the driver to take the pure mmio > > > >> > path instead. > > > >> > > > > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >> > --- > > > >> > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > > > >> > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > > > >> > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ > > > >> > 3 files changed, 7 insertions(+) > > > >> > > > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c > > > >> > index aebdb7b59dbf..449a31767791 100644 > > > >> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > > > >> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > > > >> > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, > > > >> > intel_display_param_named_unsafe(enable_dpt, bool, 0400, > > > >> > "Enable display page table (DPT) (default: true)"); > > > >> > > > > >> > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, > > > >> > + "Enable display state buffer (DSB) (default: true)"); > > > >> > > > >> Not much point in leaving the module param 0600, is there? > > > > > > > > It'll let you try both dsb and mmio paths at runtime without > > > > having to do a full reboot/reload. > > > > > > I mean does any code actually look at the *module* parameter runtime? > > > It's only the initial value for the device param? > > > > You can change it via the debugfs i915_params/* thing. > > Apparently the modparam vs. debugfs permissions are specified in two > different places. This is rather confusing. > > Is there no way to put them in the same place? Or can we just nuke > the permission stuff from the modparam macro entirely so it won't > end up confusing me again? Looks like there is exactly one (gem related) > modparam that uses 0600, everything else seems to be 0400. And even that seems to use the per-device copy in the actual code. So looks to me like we can just rip out the macro argument and make it 0400 always.
On Wed, 19 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Jun 19, 2024 at 05:44:08PM +0300, Ville Syrjälä wrote: >> On Wed, Jun 19, 2024 at 04:24:16PM +0300, Ville Syrjälä wrote: >> > On Wed, Jun 19, 2024 at 04:11:08PM +0300, Jani Nikula wrote: >> > > On Wed, 19 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > > > On Tue, Jun 18, 2024 at 02:07:56PM +0300, Jani Nikula wrote: >> > > >> On Tue, 11 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> > > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > >> > >> > > >> > As we extend the use of DSB for critical pipe/plane register >> > > >> > programming, it'll be nice to have an escape valve at hand, >> > > >> > in case things go very poorly. To that end, add a i915.enable_dsb >> > > >> > modparam by which we can force the driver to take the pure mmio >> > > >> > path instead. >> > > >> > >> > > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > >> > --- >> > > >> > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ >> > > >> > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + >> > > >> > drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++ >> > > >> > 3 files changed, 7 insertions(+) >> > > >> > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c >> > > >> > index aebdb7b59dbf..449a31767791 100644 >> > > >> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c >> > > >> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c >> > > >> > @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, >> > > >> > intel_display_param_named_unsafe(enable_dpt, bool, 0400, >> > > >> > "Enable display page table (DPT) (default: true)"); >> > > >> > >> > > >> > +intel_display_param_named_unsafe(enable_dsb, bool, 0600, >> > > >> > + "Enable display state buffer (DSB) (default: true)"); >> > > >> >> > > >> Not much point in leaving the module param 0600, is there? >> > > > >> > > > It'll let you try both dsb and mmio paths at runtime without >> > > > having to do a full reboot/reload. >> > > >> > > I mean does any code actually look at the *module* parameter runtime? >> > > It's only the initial value for the device param? >> > >> > You can change it via the debugfs i915_params/* thing. >> >> Apparently the modparam vs. debugfs permissions are specified in two >> different places. This is rather confusing. >> >> Is there no way to put them in the same place? Or can we just nuke >> the permission stuff from the modparam macro entirely so it won't >> end up confusing me again? Looks like there is exactly one (gem related) >> modparam that uses 0600, everything else seems to be 0400. > > And even that seems to use the per-device copy in the actual code. > So looks to me like we can just rip out the macro argument and > make it 0400 always. Yeah, it's not great. I guess the only reason the modparams would need to be writable is to be able to use different values for different devices when the modparam values are needed at probe, before they can be changed via device param debugfs: - set modparam to x - probe device 1 - set modparam to y - probe device 2 I don't know if anyone really uses that. There are really no good solutions to this. :( BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c index aebdb7b59dbf..449a31767791 100644 --- a/drivers/gpu/drm/i915/display/intel_display_params.c +++ b/drivers/gpu/drm/i915/display/intel_display_params.c @@ -54,6 +54,9 @@ intel_display_param_named_unsafe(enable_dc, int, 0400, intel_display_param_named_unsafe(enable_dpt, bool, 0400, "Enable display page table (DPT) (default: true)"); +intel_display_param_named_unsafe(enable_dsb, bool, 0600, + "Enable display state buffer (DSB) (default: true)"); + intel_display_param_named_unsafe(enable_sagv, bool, 0400, "Enable system agent voltage/frequency scaling (SAGV) (default: true)"); diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h index 1208a62c16d2..48c29c55c939 100644 --- a/drivers/gpu/drm/i915/display/intel_display_params.h +++ b/drivers/gpu/drm/i915/display/intel_display_params.h @@ -31,6 +31,7 @@ struct drm_i915_private; param(int, vbt_sdvo_panel_type, -1, 0400) \ param(int, enable_dc, -1, 0400) \ param(bool, enable_dpt, true, 0400) \ + param(bool, enable_dsb, true, 0600) \ param(bool, enable_sagv, true, 0600) \ param(int, disable_power_well, -1, 0400) \ param(bool, enable_ips, true, 0600) \ diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c index bee48ac419ce..2ab3765f6c06 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.c +++ b/drivers/gpu/drm/i915/display/intel_dsb.c @@ -460,6 +460,9 @@ struct intel_dsb *intel_dsb_prepare(struct intel_atomic_state *state, if (!HAS_DSB(i915)) return NULL; + if (!i915->display.params.enable_dsb) + return NULL; + /* TODO: DSB is broken in Xe KMD, so disabling it until fixed */ if (!IS_ENABLED(I915)) return NULL;