Re: [PATCH] drm/amdgpu/display/mst: fix an unused-variable warning

2023-01-26 Thread Dave Airlie
On Fri, 27 Jan 2023 at 07:06, Hamza Mahfooz  wrote:
>
> On 1/26/23 11:35, Arnd Bergmann wrote:
> > From: Arnd Bergmann 
> >
> > The newly added code is in an #ifdef, so the variables that
> > are only used in there cause a warning if CONFIG_DRM_AMD_DC_DCN
> > is disabled:
> >
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 
> > 'amdgpu_dm_atomic_check':
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9698:43: error: 
> > unused variable 'mst_state' [-Werror=unused-variable]
> >   9698 | struct drm_dp_mst_topology_state *mst_state;
> >|   ^
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9697:41: error: 
> > unused variable 'mgr' [-Werror=unused-variable]
> >   9697 | struct drm_dp_mst_topology_mgr *mgr;
> >| ^~~
> >
> > Fixes: c689e1e362ea ("drm/amdgpu/display/mst: Fix mst_state->pbn_div and 
> > slot count assignments")
> > Signed-off-by: Arnd Bergmann 
>
> Applied, thanks!
>
> > ---
> >   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index be1232356f9e..c966bb05f6c7 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -9694,8 +9694,10 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> > *dev,
> >   struct drm_connector_state *old_con_state, *new_con_state;
> >   struct drm_crtc *crtc;
> >   struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > +#if defined(CONFIG_DRM_AMD_DC_DCN)
> >   struct drm_dp_mst_topology_mgr *mgr;
> >   struct drm_dp_mst_topology_state *mst_state;
> > +#endif
> >   struct drm_plane *plane;
> >   struct drm_plane_state *old_plane_state, *new_plane_state;
> >   enum dc_status status;
>

I've pushed an alternate fix to drm-fixes as I pulled in a tree this
morning and it failed to build on arm here.

Dave.


[PATCH] drm/amd/display: reduce else-if to else in dcn10_blank_pixel_data()

2023-01-26 Thread Tom Rix
checkpatch reports
drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c:2902:13: style:
  Expression is always true because 'else if' condition is opposite to previous 
condition at line 2895. [multiCondition]
 } else if (blank) {
^
drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c:2895:6: note: first 
condition
 if (!blank) {
 ^
drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c:2902:13: note: else 
if condition is opposite to first condition
 } else if (blank) {

It is not necessary to explicitly the check != condition, an else is simplier.

Fixes: aa5a57773042 ("drm/amd/display: Vari-bright looks disabled near end of 
MM14")
Signed-off-by: Tom Rix 
---
 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
index bb155734ac93..f735ae5e045f 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
@@ -2899,7 +2899,7 @@ void dcn10_blank_pixel_data(
dc->hwss.set_pipe(pipe_ctx);
stream_res->abm->funcs->set_abm_level(stream_res->abm, 
stream->abm_level);
}
-   } else if (blank) {
+   } else {
dc->hwss.set_abm_immediate_disable(pipe_ctx);
if (stream_res->tg->funcs->set_blank) {
stream_res->tg->funcs->wait_for_state(stream_res->tg, 
CRTC_STATE_VBLANK);
-- 
2.26.3



Re: [PATCH] drm/amdgpu: add force_sg_display module parameter

2023-01-26 Thread Alex Deucher
Ignore this patch.  It has a typo.  I have a fixed version.

Alex

On Tue, Jan 24, 2023 at 10:13 AM Alex Deucher  wrote:
>
> Add a module parameter to force sg (scatter/gather) display
> on APUs.  Normally we allow displays in both VRAM and GTT,
> but this option forces displays into GTT so we can explicitly
> test more scenarios with GTT.
>
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h|  2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 12 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  4 
>  3 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 872450a3a164..73d0a0807138 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -244,6 +244,8 @@ extern int amdgpu_num_kcq;
>  #define AMDGPU_VCNFW_LOG_SIZE (32 * 1024)
>  extern int amdgpu_vcnfw_log;
>
> +extern int amdgpu_force_sg_display;
> +
>  #define AMDGPU_VM_MAX_NUM_CTX  4096
>  #define AMDGPU_SG_THRESHOLD(256*1024*1024)
>  #define AMDGPU_DEFAULT_GTT_SIZE_MB 3072ULL /* 3GB by default */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index a75dba2caeca..bc0eaf2330f2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -942,6 +942,18 @@ MODULE_PARM_DESC(smu_pptable_id,
> "specify pptable id to be used (-1 = auto(default) value, 0 = use 
> pptable from vbios, > 0 = soft pptable id)");
>  module_param_named(smu_pptable_id, amdgpu_smu_pptable_id, int, 0444);
>
> +/**
> + * DOC: force_sg_display (int)
> + * Force display buffers into GTT (scatter/gather) memory for APUs.
> + * This is used to force GTT only for displays rather than displaying from
> + * either VRAM (carve out) or GTT.
> + *
> + * Defaults to 0, or disabled.
> + */
> +int amdgpu_force_sg_display;
> +MODULE_PARM_DESC(force_sg_display, "Force S/G display (0 = off (default), 1 
> = force display to use GTT) ");
> +module_param_named(force_sg_display, amdgpu_force_sg_display, int, 0444);
> +
>  /* These devices are not supported by amdgpu.
>   * They are supported by the mach64, r128, radeon drivers
>   */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 2d237f3d3a2e..78dc5d63a6dd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1515,6 +1515,10 @@ uint32_t amdgpu_bo_get_preferred_domain(struct 
> amdgpu_device *adev,
> if (adev->gmc.real_vram_size <= AMDGPU_SG_THRESHOLD)
> domain = AMDGPU_GEM_DOMAIN_GTT;
> }
> +   if (amdgpu_force_sg_display &&
> +   (adev->asic_type >= CHIP_CARRIZO) &&
> +   (adev->flags & AMD_IS_APU))
> +   domain = AMDGPU_GEM_DOMAIN_GTT;
> return domain;
>  }
>
> --
> 2.39.0
>


Re: drm/amdgpu: add force_sg_display module parameter

2023-01-26 Thread Limonciello, Mario

On 1/24/2023 09:13, Alex Deucher wrote:

Add a module parameter to force sg (scatter/gather) display
on APUs.  Normally we allow displays in both VRAM and GTT,
but this option forces displays into GTT so we can explicitly
test more scenarios with GTT.

Signed-off-by: Alex Deucher 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h|  2 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 12 
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  4 
  3 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 872450a3a164..73d0a0807138 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -244,6 +244,8 @@ extern int amdgpu_num_kcq;
  #define AMDGPU_VCNFW_LOG_SIZE (32 * 1024)
  extern int amdgpu_vcnfw_log;
  
+extern int amdgpu_force_sg_display;

+
  #define AMDGPU_VM_MAX_NUM_CTX 4096
  #define AMDGPU_SG_THRESHOLD   (256*1024*1024)
  #define AMDGPU_DEFAULT_GTT_SIZE_MB3072ULL /* 3GB by default */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index a75dba2caeca..bc0eaf2330f2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -942,6 +942,18 @@ MODULE_PARM_DESC(smu_pptable_id,
"specify pptable id to be used (-1 = auto(default) value, 0 = use pptable from 
vbios, > 0 = soft pptable id)");
  module_param_named(smu_pptable_id, amdgpu_smu_pptable_id, int, 0444);
  
+/**

+ * DOC: force_sg_display (int)
+ * Force display buffers into GTT (scatter/gather) memory for APUs.
+ * This is used to force GTT only for displays rather than displaying from
+ * either VRAM (carve out) or GTT.
+ *
+ * Defaults to 0, or disabled.
+ */
+int amdgpu_force_sg_display;
+MODULE_PARM_DESC(force_sg_display, "Force S/G display (0 = off (default), 1 = force 
display to use GTT) ");
+module_param_named(force_sg_display, amdgpu_force_sg_display, int, 0444);


To discourage the use of this from non-developers, perhaps it should be:
`module_param_named_unsafe`

That will taint the kernel when it's used.


+
  /* These devices are not supported by amdgpu.
   * They are supported by the mach64, r128, radeon drivers
   */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 2d237f3d3a2e..78dc5d63a6dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1515,6 +1515,10 @@ uint32_t amdgpu_bo_get_preferred_domain(struct 
amdgpu_device *adev,
if (adev->gmc.real_vram_size <= AMDGPU_SG_THRESHOLD)
domain = AMDGPU_GEM_DOMAIN_GTT;
}
+   if (amdgpu_force_sg_display &&
+   (adev->asic_type >= CHIP_CARRIZO) &&
+   (adev->flags & AMD_IS_APU))
+   domain = AMDGPU_GEM_DOMAIN_GTT;
return domain;
  }
  




