Re: [PATCH 08/11] drm/i915/dsb: Add i915.enable_dsb module parameter

[Jani Nikula <jani.nikula@linux.intel.com>]

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.


-- 
Jani Nikula, Intel