[Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest
Generally we have writable device parameters in debugfs. No need to allow writing module parameters. Signed-off-by: Jouni Högander --- 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, -- 2.34.1
Re: [Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest
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 > --- > 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 -- Cheers, Luca.
Re: [Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest
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 > > --- > > 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 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? BR, Jouni Högander > > -- > Cheers, > Luca.
Re: [Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest
On Tue, 2023-10-24 at 08:51 +, 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 > > > --- > > > 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 > > 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. -- Cheers, Luca.
Re: [Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest
On Tue, 24 Oct 2023, Luca Coelho wrote: > On Tue, 2023-10-24 at 08:51 +, 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 >> > > --- >> > > 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 >> >> 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
Re: [Intel-gfx] [PATCH v2 24/24] drm/i915/display: Use same permissions for enable_sagv as for rest
On Tue, 2023-10-24 at 15:15 +0300, Jani Nikula wrote: > On Tue, 24 Oct 2023, Luca Coelho wrote: > > On Tue, 2023-10-24 at 08:51 +, 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 > > > > > --- > > > > > 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 > > > > > > 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. Right, this was my conclusion as well, and thus, got my r-b. :) -- Cheers, Luca.