Re: [PATCH] drm/amdgpu/display/mst: fix an unused-variable warning

2023-01-26 Thread Hamza Mahfooz

On 1/26/23 11:35, Arnd Bergmann wrote:

From: Arnd Bergmann 

The newly added code is in an #ifdef, so the variables that
are only used in there cause a warning if CONFIG_DRM_AMD_DC_DCN
is disabled:

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 
'amdgpu_dm_atomic_check':
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9698:43: error: 
unused variable 'mst_state' [-Werror=unused-variable]
  9698 | struct drm_dp_mst_topology_state *mst_state;
   |   ^
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9697:41: error: 
unused variable 'mgr' [-Werror=unused-variable]
  9697 | struct drm_dp_mst_topology_mgr *mgr;
   | ^~~

Fixes: c689e1e362ea ("drm/amdgpu/display/mst: Fix mst_state->pbn_div and slot count 
assignments")
Signed-off-by: Arnd Bergmann 


Applied, thanks!


---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index be1232356f9e..c966bb05f6c7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9694,8 +9694,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
struct drm_connector_state *old_con_state, *new_con_state;
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+#if defined(CONFIG_DRM_AMD_DC_DCN)
struct drm_dp_mst_topology_mgr *mgr;
struct drm_dp_mst_topology_state *mst_state;
+#endif
struct drm_plane *plane;
struct drm_plane_state *old_plane_state, *new_plane_state;
enum dc_status status;


--
Hamza



[PATCH] drm/amd/display: reduce else-if to else in dcn32_calculate_dlg_params()

2023-01-26 Thread Tom Rix
cppcheck reports
drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c:1403:76: style:
  Expression is always true because 'else if' condition is opposite to previous 
condition at line 1396. [multiCondition]
   } else if (context->res_ctx.pipe_ctx[i].stream->mall_stream_config.type == 
SUBVP_PHANTOM) {
   ^
drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c:1396:69: note: first 
condition
   if (context->res_ctx.pipe_ctx[i].stream->mall_stream_config.type != 
SUBVP_PHANTOM) {
^
drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c:1403:76: note: else if 
condition is opposite to first condition
   } else if (context->res_ctx.pipe_ctx[i].stream->mall_stream_config.type == 
SUBVP_PHANTOM) {

It is not necessary to explicitly the check != condition, an else is simplier.

Fixes: 238debcaebe4 ("drm/amd/display: Use DML for MALL SS and Subvp allocation 
calculations")
Signed-off-by: Tom Rix 
---
 drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c 
b/drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c
index 0dc1a03999b6..c96cbd88e20d 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/dcn32/dcn32_fpu.c
@@ -1400,7 +1400,7 @@ static void dcn32_calculate_dlg_params(struct dc *dc, 
struct dc_state *context,
/* SS PSR On: all active surfaces part 
of streams not supporting PSR stored in MALL */

context->bw_ctx.bw.dcn.mall_ss_psr_active_size_bytes += 
context->res_ctx.pipe_ctx[i].surface_size_in_mall_bytes;
}
-   } else if 
(context->res_ctx.pipe_ctx[i].stream->mall_stream_config.type == SUBVP_PHANTOM) 
{
+   } else {
/* SUBVP: phantom surfaces only stored in MALL 
*/
context->bw_ctx.bw.dcn.mall_subvp_size_bytes += 
context->res_ctx.pipe_ctx[i].surface_size_in_mall_bytes;
}
-- 
2.26.3



Re: [PATCH 8/8] drm/amd/pm: drop the support for manual fan speed setting on SMU13.0.7

2023-01-26 Thread Alex Deucher
On Wed, Jan 25, 2023 at 7:14 PM Matt Coffin  wrote:
>
> On Wed Jan 11, 2023 at 7:47 AM MST, Alex Deucher wrote:
> > On Wed, Jan 11, 2023 at 8:23 AM Quan, Evan  wrote:
> > >
> > > [AMD Official Use Only - General]
> > >
> > > Regarding the manual fan speed setting issue targeted by this patch, the 
> > > SCPM feature of the new SMU13 asics prevents us from toggling the fan 
> > > control feature from auto to manual.
>
> This makes sense as a move towards using the same interface that other
> platforms are most likely using.
>
> > > About the capability in the OD table you mentioned, it might be a 
> > > different issue.
>
> I included some info/questions below; any hints you could give to point
> me in the right way to keep learning would be appreciated.

I'm following up with the SMU teams.  Will get back to you when I know more.

Thanks,

Alex

>
> >
> > Right.  Manual fan control is no longer possible.  As Evan said, you
> > can adjust the automatic fan curve using the OD interface, but that is
> > it.
>
> Sorry for the late reply; I became busy with day job. I've been working
> on implementing OD support (and a sysfs interface to set *any* OD
> setting by number, in contrast with pp_od_clk_voltage's pidgeon-holing
> into supporting only PP_OD_DPM_TABLE_COMMAND commands), at the very
> least for my own experimentation.
>
> The following is what I see when I read the OD table out from the SMU
> (assuming that the inclusion of another VF curve setting at index 4 in
> the header was a mistake, based on the values returned by the SMU).
>
> It seems that, at least in my case, my hardware is running in some kind
> of mode that would *not* allow changing of the fan curve? Is it possble
> that the header information in pm/inc/smu_v13_0_0_pptable.h is incorrect
> even beyond the potential idx 4 of ODSETTINGs?
>
> It appears also that transferring the OD table *back* to the SMU results
> in no error, but also no action taken, as subsequent reads to not
> reflect any changes. I'm thinking this is due to some values read in on
> the inital read of the table being invalid, but seemingly irrelevant
> given what is reported by the capabilities (see: FAN_CURVE[*]).
>
> Is there any hints you guys could offer in terms of
>
> 1. what might be mal-aligned or mis-labeled in the smu_v13 pptable
> header above?
> 2. What pre-requisites I might be missing to allow the support for
> ODCAP_FAN_CURVE?
> 3. Why the apparent values for some settings in the boot table seemingly
> wildly invalid? Will those somehow become valid once pre-requisites for
> OD operation are met?
>
> I also feel like I've strayed from the original topic of the proposed
> patch, and this probably belongs in it's own thread... but quite know
> how to preserve any context there (sorry).
>
> Thanks in advance for helping out an eager outsider,
> Matt
>
> Capabilities:
> SMU_13_0_0_ODCAP_GFXCLK_LIMITS[0] true
> SMU_13_0_0_ODCAP_GFXCLK_CURVE[1] true
> SMU_13_0_0_ODCAP_UCLK_LIMITS[2] true
> SMU_13_0_0_ODCAP_POWER_LIMIT[3] true
> SMU_13_0_0_ODCAP_FAN_ACOUSTIC_LIMIT[4] true
> SMU_13_0_0_ODCAP_FAN_SPEED_MIN[5] true
> SMU_13_0_0_ODCAP_TEMPERATURE_FAN[6] true
> SMU_13_0_0_ODCAP_TEMPERATURE_SYSTEM[7] true
> SMU_13_0_0_ODCAP_MEMORY_TIMING_TUNE[8] true
> SMU_13_0_0_ODCAP_FAN_ZERO_RPM_CONTROL[9] true
> SMU_13_0_0_ODCAP_AUTO_UV_ENGINE[10] true
> SMU_13_0_0_ODCAP_AUTO_OC_ENGINE[11] true
> SMU_13_0_0_ODCAP_AUTO_OC_MEMORY[12] true
> SMU_13_0_0_ODCAP_FAN_CURVE[13] false
> SMU_13_0_0_ODCAP_AUTO_FAN_ACOUSTIC_LIMIT[14] true
> SMU_13_0_0_ODCAP_POWER_MODE[15] false
>
> Limits:
> SMU_13_0_0_ODSETTING_GFXCLKFMAX[0] - [500,5000]
> SMU_13_0_0_ODSETTING_GFXCLKFMIN[1] - [500,5000]
> SMU_13_0_0_ODSETTING_CUSTOM_GFX_VF_CURVE_A[2] - [97,1500]
> SMU_13_0_0_ODSETTING_CUSTOM_GFX_VF_CURVE_B[3] - [97,1500]
> SMU_13_0_0_ODSETTING_CUSTOM_CURVE_VFT_FMIN[4] - [10,15]
> SMU_13_0_0_ODSETTING_UCLKFMIN[5] - [500,3200]
> SMU_13_0_0_ODSETTING_UCLKFMAX[6] - [500,3200]
> SMU_13_0_0_ODSETTING_POWERPERCENTAGE[7] - [25,105]
> SMU_13_0_0_ODSETTING_FANRPMMIN[8] - [50,110]
> SMU_13_0_0_ODSETTING_FANRPMACOUSTICLIMIT[9] - [0,1]
> SMU_13_0_0_ODSETTING_FANTARGETTEMPERATURE[10] - [0,1]
> SMU_13_0_0_ODSETTING_OPERATINGTEMPMAX[11] - [0,1]
> SMU_13_0_0_ODSETTING_ACTIMING[12] - [0,1]
> SMU_13_0_0_ODSETTING_FAN_ZERO_RPM_CONTROL[13] - [0,1]
> SMU_13_0_0_ODSETTING_AUTOUVENGINE[14] - [25,100]
> SMU_13_0_0_ODSETTING_AUTOOCENGINE[15] - [23,100]
> SMU_13_0_0_ODSETTING_AUTOOCMEMORY[16] - [25,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_TEMPERATURE_1[17] - [23,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_SPEED_1[18] - [25,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_TEMPERATURE_2[19] - [23,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_SPEED_2[20] - [25,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_TEMPERATURE_3[21] - [23,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_SPEED_3[22] - [25,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_TEMPERATURE_4[23] - [23,100]
> SMU_13_0_0_ODSETTING_FAN_CURVE_SPEED_4[24] - [0,0]
> SMU_13_0_0_ODSETTING_FAN_CURVE_TEMPERATURE_5[25] - [0,1]
> 

Re: [PATCH v2 3/6] mm: replace vma->vm_flags direct modifications with modifier calls

2023-01-26 Thread Sebastian Reichel
Hi,

On Wed, Jan 25, 2023 at 12:38:48AM -0800, Suren Baghdasaryan wrote:
> Replace direct modifications to vma->vm_flags with calls to modifier
> functions to be able to track flag changes and to keep vma locking
> correctness.
> 
> Signed-off-by: Suren Baghdasaryan 
> ---
> [...]
>  drivers/hsi/clients/cmt_speech.c   |  2 +-
>  120 files changed, 188 insertions(+), 199 deletions(-)
> [...]
> diff --git a/drivers/hsi/clients/cmt_speech.c 
> b/drivers/hsi/clients/cmt_speech.c
> index 8069f795c864..952a31e742a1 100644
> --- a/drivers/hsi/clients/cmt_speech.c
> +++ b/drivers/hsi/clients/cmt_speech.c
> @@ -1264,7 +1264,7 @@ static int cs_char_mmap(struct file *file, struct 
> vm_area_struct *vma)
>   if (vma_pages(vma) != 1)
>   return -EINVAL;
>  
> - vma->vm_flags |= VM_IO | VM_DONTDUMP | VM_DONTEXPAND;
> + set_vm_flags(vma, VM_IO | VM_DONTDUMP | VM_DONTEXPAND);
>   vma->vm_ops = _char_vm_ops;
>   vma->vm_private_data = file->private_data;
>  

Acked-by: Sebastian Reichel 

-- Sebastian


signature.asc
Description: PGP signature


[PATCH] drm/amdgpu/display/mst: fix an unused-variable warning

2023-01-26 Thread Arnd Bergmann
From: Arnd Bergmann 

The newly added code is in an #ifdef, so the variables that
are only used in there cause a warning if CONFIG_DRM_AMD_DC_DCN
is disabled:

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 
'amdgpu_dm_atomic_check':
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9698:43: error: 
unused variable 'mst_state' [-Werror=unused-variable]
 9698 | struct drm_dp_mst_topology_state *mst_state;
  |   ^
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9697:41: error: 
unused variable 'mgr' [-Werror=unused-variable]
 9697 | struct drm_dp_mst_topology_mgr *mgr;
  | ^~~

Fixes: c689e1e362ea ("drm/amdgpu/display/mst: Fix mst_state->pbn_div and slot 
count assignments")
Signed-off-by: Arnd Bergmann 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index be1232356f9e..c966bb05f6c7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9694,8 +9694,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
struct drm_connector_state *old_con_state, *new_con_state;
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+#if defined(CONFIG_DRM_AMD_DC_DCN)
struct drm_dp_mst_topology_mgr *mgr;
struct drm_dp_mst_topology_state *mst_state;
+#endif
struct drm_plane *plane;
struct drm_plane_state *old_plane_state, *new_plane_state;
enum dc_status status;
-- 
2.39.0



Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions

2023-01-26 Thread Suren Baghdasaryan
On Thu, Jan 26, 2023 at 7:09 AM Matthew Wilcox  wrote:
>
> On Thu, Jan 26, 2023 at 04:50:59PM +0200, Mike Rapoport wrote:
> > On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote:
> > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote:
> > > > +/* Use when VMA is not part of the VMA tree and needs no locking */
> > > > +static inline void init_vm_flags(struct vm_area_struct *vma,
> > > > +  unsigned long flags)
> > >
> > > I'd suggest to make it vm_flags_init() etc.
> >
> > Thinking more about it, it will be even clearer to name these 
> > vma_flags_xyz()
>
> Perhaps vma_VERB_flags()?
>
> vma_init_flags()
> vma_reset_flags()
> vma_set_flags()
> vma_clear_flags()
> vma_mod_flags()

Due to excessive email bouncing I posted the v3 of this patchset using
the original per-VMA patchset's distribution list. That might have
dropped Mike from the list. Sorry about that Mike, I'll add you to my
usual list of suspects :)
The v3 is here:
https://lore.kernel.org/all/20230125233554.153109-1-sur...@google.com/
and Andrew did suggest the same renames, so I'll be posting v4 with
those changes later today.
Thanks for the feedback!

>



Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions

2023-01-26 Thread Matthew Wilcox
On Thu, Jan 26, 2023 at 04:50:59PM +0200, Mike Rapoport wrote:
> On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote:
> > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote:
> > > +/* Use when VMA is not part of the VMA tree and needs no locking */
> > > +static inline void init_vm_flags(struct vm_area_struct *vma,
> > > +  unsigned long flags)
> > 
> > I'd suggest to make it vm_flags_init() etc.
> 
> Thinking more about it, it will be even clearer to name these vma_flags_xyz()

Perhaps vma_VERB_flags()?

vma_init_flags()
vma_reset_flags()
vma_set_flags()
vma_clear_flags()
vma_mod_flags()




RE: [PATCH 12/16] drm/amd/display: Add Function delaration in dc_link

2023-01-26 Thread Li, Roman
[AMD Official Use Only - General]

Please fix typo in commit title.

> -Original Message-
> From: Hung, Alex 
> Sent: Wednesday, January 25, 2023 7:32 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Wentland, Harry ; Li, Sun peng (Leo)
> ; Lakha, Bhawanpreet
> ; Siqueira, Rodrigo
> ; Pillai, Aurabindo
> ; Zhuo, Qingqing (Lillian)
> ; Li, Roman ; Lin, Wayne
> ; Wang, Chao-kai (Stylon)
> ; Chiu, Solomon ;
> Kotarac, Pavle ; Gutierrez, Agustin
> ; Ghaddar, Mustapha
> ; Lei, Jun ; Hung, Alex
> 
> Subject: [PATCH 12/16] drm/amd/display: Add Function delaration in dc_link
>
> From: Mustapha Ghaddar 
>
> [WHY]
> Housekeeping cleaning and adding declaration for function to be called from
> DM layer
>
> [HOW]
> Adding public functions to dc_link.h
>
> Reviewed-by: Jun Lei 
> Acked-by: Alex Hung 
> Signed-off-by: Mustapha Ghaddar 
> ---
>  drivers/gpu/drm/amd/display/dc/dc_link.h  | 27 +++
>  .../dc/link/protocols/link_dp_dpia_bw.h   | 24 -
>  2 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_link.h
> b/drivers/gpu/drm/amd/display/dc/dc_link.h
> index 1927eacbfa71..85b57848f5cb 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_link.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_link.h
> @@ -627,4 +627,31 @@ struct fixed31_32
> calculate_sst_avg_time_slots_per_mtp(
>  void setup_dp_hpo_stream(struct pipe_ctx *pipe_ctx, bool enable);  void
> dp_source_sequence_trace(struct dc_link *link, uint8_t dp_test_mode);
>
> +/*
> + *  USB4 DPIA BW ALLOCATION PUBLIC FUNCTIONS  */
> +/*
> + * Send a request from DP-Tx requesting to allocate BW remotely after
> + * allocating it locally. This will get processed by CM and a CB
> +function
> + * will be called.
> + *
> + * @link: pointer to the dc_link struct instance
> + * @req_bw: The requested bw in Kbyte to allocated
> + *
> + * return: none
> + */
> +void dc_link_set_usb4_req_bw_req(struct dc_link *link, int req_bw);
> +
> +/*
> + * CB function for when the status of the Req above is complete. We
> +will
> + * find out the result of allocating on CM and update structs
> +accordingly
> + *
> + * @link: pointer to the dc_link struct instance
> + * @bw: Allocated or Estimated BW depending on the result
> + * @result: Response type
> + *
> + * return: none
> + */
> +void dc_link_get_usb4_req_bw_resp(struct dc_link *link, uint8_t bw,
> +uint8_t result);
> +
>  #endif /* DC_LINK_H_ */
> diff --git
> a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.h
> b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.h
> index 58eb7b581093..832a6dd2c5fa 100644
> --- a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.h
> +++ b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.h
> @@ -44,30 +44,6 @@ enum bw_type {
>   */
>  bool set_dptx_usb4_bw_alloc_support(struct dc_link *link);
>
> -/*
> - * Send a request from DP-Tx requesting to allocate BW remotely after
> - * allocating it locally. This will get processed by CM and a CB function
> - * will be called.
> - *
> - * @link: pointer to the dc_link struct instance
> - * @req_bw: The requested bw in Kbyte to allocated
> - *
> - * return: none
> - */
> -void set_usb4_req_bw_req(struct dc_link *link, int req_bw);
> -
> -/*
> - * CB function for when the status of the Req above is complete. We will
> - * find out the result of allocating on CM and update structs accordingly
> - *
> - * @link: pointer to the dc_link struct instance
> - * @bw: Allocated or Estimated BW depending on the result
> - * @result: Response type
> - *
> - * return: none
> - */
> -void get_usb4_req_bw_resp(struct dc_link *link, uint8_t bw, uint8_t
> result);
> -
>  /*
>   * Return the response_ready flag from dc_link struct
>   *
> --
> 2.39.1



Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions

2023-01-26 Thread Mike Rapoport
On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote:
> On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote:
> > vm_flags are among VMA attributes which affect decisions like VMA merging
> > and splitting. Therefore all vm_flags modifications are performed after
> > taking exclusive mmap_lock to prevent vm_flags updates racing with such
> > operations. Introduce modifier functions for vm_flags to be used whenever
> > flags are updated. This way we can better check and control correct
> > locking behavior during these updates.
> > 
> > Signed-off-by: Suren Baghdasaryan 
> > ---
> >  include/linux/mm.h   | 37 +
> >  include/linux/mm_types.h |  8 +++-
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index c2f62bdce134..b71f2809caac 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct 
> > *vma, struct mm_struct *mm)
> > INIT_LIST_HEAD(>anon_vma_chain);
> >  }
> >  
> > +/* Use when VMA is not part of the VMA tree and needs no locking */
> > +static inline void init_vm_flags(struct vm_area_struct *vma,
> > +unsigned long flags)
> 
> I'd suggest to make it vm_flags_init() etc.

Thinking more about it, it will be even clearer to name these vma_flags_xyz()

> Except that
> 
> Acked-by: Mike Rapoport (IBM) 
> 

--
Sincerely yours,
Mike.



Re: [PATCH v2 6/6] mm: export dump_mm()

2023-01-26 Thread Mike Rapoport
On Wed, Jan 25, 2023 at 12:38:51AM -0800, Suren Baghdasaryan wrote:
> mmap_assert_write_locked() is used in vm_flags modifiers. Because
> mmap_assert_write_locked() uses dump_mm() and vm_flags are sometimes
> modified from from inside a module, it's necessary to export
> dump_mm() function.
> 
> Signed-off-by: Suren Baghdasaryan 

Acked-by: Mike Rapoport (IBM) 

> ---
>  mm/debug.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 9d3d893dc7f4..96d594e16292 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -215,6 +215,7 @@ void dump_mm(const struct mm_struct *mm)
>   mm->def_flags, >def_flags
>   );
>  }
> +EXPORT_SYMBOL(dump_mm);
>  
>  static bool page_init_poisoning __read_mostly = true;
>  
> -- 
> 2.39.1
> 



[PATCH 2/3] drm/amdgpu: always sending PSP messages LOAD_ASD and UNLOAD_TA

2023-01-26 Thread vitaly.prosyak
From: Vitaly Prosyak 

We allow sending PSP messages LOAD_ASD and UNLOAD_TA without
acquiring a lock in drm_dev_enter during driver unload
because we must call drm_dev_unplug as the beginning
of unload driver sequence.
Added WARNING if other PSP messages are sent without a lock.
After this commit, the following commands would work
 -sudo modprobe -r amdgpu
 -sudo modprobe amdgpu

Signed-off-by: Vitaly Prosyak 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index a8391f269cd0..40929f34447c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -606,12 +606,21 @@ psp_cmd_submit_buf(struct psp_context *psp,
int timeout = 2;
bool ras_intr = false;
bool skip_unsupport = false;
+   bool dev_entered;
 
if (psp->adev->no_hw_access)
return 0;
 
-   if (!drm_dev_enter(adev_to_drm(psp->adev), ))
-   return 0;
+   dev_entered = drm_dev_enter(adev_to_drm(psp->adev), );
+   /*
+* We allow sending PSP messages LOAD_ASD and UNLOAD_TA without 
acquiring
+* a lock in drm_dev_enter during driver unload because we must call
+* drm_dev_unplug as the beginning  of unload driver sequence . It is 
very
+* crucial that userspace can't access device instances anymore.
+*/
+   if (!dev_entered)
+   WARN_ON(psp->cmd_buf_mem->cmd_id != GFX_CMD_ID_LOAD_ASD &&
+   psp->cmd_buf_mem->cmd_id != GFX_CMD_ID_UNLOAD_TA);
 
memset(psp->cmd_buf_mem, 0, PSP_CMD_BUFFER_SIZE);
 
@@ -676,7 +685,8 @@ psp_cmd_submit_buf(struct psp_context *psp,
}
 
 exit:
-   drm_dev_exit(idx);
+   if (dev_entered)
+   drm_dev_exit(idx);
return ret;
 }
 
-- 
2.25.1



[PATCH 1/3] Revert "drm/amdgpu: TA unload messages are not actually sent to psp when amdgpu is uninstalled"

2023-01-26 Thread vitaly.prosyak
From: Vitaly Prosyak 

This reverts commit fac53471d0ea9693d314aa2df08d62b2e7e3a0f8.
The following change: move the drm_dev_unplug call after
amdgpu_driver_unload_kms in amdgpu_pci_remove. The reason is
the following: amdgpu_pci_remove calls drm_dev_unregister
and it should be called first to ensure userspace can't access the
device instance anymore. If we call drm_dev_unplug after
amdgpu_driver_unload_kms then we observe IGT PCI software unplug
test failure (kernel hung) for all ASICs. This is how this
regression was found.

After this revert, the following commands do work not, but it would
be fixed in the next commit:
 - sudo modprobe -r amdgpu
 - sudo modprobe amdgpu

Signed-off-by: Vitaly Prosyak 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 0f9a5b12c3a5..a10b627c8357 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4031,7 +4031,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 
amdgpu_gart_dummy_page_fini(adev);
 
-   amdgpu_device_unmap_mmio(adev);
+   if (drm_dev_is_unplugged(adev_to_drm(adev)))
+   amdgpu_device_unmap_mmio(adev);
 
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index a75dba2caeca..7edbaa90fac9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2227,6 +2227,8 @@ amdgpu_pci_remove(struct pci_dev *pdev)
struct drm_device *dev = pci_get_drvdata(pdev);
struct amdgpu_device *adev = drm_to_adev(dev);
 
+   drm_dev_unplug(dev);
+
if (adev->pm.rpm_mode != AMDGPU_RUNPM_NONE) {
pm_runtime_get_sync(dev->dev);
pm_runtime_forbid(dev->dev);
@@ -2266,8 +2268,6 @@ amdgpu_pci_remove(struct pci_dev *pdev)
 
amdgpu_driver_unload_kms(dev);
 
-   drm_dev_unplug(dev);
-
/*
 * Flush any in flight DMA operations from device.
 * Clear the Bus Master Enable bit and then wait on the PCIe Device
-- 
2.25.1



Re: [PATCH 3/3] drm/amdgpu: use pci_dev_is_disconnected

2023-01-26 Thread vitaly prosyak



On 2023-01-26 04:20, Christian König wrote:

Am 25.01.23 um 18:16 schrieb vitaly.pros...@amd.com:

From: Vitaly Prosyak 

Added condition for pci_dev_is_disconnected and keeps
drm_dev_is_unplugged to check whether we should unmap MMIO.
Suggested by Alex regarding pci_dev_is_disconnected.
Suggested by Christian keeping drm_dev_is_unplugged.

Signed-off-by: Vitaly Prosyak 
Reviewed-by Alex Deucher 
Reviewed-by Christian Koenig 


Did I gave my rb with this include path below???
Explicitly there was no RB, but there was a private thread which does 
not include this include.



Change-Id: I618c471cd398437d4ed6dec6d22be78e12683ae6
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

index a10b627c8357..d3568e1ded23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -78,6 +78,8 @@
    #include 
  +#include "../../../../pci/pci.h"


That include path looks more than suspicious.

If we want to use pci_dev_is_disconnected() outside of the pci 
subsystem we should probably move it to include/linux/pci.h


Alright, I would drop this patch until the  It would be a separate patch 
to create a new:


include/linux/pci.h  with the following content:
#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_PCI)
bool pci_dev_is_disconnected(const struct pci_dev *dev)
{
    return dev->error_state == pci_channel_io_perm_failure;
}
#else
bool pci_dev_is_disconnected(const struct pci_dev *dev)
{
    return true;
}

#endif

I am not sure about enablement  of amdgpu when PCI is off in the config, 
but this is related to move pci_dev_is_disconnected into separate file 
and receive review from PCI system/.




Regards,
Christian.


+
  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
  MODULE_FIRMWARE("amdgpu/vega12_gpu_info.bin");
  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
@@ -4031,7 +4033,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device 
*adev)

    amdgpu_gart_dummy_page_fini(adev);
  -    if (drm_dev_is_unplugged(adev_to_drm(adev)))
+    if (pci_dev_is_disconnected(adev->pdev) &&
+    drm_dev_is_unplugged(adev_to_drm(adev)))
  amdgpu_device_unmap_mmio(adev);
    }




[bug report] drm/amd/display: move eDP panel control logic to link_edp_panel_control

2023-01-26 Thread Dan Carpenter
Hello Wenjing Liu,

The patch 0078c924e733: "drm/amd/display: move eDP panel control
logic to link_edp_panel_control" from Dec 19, 2022, leads to the
following Smatch static checker warning:


drivers/gpu/drm/amd/amdgpu/../display/dc/link/protocols/link_edp_panel_control.c:353
link_edp_receiver_ready_T9() warn: potential negative cast to bool 'result'


drivers/gpu/drm/amd/amdgpu/../display/dc/link/protocols/link_edp_panel_control.c:388
link_edp_receiver_ready_T7() warn: potential negative cast to bool 'result'

drivers/gpu/drm/amd/amdgpu/../display/dc/link/protocols/link_edp_panel_control.c
331 bool link_edp_receiver_ready_T9(struct dc_link *link)

This function returns DC_OK (1) on success or positive on error or -1
on unknown error.  So casting it to bool means it always returns true.

332 {
333 unsigned int tries = 0;
334 unsigned char sinkstatus = 0;
335 unsigned char edpRev = 0;
336 enum dc_status result = DC_OK;
337 
338 result = core_link_read_dpcd(link, DP_EDP_DPCD_REV, , 
sizeof(edpRev));
339 
340 /* start from eDP version 1.2, SINK_STAUS indicate the sink is 
ready.*/
341 if (result == DC_OK && edpRev >= DP_EDP_12) {
342 do {
343 sinkstatus = 1;
344 result = core_link_read_dpcd(link, 
DP_SINK_STATUS, , sizeof(sinkstatus));
345 if (sinkstatus == 0)
346 break;
347 if (result != DC_OK)
348 break;
349 udelay(100); //MAx T9
350 } while (++tries < 50);
351 }
352 
--> 353 return result;
354 }

regards,
dan carpenter


Re: [PATCH] drm/amd: Allow s0ix without BIOS support

2023-01-26 Thread Rafael Ávila de Espíndola
Mario Limonciello  writes:

> We guard the suspend entry code from running unless we have proper
> BIOS support for either S3 mode or s0ix mode.
>
> If a user's system doesn't support either of these modes the kernel
> still does offer s2idle in `/sys/power/mem_sleep` so there is an
> expectation from users that it works even if the power consumption
> remains very high.
>
> Rafael Ávila de Espíndola reports that a system of his has a
> non-functional graphics stack after resuming.  That system doesn't
> support S3 and the FADT doesn't indicate support for low power idle.
>
> Through some experimentation it was concluded that even without the
> hardware s0i3 support provided by the amd_pmc driver the power
> consumption over suspend is decreased by running amdgpu's s0ix
> suspend routine.
>
> The numbers over suspend showed:
> * No patch: 9.2W
> * Skip amdgpu suspend entirely: 10.5W
> * Run amdgpu s0ix routine: 7.7W
>
> As this does improve the power, remove some of the guard rails in
> `amdgpu_acpi.c` for only running s0ix suspend routines in the right
> circumstances.
>
> However if this turns out to cause regressions for anyone, we should
> revert this change and instead opt for skipping suspend/resume routines
> entirely or try to fix the underlying behavior that makes graphics fail
> after resume without underlying platform support.
>
> Reported-by: Rafael Ávila de Espíndola 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2364
> Signed-off-by: Mario Limonciello 

Acked-by: Rafael Ávila de Espíndola 

I have tested this patch on a Gigabyte B550I AORUS PRO AX with a 4350G
and can confirm that the errors are gone and the power consumption during
suspend is down to 7.7 W.

Thanks,
Rafael


> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 57b5e11446c65..fa7375b97fd47 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1079,20 +1079,16 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device 
> *adev)
>* S0ix even though the system is suspending to idle, so return false
>* in that case.
>*/
> - if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
> + if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
>   dev_warn_once(adev->dev,
> "Power consumption will be higher as BIOS has not 
> been configured for suspend-to-idle.\n"
> "To use suspend-to-idle change the sleep mode in 
> BIOS setup.\n");
> - return false;
> - }
>  
>  #if !IS_ENABLED(CONFIG_AMD_PMC)
>   dev_warn_once(adev->dev,
> "Power consumption will be higher as the kernel has not 
> been compiled with CONFIG_AMD_PMC.\n");
> - return false;
> -#else
> - return true;
>  #endif /* CONFIG_AMD_PMC */
> + return true;
>  }
>  
>  #endif /* CONFIG_SUSPEND */
> -- 
> 2.25.1


Re: [PATCH v4 3/4] drm/amdgpu: Movie the amdgpu_gtt_mgr start and size from pages to bytes

2023-01-26 Thread Christian König

Am 25.01.23 um 16:20 schrieb Somalapuram Amaranath:

To support GTT manager amdgpu_res_first, amdgpu_res_next
from pages to bytes and clean up PAGE_SHIFT operation.
Change the GTT manager init and allocate from pages to bytes
v1 -> v2: reorder patch sequence
v3 -> v4: reorder patch sequence


That won't work like this and break the driver because you only have 
halve of the necessary changes inside this patch here.


Please *never ever* again send out incomplete patches like this.

Christian.



Signed-off-by: Somalapuram Amaranath 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c| 13 +++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h |  8 
  2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 44367f03316f..a1fbfc5984d8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -116,7 +116,6 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager 
*man,
  struct ttm_resource **res)
  {
struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
-   uint32_t num_pages = PFN_UP(tbo->base.size);
struct ttm_range_mgr_node *node;
int r;
  
@@ -134,8 +133,10 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,

if (place->lpfn) {
spin_lock(>lock);
r = drm_mm_insert_node_in_range(>mm, >mm_nodes[0],
-   num_pages, tbo->page_alignment,
-   0, place->fpfn, place->lpfn,
+   tbo->base.size,
+   tbo->page_alignment << 
PAGE_SHIFT, 0,
+   place->fpfn << PAGE_SHIFT,
+   place->lpfn << PAGE_SHIFT,
DRM_MM_INSERT_BEST);
spin_unlock(>lock);
if (unlikely(r))
@@ -144,7 +145,7 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager 
*man,
node->base.start = node->mm_nodes[0].start;
} else {
node->mm_nodes[0].start = 0;
-   node->mm_nodes[0].size = PFN_UP(node->base.size);
+   node->mm_nodes[0].size = node->base.size;
node->base.start = AMDGPU_BO_INVALID_OFFSET;
}
  
@@ -285,8 +286,8 @@ int amdgpu_gtt_mgr_init(struct amdgpu_device *adev, uint64_t gtt_size)
  
  	ttm_resource_manager_init(man, >mman.bdev, gtt_size);
  
-	start = AMDGPU_GTT_MAX_TRANSFER_SIZE * AMDGPU_GTT_NUM_TRANSFER_WINDOWS;

-   size = (adev->gmc.gart_size >> PAGE_SHIFT) - start;
+   start = (AMDGPU_GTT_MAX_TRANSFER_SIZE * AMDGPU_GTT_NUM_TRANSFER_WINDOWS) 
<< PAGE_SHIFT;
+   size = adev->gmc.gart_size - start;
drm_mm_init(>mm, start, size);
spin_lock_init(>lock);
  
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h

index 5c4f93ee0c57..5c78f0b09351 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
@@ -94,8 +94,8 @@ static inline void amdgpu_res_first(struct ttm_resource *res,
while (start >= node->size << PAGE_SHIFT)
start -= node++->size << PAGE_SHIFT;
  
-		cur->start = (node->start << PAGE_SHIFT) + start;

-   cur->size = min((node->size << PAGE_SHIFT) - start, size);
+   cur->start = node->start + start;
+   cur->size = min(node->size - start, size);
cur->remaining = size;
cur->node = node;
break;
@@ -155,8 +155,8 @@ static inline void amdgpu_res_next(struct amdgpu_res_cursor 
*cur, uint64_t size)
node = cur->node;
  
  		cur->node = ++node;

-   cur->start = node->start << PAGE_SHIFT;
-   cur->size = min(node->size << PAGE_SHIFT, cur->remaining);
+   cur->start = node->start;
+   cur->size = min(node->size, cur->remaining);
break;
default:
return;




Re: [PATCH v2 5/6] mm: introduce mod_vm_flags_nolock and use it in untrack_pfn

2023-01-26 Thread Mike Rapoport
On Wed, Jan 25, 2023 at 12:38:50AM -0800, Suren Baghdasaryan wrote:
> In cases when VMA flags are modified after VMA was isolated and mmap_lock
> was downgraded, flags modifications would result in an assertion because
> mmap write lock is not held.
> Introduce mod_vm_flags_nolock to be used in such situation.

vm_flags_mod_nolock?

> Pass a hint to untrack_pfn to conditionally use mod_vm_flags_nolock for
> flags modification and to avoid assertion.
> 
> Signed-off-by: Suren Baghdasaryan 
> ---
>  arch/x86/mm/pat/memtype.c | 10 +++---
>  include/linux/mm.h| 12 +---
>  include/linux/pgtable.h   |  5 +++--
>  mm/memory.c   | 13 +++--
>  mm/memremap.c |  4 ++--
>  mm/mmap.c | 16 ++--
>  6 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index ae9645c900fa..d8adc0b42cf2 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -1046,7 +1046,7 @@ void track_pfn_insert(struct vm_area_struct *vma, 
> pgprot_t *prot, pfn_t pfn)
>   * can be for the entire vma (in which case pfn, size are zero).
>   */
>  void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
> -  unsigned long size)
> +  unsigned long size, bool mm_wr_locked)
>  {
>   resource_size_t paddr;
>   unsigned long prot;
> @@ -1065,8 +1065,12 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned 
> long pfn,
>   size = vma->vm_end - vma->vm_start;
>   }
>   free_pfn_range(paddr, size);
> - if (vma)
> - clear_vm_flags(vma, VM_PAT);
> + if (vma) {
> + if (mm_wr_locked)
> + clear_vm_flags(vma, VM_PAT);
> + else
> + mod_vm_flags_nolock(vma, 0, VM_PAT);
> + }
>  }
>  
>  /*
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 55335edd1373..48d49930c411 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -656,12 +656,18 @@ static inline void clear_vm_flags(struct vm_area_struct 
> *vma,
>   vma->vm_flags &= ~flags;
>  }
>  
> +static inline void mod_vm_flags_nolock(struct vm_area_struct *vma,
> +unsigned long set, unsigned long clear)
> +{
> + vma->vm_flags |= set;
> + vma->vm_flags &= ~clear;
> +}
> +
>  static inline void mod_vm_flags(struct vm_area_struct *vma,
>   unsigned long set, unsigned long clear)
>  {
>   mmap_assert_write_locked(vma->vm_mm);
> - vma->vm_flags |= set;
> - vma->vm_flags &= ~clear;
> + mod_vm_flags_nolock(vma, set, clear);
>  }
>  
>  static inline void vma_set_anonymous(struct vm_area_struct *vma)
> @@ -2087,7 +2093,7 @@ static inline void zap_vma_pages(struct vm_area_struct 
> *vma)
>  }
>  void unmap_vmas(struct mmu_gather *tlb, struct maple_tree *mt,
>   struct vm_area_struct *start_vma, unsigned long start,
> - unsigned long end);
> + unsigned long end, bool mm_wr_locked);
>  
>  struct mmu_notifier_range;
>  
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 5fd45454c073..c63cd44777ec 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1185,7 +1185,8 @@ static inline int track_pfn_copy(struct vm_area_struct 
> *vma)
>   * can be for the entire vma (in which case pfn, size are zero).
>   */
>  static inline void untrack_pfn(struct vm_area_struct *vma,
> -unsigned long pfn, unsigned long size)
> +unsigned long pfn, unsigned long size,
> +bool mm_wr_locked)
>  {
>  }
>  
> @@ -1203,7 +1204,7 @@ extern void track_pfn_insert(struct vm_area_struct 
> *vma, pgprot_t *prot,
>pfn_t pfn);
>  extern int track_pfn_copy(struct vm_area_struct *vma);
>  extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
> - unsigned long size);
> + unsigned long size, bool mm_wr_locked);
>  extern void untrack_pfn_moved(struct vm_area_struct *vma);
>  #endif
>  
> diff --git a/mm/memory.c b/mm/memory.c
> index d6902065e558..5b11b50e2c4a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1613,7 +1613,7 @@ void unmap_page_range(struct mmu_gather *tlb,
>  static void unmap_single_vma(struct mmu_gather *tlb,
>   struct vm_area_struct *vma, unsigned long start_addr,
>   unsigned long end_addr,
> - struct zap_details *details)
> + struct zap_details *details, bool mm_wr_locked)
>  {
>   unsigned long start = max(vma->vm_start, start_addr);
>   unsigned long end;
> @@ -1628,7 +1628,7 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>   uprobe_munmap(vma, start, end);
>  
>   if (unlikely(vma->vm_flags & VM_PFNMAP))
> - untrack_pfn(vma, 0, 0);
> + untrack_pfn(vma, 0, 0, 

Re: [PATCH v2 4/6] mm: replace vma->vm_flags indirect modification in ksm_madvise

2023-01-26 Thread Mike Rapoport
On Wed, Jan 25, 2023 at 12:38:49AM -0800, Suren Baghdasaryan wrote:
> Replace indirect modifications to vma->vm_flags with calls to modifier
> functions to be able to track flag changes and to keep vma locking
> correctness. Add a BUG_ON check in ksm_madvise() to catch indirect
> vm_flags modification attempts.
> 
> Signed-off-by: Suren Baghdasaryan 

Acked-by: Mike Rapoport (IBM) 

> ---
>  arch/powerpc/kvm/book3s_hv_uvmem.c | 5 -
>  arch/s390/mm/gmap.c| 5 -
>  mm/khugepaged.c| 2 ++
>  mm/ksm.c   | 2 ++
>  4 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c 
> b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 1d67baa5557a..325a7a47d348 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -393,6 +393,7 @@ static int kvmppc_memslot_page_merge(struct kvm *kvm,
>  {
>   unsigned long gfn = memslot->base_gfn;
>   unsigned long end, start = gfn_to_hva(kvm, gfn);
> + unsigned long vm_flags;
>   int ret = 0;
>   struct vm_area_struct *vma;
>   int merge_flag = (merge) ? MADV_MERGEABLE : MADV_UNMERGEABLE;
> @@ -409,12 +410,14 @@ static int kvmppc_memslot_page_merge(struct kvm *kvm,
>   ret = H_STATE;
>   break;
>   }
> + vm_flags = vma->vm_flags;
>   ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -   merge_flag, >vm_flags);
> +   merge_flag, _flags);
>   if (ret) {
>   ret = H_STATE;
>   break;
>   }
> + reset_vm_flags(vma, vm_flags);
>   start = vma->vm_end;
>   } while (end > vma->vm_end);
>  
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 3a695b8a1e3c..d5eb47dcdacb 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2587,14 +2587,17 @@ int gmap_mark_unmergeable(void)
>  {
>   struct mm_struct *mm = current->mm;
>   struct vm_area_struct *vma;
> + unsigned long vm_flags;
>   int ret;
>   VMA_ITERATOR(vmi, mm, 0);
>  
>   for_each_vma(vmi, vma) {
> + vm_flags = vma->vm_flags;
>   ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -   MADV_UNMERGEABLE, >vm_flags);
> +   MADV_UNMERGEABLE, _flags);
>   if (ret)
>   return ret;
> + reset_vm_flags(vma, vm_flags);
>   }
>   mm->def_flags &= ~VM_MERGEABLE;
>   return 0;
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 8abc59345bf2..76b24cd0c179 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -354,6 +354,8 @@ struct attribute_group khugepaged_attr_group = {
>  int hugepage_madvise(struct vm_area_struct *vma,
>unsigned long *vm_flags, int advice)
>  {
> + /* vma->vm_flags can be changed only using modifier functions */
> + BUG_ON(vm_flags == >vm_flags);
>   switch (advice) {
>   case MADV_HUGEPAGE:
>  #ifdef CONFIG_S390
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 04f1c8c2df11..992b2be9f5e6 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2573,6 +2573,8 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned 
> long start,
>   struct mm_struct *mm = vma->vm_mm;
>   int err;
>  
> + /* vma->vm_flags can be changed only using modifier functions */
> + BUG_ON(vm_flags == >vm_flags);
>   switch (advice) {
>   case MADV_MERGEABLE:
>   /*
> -- 
> 2.39.1
> 
> 



Re: [PATCH v2 3/6] mm: replace vma->vm_flags direct modifications with modifier calls

2023-01-26 Thread Mike Rapoport
On Wed, Jan 25, 2023 at 12:38:48AM -0800, Suren Baghdasaryan wrote:
> Replace direct modifications to vma->vm_flags with calls to modifier
> functions to be able to track flag changes and to keep vma locking
> correctness.
> 
> Signed-off-by: Suren Baghdasaryan 

Acked-by: Mike Rapoport (IBM) 

> ---
>  arch/arm/kernel/process.c  |  2 +-
>  arch/ia64/mm/init.c|  8 
>  arch/loongarch/include/asm/tlb.h   |  2 +-
>  arch/powerpc/kvm/book3s_xive_native.c  |  2 +-
>  arch/powerpc/mm/book3s64/subpage_prot.c|  2 +-
>  arch/powerpc/platforms/book3s/vas-api.c|  2 +-
>  arch/powerpc/platforms/cell/spufs/file.c   | 14 +++---
>  arch/s390/mm/gmap.c|  3 +--
>  arch/x86/entry/vsyscall/vsyscall_64.c  |  2 +-
>  arch/x86/kernel/cpu/sgx/driver.c   |  2 +-
>  arch/x86/kernel/cpu/sgx/virt.c |  2 +-
>  arch/x86/mm/pat/memtype.c  |  6 +++---
>  arch/x86/um/mem_32.c   |  2 +-
>  drivers/acpi/pfr_telemetry.c   |  2 +-
>  drivers/android/binder.c   |  3 +--
>  drivers/char/mspec.c   |  2 +-
>  drivers/crypto/hisilicon/qm.c  |  2 +-
>  drivers/dax/device.c   |  2 +-
>  drivers/dma/idxd/cdev.c|  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c|  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c   |  4 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c  |  4 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c|  4 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c   |  4 ++--
>  drivers/gpu/drm/drm_gem.c  |  2 +-
>  drivers/gpu/drm/drm_gem_dma_helper.c   |  3 +--
>  drivers/gpu/drm/drm_gem_shmem_helper.c |  2 +-
>  drivers/gpu/drm/drm_vm.c   |  8 
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c  |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_gem.c|  4 ++--
>  drivers/gpu/drm/gma500/framebuffer.c   |  2 +-
>  drivers/gpu/drm/i810/i810_dma.c|  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c |  2 +-
>  drivers/gpu/drm/msm/msm_gem.c  |  2 +-
>  drivers/gpu/drm/omapdrm/omap_gem.c |  3 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c|  3 +--
>  drivers/gpu/drm/tegra/gem.c|  5 ++---
>  drivers/gpu/drm/ttm/ttm_bo_vm.c|  3 +--
>  drivers/gpu/drm/virtio/virtgpu_vram.c  |  2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c   |  2 +-
>  drivers/gpu/drm/xen/xen_drm_front_gem.c|  3 +--
>  drivers/hsi/clients/cmt_speech.c   |  2 +-
>  drivers/hwtracing/intel_th/msu.c   |  2 +-
>  drivers/hwtracing/stm/core.c   |  2 +-
>  drivers/infiniband/hw/hfi1/file_ops.c  |  4 ++--
>  drivers/infiniband/hw/mlx5/main.c  |  4 ++--
>  drivers/infiniband/hw/qib/qib_file_ops.c   | 13 ++---
>  drivers/infiniband/hw/usnic/usnic_ib_verbs.c   |  2 +-
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c|  2 +-
>  .../media/common/videobuf2/videobuf2-dma-contig.c  |  2 +-
>  drivers/media/common/videobuf2/videobuf2-vmalloc.c |  2 +-
>  drivers/media/v4l2-core/videobuf-dma-contig.c  |  2 +-
>  drivers/media/v4l2-core/videobuf-dma-sg.c  |  4 ++--
>  drivers/media/v4l2-core/videobuf-vmalloc.c |  2 +-
>  drivers/misc/cxl/context.c |  2 +-
>  drivers/misc/habanalabs/common/memory.c|  2 +-
>  drivers/misc/habanalabs/gaudi/gaudi.c  |  4 ++--
>  drivers/misc/habanalabs/gaudi2/gaudi2.c|  8 
>  drivers/misc/habanalabs/goya/goya.c|  4 ++--
>  drivers/misc/ocxl/context.c|  4 ++--
>  drivers/misc/ocxl/sysfs.c  |  2 +-
>  drivers/misc/open-dice.c   |  4 ++--
>  drivers/misc/sgi-gru/grufile.c |  4 ++--
>  drivers/misc/uacce/uacce.c |  2 +-
>  drivers/sbus/char/oradax.c |  2 +-
>  drivers/scsi/cxlflash/ocxl_hw.c|  2 +-
>  drivers/scsi/sg.c  |  2 +-
>  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c |  2 +-
>  drivers/staging/media/deprecated/meye/meye.c   |  4 ++--
>  .../media/deprecated/stkwebcam/stk-webcam.c|  2 +-
>  drivers/target/target_core_user.c  |  2 +-
>  drivers/uio/uio.c  |  2 +-
>  drivers/usb/core/devio.c   |  3 +--
>  

Re: [PATCH v2 2/6] mm: replace VM_LOCKED_CLEAR_MASK with VM_LOCKED_MASK

2023-01-26 Thread Mike Rapoport
On Wed, Jan 25, 2023 at 12:38:47AM -0800, Suren Baghdasaryan wrote:
> To simplify the usage of VM_LOCKED_CLEAR_MASK in clear_vm_flags(),
> replace it with VM_LOCKED_MASK bitmask and convert all users.
> 
> Signed-off-by: Suren Baghdasaryan 

Acked-by: Mike Rapoport (IBM) 

> ---
>  include/linux/mm.h | 4 ++--
>  kernel/fork.c  | 2 +-
>  mm/hugetlb.c   | 4 ++--
>  mm/mlock.c | 6 +++---
>  mm/mmap.c  | 6 +++---
>  mm/mremap.c| 2 +-
>  6 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b71f2809caac..da62bdd627bf 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -421,8 +421,8 @@ extern unsigned int kobjsize(const void *objp);
>  /* This mask defines which mm->def_flags a process can inherit its parent */
>  #define VM_INIT_DEF_MASK VM_NOHUGEPAGE
>  
> -/* This mask is used to clear all the VMA flags used by mlock */
> -#define VM_LOCKED_CLEAR_MASK (~(VM_LOCKED | VM_LOCKONFAULT))
> +/* This mask represents all the VMA flag bits used by mlock */
> +#define VM_LOCKED_MASK   (VM_LOCKED | VM_LOCKONFAULT)
>  
>  /* Arch-specific flags to clear when updating VM flags on protection change 
> */
>  #ifndef VM_ARCH_CLEAR
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6683c1b0f460..03d472051236 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -669,7 +669,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>   tmp->anon_vma = NULL;
>   } else if (anon_vma_fork(tmp, mpnt))
>   goto fail_nomem_anon_vma_fork;
> - tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT);
> + clear_vm_flags(tmp, VM_LOCKED_MASK);
>   file = tmp->vm_file;
>   if (file) {
>   struct address_space *mapping = file->f_mapping;
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d20c8b09890e..4ecdbad9a451 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6973,8 +6973,8 @@ static unsigned long page_table_shareable(struct 
> vm_area_struct *svma,
>   unsigned long s_end = sbase + PUD_SIZE;
>  
>   /* Allow segments to share if only one is marked locked */
> - unsigned long vm_flags = vma->vm_flags & VM_LOCKED_CLEAR_MASK;
> - unsigned long svm_flags = svma->vm_flags & VM_LOCKED_CLEAR_MASK;
> + unsigned long vm_flags = vma->vm_flags & ~VM_LOCKED_MASK;
> + unsigned long svm_flags = svma->vm_flags & ~VM_LOCKED_MASK;
>  
>   /*
>* match the virtual addresses, permission and the alignment of the
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 0336f52e03d7..5c4fff93cd6b 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -497,7 +497,7 @@ static int apply_vma_lock_flags(unsigned long start, 
> size_t len,
>   if (vma->vm_start != tmp)
>   return -ENOMEM;
>  
> - newflags = vma->vm_flags & VM_LOCKED_CLEAR_MASK;
> + newflags = vma->vm_flags & ~VM_LOCKED_MASK;
>   newflags |= flags;
>   /* Here we know that  vma->vm_start <= nstart < vma->vm_end. */
>   tmp = vma->vm_end;
> @@ -661,7 +661,7 @@ static int apply_mlockall_flags(int flags)
>   struct vm_area_struct *vma, *prev = NULL;
>   vm_flags_t to_add = 0;
>  
> - current->mm->def_flags &= VM_LOCKED_CLEAR_MASK;
> + current->mm->def_flags &= ~VM_LOCKED_MASK;
>   if (flags & MCL_FUTURE) {
>   current->mm->def_flags |= VM_LOCKED;
>  
> @@ -681,7 +681,7 @@ static int apply_mlockall_flags(int flags)
>   for_each_vma(vmi, vma) {
>   vm_flags_t newflags;
>  
> - newflags = vma->vm_flags & VM_LOCKED_CLEAR_MASK;
> + newflags = vma->vm_flags & ~VM_LOCKED_MASK;
>   newflags |= to_add;
>  
>   /* Ignore errors */
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d4abc6feced1..323bd253b25a 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2671,7 +2671,7 @@ unsigned long mmap_region(struct file *file, unsigned 
> long addr,
>   if ((vm_flags & VM_SPECIAL) || vma_is_dax(vma) ||
>   is_vm_hugetlb_page(vma) ||
>   vma == get_gate_vma(current->mm))
> - vma->vm_flags &= VM_LOCKED_CLEAR_MASK;
> + clear_vm_flags(vma, VM_LOCKED_MASK);
>   else
>   mm->locked_vm += (len >> PAGE_SHIFT);
>   }
> @@ -3340,8 +3340,8 @@ static struct vm_area_struct *__install_special_mapping(
>   vma->vm_start = addr;
>   vma->vm_end = addr + len;
>  
> - vma->vm_flags = vm_flags | mm->def_flags | VM_DONTEXPAND | VM_SOFTDIRTY;
> - vma->vm_flags &= VM_LOCKED_CLEAR_MASK;
> + init_vm_flags(vma, (vm_flags | mm->def_flags |
> +   VM_DONTEXPAND | VM_SOFTDIRTY) & ~VM_LOCKED_MASK);
>   vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  
>   vma->vm_ops = ops;
> diff --git a/mm/mremap.c 

Re: [PATCH 3/3] drm/amdgpu: use pci_dev_is_disconnected

2023-01-26 Thread Christian König

Am 25.01.23 um 18:16 schrieb vitaly.pros...@amd.com:

From: Vitaly Prosyak 

Added condition for pci_dev_is_disconnected and keeps
drm_dev_is_unplugged to check whether we should unmap MMIO.
Suggested by Alex regarding pci_dev_is_disconnected.
Suggested by Christian keeping drm_dev_is_unplugged.

Signed-off-by: Vitaly Prosyak 
Reviewed-by Alex Deucher 
Reviewed-by Christian Koenig 


Did I gave my rb with this include path below???


Change-Id: I618c471cd398437d4ed6dec6d22be78e12683ae6
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a10b627c8357..d3568e1ded23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -78,6 +78,8 @@
  
  #include 
  
+#include "../../../../pci/pci.h"


That include path looks more than suspicious.

If we want to use pci_dev_is_disconnected() outside of the pci subsystem 
we should probably move it to include/linux/pci.h


Regards,
Christian.


+
  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
  MODULE_FIRMWARE("amdgpu/vega12_gpu_info.bin");
  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
@@ -4031,7 +4033,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
  
  	amdgpu_gart_dummy_page_fini(adev);
  
-	if (drm_dev_is_unplugged(adev_to_drm(adev)))

+   if (pci_dev_is_disconnected(adev->pdev) &&
+   drm_dev_is_unplugged(adev_to_drm(adev)))
amdgpu_device_unmap_mmio(adev);
  
  }




Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions

2023-01-26 Thread Mike Rapoport
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote:
> vm_flags are among VMA attributes which affect decisions like VMA merging
> and splitting. Therefore all vm_flags modifications are performed after
> taking exclusive mmap_lock to prevent vm_flags updates racing with such
> operations. Introduce modifier functions for vm_flags to be used whenever
> flags are updated. This way we can better check and control correct
> locking behavior during these updates.
> 
> Signed-off-by: Suren Baghdasaryan 
> ---
>  include/linux/mm.h   | 37 +
>  include/linux/mm_types.h |  8 +++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c2f62bdce134..b71f2809caac 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, 
> struct mm_struct *mm)
>   INIT_LIST_HEAD(>anon_vma_chain);
>  }
>  
> +/* Use when VMA is not part of the VMA tree and needs no locking */
> +static inline void init_vm_flags(struct vm_area_struct *vma,
> +  unsigned long flags)

I'd suggest to make it vm_flags_init() etc.
Except that

Acked-by: Mike Rapoport (IBM) 

> +{
> + vma->vm_flags = flags;
> +}
> +
> +/* Use when VMA is part of the VMA tree and modifications need coordination 
> */
> +static inline void reset_vm_flags(struct vm_area_struct *vma,
> +   unsigned long flags)
> +{
> + mmap_assert_write_locked(vma->vm_mm);
> + init_vm_flags(vma, flags);
> +}
> +
> +static inline void set_vm_flags(struct vm_area_struct *vma,
> + unsigned long flags)
> +{
> + mmap_assert_write_locked(vma->vm_mm);
> + vma->vm_flags |= flags;
> +}
> +
> +static inline void clear_vm_flags(struct vm_area_struct *vma,
> +   unsigned long flags)
> +{
> + mmap_assert_write_locked(vma->vm_mm);
> + vma->vm_flags &= ~flags;
> +}
> +
> +static inline void mod_vm_flags(struct vm_area_struct *vma,
> + unsigned long set, unsigned long clear)
> +{
> + mmap_assert_write_locked(vma->vm_mm);
> + vma->vm_flags |= set;
> + vma->vm_flags &= ~clear;
> +}
> +
>  static inline void vma_set_anonymous(struct vm_area_struct *vma)
>  {
>   vma->vm_ops = NULL;
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 2d6d790d9bed..6c7c70bf50dd 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -491,7 +491,13 @@ struct vm_area_struct {
>* See vmf_insert_mixed_prot() for discussion.
>*/
>   pgprot_t vm_page_prot;
> - unsigned long vm_flags; /* Flags, see mm.h. */
> +
> + /*
> +  * Flags, see mm.h.
> +  * WARNING! Do not modify directly.
> +  * Use {init|reset|set|clear|mod}_vm_flags() functions instead.
> +  */
> + unsigned long vm_flags;
>  
>   /*
>* For areas with an address space and backing store,
> -- 
> 2.39.1
> 
>