Re: [PATCH 3/3] drm/amdgpu: Implement bad_page_threshold = -2 case

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 10/21/2021 7:26 PM, Russell, Kent wrote:
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> > Sent: Thursday, October 21, 2021 1:25 AM
> > To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Tuikov, Luben <Luben.Tuikov@xxxxxxx>; Joshi, Mukul <Mukul.Joshi@xxxxxxx>
> > Subject: Re: [PATCH 3/3] drm/amdgpu: Implement bad_page_threshold = -2 case
> >
> >
> >
> > On 10/20/2021 10:05 PM, Kent Russell wrote:
> > > If the bad_page_threshold kernel parameter is set to -2,
> > > continue to post the GPU. Print a warning to dmesg that this action has
> > > been done, and that page retirement will obviously not work for said GPU
> > >
> > > Cc: Luben Tuikov <luben.tuikov@xxxxxxx>
> > > Cc: Mukul Joshi <Mukul.Joshi@xxxxxxx>
> > > Signed-off-by: Kent Russell <kent.russell@xxxxxxx>
> > > ---
> > >    drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 13 +++++++++----
> > >    1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> > > index 1ede0f0d6f55..31852330c1db 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> > > @@ -1115,11 +1115,16 @@ int amdgpu_ras_eeprom_init(struct
> > amdgpu_ras_eeprom_control *control,
> > >    			res = amdgpu_ras_eeprom_correct_header_tag(control,
> > >    								   RAS_TABLE_HDR_VAL);
> > >    		} else {
> > > -			*exceed_err_limit = true;
> > > -			dev_err(adev->dev,
> > > -				"RAS records:%d exceed threshold:%d, "
> > > -				"GPU will not be initialized. Replace this GPU or increase the
> > threshold",
> > > +			dev_err(adev->dev, "RAS records:%d exceed threshold:%d",
> > >    				control->ras_num_recs, ras->bad_page_cnt_threshold);
> > > +			if (amdgpu_bad_page_threshold == -2) {
> > > +				dev_warn(adev->dev, "GPU will be initialized due to
> > bad_page_threshold = -2.");
> > > +				dev_warn(adev->dev, "Page retirement will not work for
> > this GPU in this state.");
> >
> > Now, this looks as good as booting with 0 = disable bad page retirement.
> > I thought page retirement will work as long as EEPROM has space, but it
> > won't bother about the threshold. If the intent is to ignore bad page
> > retirement, then 0 is good enough and -2 is not required.
> >
> > Also, when user passes threshold=-2, what is the threshold being
> > compared against to say *exceed_err_limit = true?
>
> My thought on having the -2 option is that we'll still enable page retirement, we just won't shut the GPU down when it hits the threshold. The bad pages will still be retired as we hit them, it will just never disable the GPU. The comment about retirement not working is definitely incorrect now (leftover from previous local patches), so I'll remove that. In this case, I don't think we'd ever exceed the error limit. exceed_err_limit is only really used when we are disabling the GPU, so we wouldn't want to set that to true. Otherwise we wouldn't be loading the bad pages in gpu_recovery_init, and we'll still return 0 from gpu_recovery_init.

Probably, this is too late as this patch is superseded. Replying to this 
to keep the context.

I realized that the question went in the wrong direction because of 
*exceed_err_limit = true. Saw it in the earlier line but didn't realize 
that it was removed.

Anyway, main question I wanted to ask was -
	
	"When user passes threshold=-2, what is the threshold being compared 
against?"

Looked it up today and saw this piece of code.
	        if (amdgpu_bad_page_threshold < 0) {
                u64 val = adev->gmc.mc_vram_size;

                do_div(val, RAS_BAD_PAGE_COVER);
                con->bad_page_cnt_threshold = min(lower_32_bits(val),
                                                  max_count);


For a user who has been specifying say bad_page_threshold=100 in the 
command line and unable to boot, now this is doing something else.

The comparison in next boot is done against a different threshold. In 
fact, it won't even come to this comparison logic in the next boot as 
the threshold is raised by default when this parameter is specified.

The only case this parameter takes effect is when a user is unable to 
boot because bad page retirements have exceeded the "default threshold". 
In other cases it just resets the comparison limit. Hope that is the 
intention.

Thanks,
Lijo


>   Kent
> > Thanks,
> > Lijo
> >
> > > +				res = 0;
> > > +			} else {
> > > +				*exceed_err_limit = true;
> > > +				dev_err(adev->dev, "GPU will not be initialized. Replace this
> > GPU or increase the threshold.");
> > > +			}
> > >    		}
> > >    	} else {
> > >    		DRM_INFO("Creating a new EEPROM table");