<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-2022-jp"> <meta name="Generator" content="Microsoft Exchange Server"> <style></style> </head> <body> <div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> Wait, this patchset intriduced a bug. will fix that later.<br> <br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> Currently, all ras ta cmds must be performed with corresponding ras objects.<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> So if vbios enable ras for us, we need set up the ras objects as soon as possible, otherwise the status does not match. This patch moves the ras_enable_all who set up the ras objects after all IP's initialization. So it is impossible to perform a ras ta disable cmd as there is no object.<br> <br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> there are 2 scenarios,<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> 1) ras is enabled by vbios. <br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> case enable:<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> enable_on_boot will bypass psp, just set up object.<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> case disable:<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> enable_on_boot need set up object first, then perform a ras ta disable cmd and update the object.<br> <br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> 2) ras is disabled when boot by default.<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> case enable:<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> enable_on_boot will make a ras ta enable cmd and set up object.<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> case disable:<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> do nothing.<br> <br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> This patch only handle #1 case enable and #2 case enable and case disable. Will fix #1 case enable later.<br> <br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> Evan, the question you raised is good.<br> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> Performing a ras ta disable cmd without a ras object makes the code logic incorrect. looks a little weird, but code is simple.</div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Quan, Evan<br> <b>Sent:</b> Monday, April 8, 2019 8:17:44 PM<br> <b>To:</b> Pan, Xinhui; amd-gfx@lists.freedesktop.org<br> <b>Cc:</b> Deucher, Alexander<br> <b>Subject:</b> RE: [PATCH 1/5] drm/amdgpu: Make default ras error type to none</font> <div> </div> </div> </div> <font size="2"><span style="font-size:11pt;"> <div class="PlainText">This series is fine. Please feel free to add my RB.<br> But it reminds me another thing: it's a little weird to enable all RAS features and disable those not needed afterwards.<br> Is it possible to only enable those which are really needed(no disablement will be needed then)?<br> <br> Regards,<br> Evan<br> > -----Original Message-----<br> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Pan,<br> > Xinhui<br> > Sent: 2019年4月8日 15:08<br> > To: amd-gfx@lists.freedesktop.org<br> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com><br> > Subject: [PATCH 1/5] drm/amdgpu: Make default ras error type to none<br> > <br> > Unless IP has implemented its own ras, use ERROR_NONE as the default type.<br> > <br> > Signed-off-by: xinhui pan <xinhui.pan@amd.com><br> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com><br> > ---<br> > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 24 +++++++++++++++------<br> > ---<br> > 1 file changed, 15 insertions(+), 9 deletions(-)<br> > <br> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c<br> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c<br> > index 2b7a420d5a1f..6cbf1d34c5b4 100644<br> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c<br> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c<br> > @@ -664,11 +664,13 @@ static int amdgpu_ras_enable_all_features(struct<br> > amdgpu_device *adev,<br> > struct amdgpu_ras *con = amdgpu_ras_get_context(adev);<br> > int ras_block_count = AMDGPU_RAS_BLOCK_COUNT;<br> > int i;<br> > + const enum amdgpu_ras_error_type default_ras_type =<br> > + AMDGPU_RAS_ERROR__NONE;<br> > <br> > for (i = 0; i < ras_block_count; i++) {<br> > struct ras_common_if head = {<br> > .block = i,<br> > - .type =<br> > AMDGPU_RAS_ERROR__MULTI_UNCORRECTABLE,<br> > + .type = default_ras_type,<br> > .sub_block_index = 0,<br> > };<br> > strcpy(head.name, ras_block_str(i));<br> > @@ -1474,9 +1476,6 @@ int amdgpu_ras_init(struct amdgpu_device *adev)<br> > <br> > amdgpu_ras_mask &= AMDGPU_RAS_BLOCK_MASK;<br> > <br> > - if (con->flags & AMDGPU_RAS_FLAG_INIT_BY_VBIOS)<br> > - amdgpu_ras_enable_all_features(adev, 1);<br> > -<br> > if (amdgpu_ras_fs_init(adev))<br> > goto fs_out;<br> > <br> > @@ -1504,18 +1503,25 @@ void amdgpu_ras_post_init(struct<br> > amdgpu_device *adev)<br> > if (!con)<br> > return;<br> > <br> > - /* We enable ras on all hw_supported block, but as boot parameter<br> > might<br> > - * disable some of them and one or more IP has not implemented<br> > yet.<br> > - * So we disable them on behalf.<br> > - */<br> > if (con->flags & AMDGPU_RAS_FLAG_INIT_BY_VBIOS) {<br> > + /* Set up all other IPs which are not implemented. There is a<br> > + * tricky thing that IP's actual ras error type should be<br> > + * MULTI_UNCORRECTABLE, but as driver does not handle it,<br> > so<br> > + * ERROR_NONE make sense anyway.<br> > + */<br> > + amdgpu_ras_enable_all_features(adev, 1);<br> > +<br> > + /* We enable ras on all hw_supported block, but as boot<br> > + * parameter might disable some of them and one or more IP<br> > has<br> > + * not implemented yet. So we disable them on behalf.<br> > + */<br> > list_for_each_entry_safe(obj, tmp, &con->head, node) {<br> > if (!amdgpu_ras_is_supported(adev, obj-<br> > >head.block)) {<br> > amdgpu_ras_feature_enable(adev, &obj-<br> > >head, 0);<br> > /* there should be no any reference. */<br> > WARN_ON(alive_obj(obj));<br> > }<br> > - };<br> > + }<br> > }<br> > }<br> > <br> > --<br> > 2.17.1<br> > <br> > _______________________________________________<br> > amd-gfx mailing list<br> > amd-gfx@lists.freedesktop.org<br> > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br> </div> </span></font> </body> </html>