| Message ID | 20231016111658.3432581-25-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Framework for display parameters | expand |
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> -- Cheers, Luca.
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? BR, Jouni Högander > > -- > Cheers, > Luca.
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. -- Cheers, Luca.
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.
On Tue, 2023-10-24 at 15:15 +0300, Jani Nikula wrote: > 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. Right, this was my conclusion as well, and thus, got my r-b. :) -- Cheers, Luca.
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,
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(-)