| Message ID | 1523410059-18415-1-git-send-email-yc_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-04-11 at 09:27 +0800, Y.C. Chen wrote: > From: "Y.C. Chen" <yc_chen@aspeedtech.com> > > There is another thread still access standard VGA I/O while loading drm driver. > Disable standard VGA I/O decode to avoid this issue. Jeremy, Joel, can we regression test that on our systems ? > Signed-off-by: Y.C. Chen <yc_chen@aspeedtech.com> > --- > drivers/gpu/drm/ast/ast_main.c | 5 ++++- > drivers/gpu/drm/ast/ast_mode.c | 2 +- > drivers/gpu/drm/ast/ast_post.c | 2 +- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c > index dac3558..82a2687 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -131,8 +131,8 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post) > > > /* Enable extended register access */ > - ast_enable_mmio(dev); > ast_open_key(ast); > + ast_enable_mmio(dev); > > /* Find out whether P2A works or whether to use device-tree */ > ast_detect_config_mode(dev, &scu_rev); > @@ -576,6 +576,9 @@ void ast_driver_unload(struct drm_device *dev) > { > struct ast_private *ast = dev->dev_private; > > + /* enable standard VGA decode */ > + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); > + > ast_release_firmware(dev); > kfree(ast->dp501_fw_addr); > ast_mode_fini(dev); > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index 831b733..9d637b4 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -599,7 +599,7 @@ static int ast_crtc_mode_set(struct drm_crtc *crtc, > return -EINVAL; > ast_open_key(ast); > > - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa1, 0xff, 0x04); > + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06); > > ast_set_std_reg(crtc, adjusted_mode, &vbios_mode); > ast_set_crtc_reg(crtc, adjusted_mode, &vbios_mode); > diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c > index f7d4213..c1d1ac5 100644 > --- a/drivers/gpu/drm/ast/ast_post.c > +++ b/drivers/gpu/drm/ast/ast_post.c > @@ -46,7 +46,7 @@ void ast_enable_mmio(struct drm_device *dev) > { > struct ast_private *ast = dev->dev_private; > > - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa1, 0xff, 0x04); > + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06); > } > >
On Wed, 2018-04-11 at 09:27 +0800, Y.C. Chen wrote: > index dac3558..82a2687 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -131,8 +131,8 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post) > > > /* Enable extended register access */ > - ast_enable_mmio(dev); > ast_open_key(ast); > + ast_enable_mmio(dev); Why that change ? The documentation doesn't really specify what the "password" register is about. What does it "open" ? I'm being a bit paranoid because we had issues with earlier drivers (before some of the latest changes to that code) where occasionally on boot, the chip wouldn't respond to an MMIO to one of the VGA registers, causing an EEH error which crashes on boot. So I want to make sure I understand what the changes precisely do. Cheers, Ben.
Hi Ben, Thanks for your reply. You need to open password register before modify extended CRT registers. Extended CRT registers cannot be modified if the read back value of password register is 0. Allow me to describe this issue more. This issue only happen on Intel x86/x86_64 system actually. We found there is a another thread still access VGA cursor registers when loading "ast" drm driver. VGA register is index I/O register, if two threads access it alternately, it may cause the un-expected settings. "ast" drm driver is using related IO or MMIO to access register, and the thread is using standard VGA IO(0x3d4) to access cursor register, so our solution is disabling standard VGA decoding(Set VGACRA1 D[1] to 1) at the entry of "ast" driver load to avoid this issue. Regards, Y.C. Chen -----Original Message----- From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] Sent: Wednesday, April 11, 2018 9:54 AM To: YC Chen <yc_chen@aspeedtech.com>; dri-devel@lists.freedesktop.org Cc: airlied@redhat.com; eich@suse.com Subject: Re: [PATCH] drm/ast: Fixed reboot test may cause system hanged On Wed, 2018-04-11 at 09:27 +0800, Y.C. Chen wrote: > index dac3558..82a2687 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -131,8 +131,8 @@ static int ast_detect_chip(struct drm_device *dev, > bool *need_post) > > > /* Enable extended register access */ > - ast_enable_mmio(dev); > ast_open_key(ast); > + ast_enable_mmio(dev); Why that change ? The documentation doesn't really specify what the "password" register is about. What does it "open" ? I'm being a bit paranoid because we had issues with earlier drivers (before some of the latest changes to that code) where occasionally on boot, the chip wouldn't respond to an MMIO to one of the VGA registers, causing an EEH error which crashes on boot. So I want to make sure I understand what the changes precisely do. Cheers, Ben.
On Wed, 2018-04-11 at 02:16 +0000, YC Chen wrote: > Hi Ben, > Thanks for your reply. You need to open password register before > modify extended CRT registers. Extended CRT registers cannot be > modified if the read back value of password register is 0. > Allow me to describe this issue more. This issue only happen on Intel > x86/x86_64 system actually. We found there is a another thread still > access VGA cursor registers when loading "ast" drm driver. VGA > register is index I/O register, if two threads access it alternately, > it may cause the un-expected settings. "ast" drm driver is using > related IO or MMIO to access register, and the thread is using > standard VGA IO(0x3d4) to access cursor register, so our solution is > disabling standard VGA decoding(Set VGACRA1 D[1] to 1) at the entry > of "ast" driver load to avoid this issue. Thanks. Pending testing on our systems to make sure it doesn't break anything (it shouldn't .. hopefully) Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Regards, > > Y.C. Chen > > -----Original Message----- > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > Sent: Wednesday, April 11, 2018 9:54 AM > To: YC Chen <yc_chen@aspeedtech.com>; dri-devel@lists.freedesktop.org > Cc: airlied@redhat.com; eich@suse.com > Subject: Re: [PATCH] drm/ast: Fixed reboot test may cause system > hanged > > On Wed, 2018-04-11 at 09:27 +0800, Y.C. Chen wrote: > > index dac3558..82a2687 100644 > > --- a/drivers/gpu/drm/ast/ast_main.c > > +++ b/drivers/gpu/drm/ast/ast_main.c > > @@ -131,8 +131,8 @@ static int ast_detect_chip(struct drm_device > > *dev, > > bool *need_post) > > > > > > /* Enable extended register access */ > > - ast_enable_mmio(dev); > > ast_open_key(ast); > > + ast_enable_mmio(dev); > > Why that change ? The documentation doesn't really specify what the > "password" register is about. What does it "open" ? > > I'm being a bit paranoid because we had issues with earlier drivers > (before some of the latest changes to that code) where occasionally > on boot, the chip wouldn't respond to an MMIO to one of the VGA > registers, causing an EEH error which crashes on boot. > > So I want to make sure I understand what the changes precisely do. > > Cheers, > Ben.
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index dac3558..82a2687 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -131,8 +131,8 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post) /* Enable extended register access */ - ast_enable_mmio(dev); ast_open_key(ast); + ast_enable_mmio(dev); /* Find out whether P2A works or whether to use device-tree */ ast_detect_config_mode(dev, &scu_rev); @@ -576,6 +576,9 @@ void ast_driver_unload(struct drm_device *dev) { struct ast_private *ast = dev->dev_private; + /* enable standard VGA decode */ + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); + ast_release_firmware(dev); kfree(ast->dp501_fw_addr); ast_mode_fini(dev); diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 831b733..9d637b4 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -599,7 +599,7 @@ static int ast_crtc_mode_set(struct drm_crtc *crtc, return -EINVAL; ast_open_key(ast); - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa1, 0xff, 0x04); + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06); ast_set_std_reg(crtc, adjusted_mode, &vbios_mode); ast_set_crtc_reg(crtc, adjusted_mode, &vbios_mode); diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c index f7d4213..c1d1ac5 100644 --- a/drivers/gpu/drm/ast/ast_post.c +++ b/drivers/gpu/drm/ast/ast_post.c @@ -46,7 +46,7 @@ void ast_enable_mmio(struct drm_device *dev) { struct ast_private *ast = dev->dev_private; - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa1, 0xff, 0x04); + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06); }