<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2021-10-20 12:35, Kent Russell wrote: </div> <blockquote type="cite" cite="mid:20211020163520.1167214-1-kent.russell@amd.com"> <pre class="moz-quote-pre" wrap="">Currently dmesg doesn't warn when the number of bad pages approaches the</pre> </blockquote> "Currently" is redundant in this sentence as it is already in present simple tense. <blockquote type="cite" cite="mid:20211020163520.1167214-1-kent.russell@amd.com"> <pre class="moz-quote-pre" wrap=""> threshold for page retirement. WARN when the number of bad pages is at 90% or greater for easier checks and planning, instead of waiting until the GPU is full of bad pages</pre> </blockquote> Missing full-stop (period) above. <blockquote type="cite" cite="mid:20211020163520.1167214-1-kent.russell@amd.com"> <pre class="moz-quote-pre" wrap=""> Cc: Luben Tuikov <a class="moz-txt-link-rfc2396E" href="mailto:luben.tuikov@amd.com"><luben.tuikov@amd.com></a> Cc: Mukul Joshi <a class="moz-txt-link-rfc2396E" href="mailto:Mukul.Joshi@amd.com"><Mukul.Joshi@amd.com></a> Signed-off-by: Kent Russell <a class="moz-txt-link-rfc2396E" href="mailto:kent.russell@amd.com"><kent.russell@amd.com></a> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index f4c05ff4b26c..1ede0f0d6f55 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -1071,12 +1071,29 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control, control->ras_fri = RAS_OFFSET_TO_INDEX(control, hdr->first_rec_offset); if (hdr->header == RAS_TABLE_HDR_VAL) { + int threshold = 0; DRM_DEBUG_DRIVER("Found existing EEPROM table with %d records", control->ras_num_recs); res = __verify_ras_table_checksum(control); if (res) DRM_ERROR("RAS table incorrect checksum or error:%d\n", res); + + /* threshold = 0 means that page retirement is disabled, while + * threshold = -1 means default behaviour + */ + if (amdgpu_bad_page_threshold == -1) + threshold = ras->bad_page_cnt_threshold; + else if (amdgpu_bad_page_threshold > 0) + threshold = amdgpu_bad_page_threshold;</pre> </blockquote> I believe we don't need this calculation here as it's already been done for us in amdgpu_ras_validate_threshold(), in amdgpu_ras.c. I believe you want to use "ras->bad_page_cnt_threshold" here instead. For instance of this, see a bit further down in this very function this check including the comment, in italics: } else if (hdr->header == RAS_TABLE_HDR_BAD && amdgpu_bad_page_threshold != 0) { res = __verify_ras_table_checksum(control); if (res) DRM_ERROR("RAS Table incorrect checksum or error:%d\n", res); if (ras->bad_page_cnt_threshold > control->ras_num_recs) { /* This means that, the threshold was increased since * the last time the system was booted, and now, * ras->bad_page_cnt_threshold - control->num_recs > 0, * so that at least one more record can be saved, * before the page count threshold is reached. */ And on the "else", a bit further down, again in italics: } else { *exceed_err_limit = true; dev_err(adev->dev, "RAS records:%d exceed threshold:%d, " "maybe retire this GPU?", control->ras_num_recs, ras->bad_page_cnt_threshold); } See how it says "records exceed threshold"--well, with this patch you want to say "records exceed 90% of threshold." :-) So these are the quantities we gauge each other to. Clarification on this below. <blockquote type="cite" cite="mid:20211020163520.1167214-1-kent.russell@amd.com"> <pre class="moz-quote-pre" wrap=""> + + /* Since multiplcation is transitive, a = 9b/10 is the same + * as 10a = 9b. Use this for our 90% limit to avoid rounding + */ </pre> </blockquote> I really like the format of the comment. But I feel that the comment itself isn't necessary... at least the way it is written ("9b" may mean "9 bits" or "9 binary". I'd avoid getting into arithmetic theory, and remove the comment completely. Anything else (explaining the math) really distracts from the real purpose of what we're doing. (After all, this is C, not a class on arithmetic--they who can, will figure it out.) Perhaps something like: /* Warn if we get past 90% of the threshold. */ <blockquote type="cite" cite="mid:20211020163520.1167214-1-kent.russell@amd.com"> <pre class="moz-quote-pre" wrap="">+ if (threshold > 0 && ((control->ras_num_recs * 10) >= (threshold * 9)))</pre> </blockquote> Right, so here perhaps we want to do this instead: diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index 98732518543e53..2aab62fa488eba 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -1077,6 +1077,12 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control, if (res) DRM_ERROR("RAS table incorrect checksum or error:%d\n", res); + /* Warn if we get past 90% of the threshold. + */ + if (10 * control->ras_num_recs >= 9 * ras->bad_page_cnt_threshold) + DRM_WARN("RAS records:%u exceeds 90%% of threshold:%d", + control->ras_num_recs, + ras->bad_page_cnt_threshold); } else if (hdr->header == RAS_TABLE_HDR_BAD && amdgpu_bad_page_threshold != 0) { res = __verify_ras_table_checksum(control); Also note that up until this point of the boot process, we don't qualify the boot by amdgpu_bad_page_threshold and I feel that this check in this embedded if-conditional shouldn't do that as well. Regards, Luben <blockquote type="cite" cite="mid:20211020163520.1167214-1-kent.russell@amd.com"> <pre class="moz-quote-pre" wrap=""> + DRM_WARN("RAS records:%u exceeds 90%% of threshold:%d", + control->ras_num_recs, + threshold); } else if (hdr->header == RAS_TABLE_HDR_BAD && amdgpu_bad_page_threshold != 0) { res = __verify_ras_table_checksum(control); </pre> </blockquote> </body> </html>