Re: [Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest

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

On Tue, 24 Oct 2023, Luca Coelho <luca@coelho.fi> wrote:
> On Tue, 2023-10-24 at 08:51 +0000, Hogander, Jouni wrote:
>> On Mon, 2023-10-23 at 17:06 +0300, Luca Coelho wrote:
>> > On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander wrote:
>> > > Generally we have writable device parameters in debugfs. No need
>> > > to allow writing module parameters.
>> > > 
>> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/display/intel_display_params.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
>> > > b/drivers/gpu/drm/i915/display/intel_display_params.c
>> > > index 8e6353c1c25e..077f2dee2975 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>> > > @@ -50,7 +50,7 @@ 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_sagv, bool, 0600,
>> > > +intel_display_param_named_unsafe(enable_sagv, bool, 0400,
>> > >         "Enable system agent voltage/frequency scaling (SAGV)
>> > > (default: true)");
>> > >  
>> > >  intel_display_param_named_unsafe(disable_power_well, int, 0400,
>> > 
>> > This, as well as other similar changes throughout this series, could
>> > be
>> > controversial, since it's a userspace API change of sorts.  It used
>> > to
>> > be possible to write but it won't be anymore.  But, as we discussed
>> > offline, it shouldn't be problem, because probably nobody is writing
>> > to
>> > them, and most likely doing so wouldn't have the expected result,
>> > since
>> > the device copies were not getting updated.
>> > 
>> > Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
>> 
>> Thank you Luca. I actually moved this change to patch 11 due to your
>> comment there and added your rb tag there. I was planning to drop this
>> patch. Are you fine with this?
>
> Yes, this is fine.  I'll review your cahnges in v3 and give the missing
> r-b tags there, if applicable.

I think this change is good and frankly needed. It's confusing to be
able to modify the module param without it having any effect.

These are for debug, the param is designated "unsafe", and for these I
don't really care if someone complains they can't write to the file
anymore.

BR,
Jani.

>
> --
> Cheers,
> Luca.

-- 
Jani Nikula, Intel