* [PATCH] drm/ast: Atomic CR/SR reg R/W @ 2021-09-17 7:22 KuoHsiang Chou 2021-09-20 8:17 ` Thomas Zimmermann 0 siblings, 1 reply; 9+ messages in thread From: KuoHsiang Chou @ 2021-09-17 7:22 UTC (permalink / raw) To: tzimmermann, dri-devel, linux-kernel Cc: airlied, airlied, daniel, jenmin_yuan, kuohsiang_chou, arc_sung 1. Avoid IO-index racing 2. IO-index racing happened on resolustion switching and mouse moving at the same time 3. System hung while IO-index racing occurred. Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> --- drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, uint32_t base, uint8_t index, uint8_t mask, uint8_t val) { - u8 tmp; - ast_io_write8(ast, base, index); - tmp = (ast_io_read8(ast, base + 1) & mask) | val; - ast_set_index_reg(ast, base, index, tmp); + uint16_t volatile usData; + uint8_t volatile jData; + + do { + ast_io_write8(ast, base, index); + usData = ast_io_read16(ast, base); + } while ((uint8_t)(usData) != index); + + jData = (uint8_t)(usData >> 8); + jData &= mask; + jData |= val; + usData = ((uint16_t) jData << 8) | (uint16_t) index; + ast_io_write16(ast, base, usData); } uint8_t ast_get_index_reg(struct ast_private *ast, uint32_t base, uint8_t index) { - uint8_t ret; - ast_io_write8(ast, base, index); - ret = ast_io_read8(ast, base + 1); - return ret; + uint16_t volatile usData; + uint8_t volatile jData; + + do { + ast_io_write8(ast, base, index); + usData = ast_io_read16(ast, base); + } while ((uint8_t)(usData) != index); + + jData = (uint8_t)(usData >> 8); + + return jData; } uint8_t ast_get_index_reg_mask(struct ast_private *ast, uint32_t base, uint8_t index, uint8_t mask) { - uint8_t ret; - ast_io_write8(ast, base, index); - ret = ast_io_read8(ast, base + 1) & mask; - return ret; + uint16_t volatile usData; + uint8_t volatile jData; + + do { + ast_io_write8(ast, base, index); + usData = ast_io_read16(ast, base); + } while ((uint8_t)(usData) != index); + + jData = (uint8_t)(usData >> 8); + jData &= mask; + + return jData; } static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev) -- 2.18.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/ast: Atomic CR/SR reg R/W 2021-09-17 7:22 [PATCH] drm/ast: Atomic CR/SR reg R/W KuoHsiang Chou @ 2021-09-20 8:17 ` Thomas Zimmermann 2021-09-30 7:19 ` Kuo-Hsiang Chou 0 siblings, 1 reply; 9+ messages in thread From: Thomas Zimmermann @ 2021-09-20 8:17 UTC (permalink / raw) To: KuoHsiang Chou, dri-devel, linux-kernel Cc: airlied, airlied, daniel, jenmin_yuan, arc_sung [-- Attachment #1.1: Type: text/plain, Size: 2953 bytes --] Hi Am 17.09.21 um 09:22 schrieb KuoHsiang Chou: > 1. Avoid IO-index racing > 2. IO-index racing happened on resolustion switching > and mouse moving at the same time > 3. System hung while IO-index racing occurred. I'd say that there's something else going one here. Mode setting and cursor movement should be protected against each other by DRM locking. Changing these low-level functions would not solve the issues. I'll try to reproduce the problem ASAP. Best regards Thomas > > Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> > --- > drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c > index 79a361867..1d8fa70c5 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, > uint8_t mask, uint8_t val) > { > - u8 tmp; > - ast_io_write8(ast, base, index); > - tmp = (ast_io_read8(ast, base + 1) & mask) | val; > - ast_set_index_reg(ast, base, index, tmp); > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + jData |= val; > + usData = ((uint16_t) jData << 8) | (uint16_t) index; > + ast_io_write16(ast, base, usData); > } > > uint8_t ast_get_index_reg(struct ast_private *ast, > uint32_t base, uint8_t index) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1); > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + > + return jData; > } > > uint8_t ast_get_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, uint8_t mask) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1) & mask; > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + > + return jData; > } > > static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev) > -- > 2.18.4 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/ast: Atomic CR/SR reg R/W 2021-09-20 8:17 ` Thomas Zimmermann @ 2021-09-30 7:19 ` Kuo-Hsiang Chou 2021-12-03 1:23 ` Kuo-Hsiang Chou 0 siblings, 1 reply; 9+ messages in thread From: Kuo-Hsiang Chou @ 2021-09-30 7:19 UTC (permalink / raw) To: Thomas Zimmermann, dri-devel, linux-kernel Cc: airlied, airlied, daniel, Jenmin Yuan, Arc Sung, Luke Chen, 清水修(o-shimizu)-台灣NEC -----Original Message----- From: Thomas Zimmermann [mailto:tzimmermann@suse.de] Sent: Monday, September 20, 2021 4:17 PM To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi Am 17.09.21 um 09:22 schrieb KuoHsiang Chou: > 1. Avoid IO-index racing > 2. IO-index racing happened on resolustion switching > and mouse moving at the same time > 3. System hung while IO-index racing occurred. I'd say that there's something else going one here. Mode setting and cursor movement should be protected against each other by DRM locking. Changing these low-level functions would not solve the issues. I'll try to reproduce the problem ASAP. Hi Thomas, Sorry to interrupt you again! May I understand the review's situation? Thanks! Best Regards, Kuo-Hsiang Chou Best regards Thomas > > Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> > --- > drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_main.c > b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, > uint8_t mask, uint8_t val) > { > - u8 tmp; > - ast_io_write8(ast, base, index); > - tmp = (ast_io_read8(ast, base + 1) & mask) | val; > - ast_set_index_reg(ast, base, index, tmp); > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + jData |= val; > + usData = ((uint16_t) jData << 8) | (uint16_t) index; > + ast_io_write16(ast, base, usData); > } > > uint8_t ast_get_index_reg(struct ast_private *ast, > uint32_t base, uint8_t index) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1); > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + > + return jData; > } > > uint8_t ast_get_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, uint8_t mask) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1) & mask; > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + > + return jData; > } > > static void ast_detect_config_mode(struct drm_device *dev, u32 > *scu_rev) > -- > 2.18.4 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/ast: Atomic CR/SR reg R/W 2021-09-30 7:19 ` Kuo-Hsiang Chou @ 2021-12-03 1:23 ` Kuo-Hsiang Chou 0 siblings, 0 replies; 9+ messages in thread From: Kuo-Hsiang Chou @ 2021-12-03 1:23 UTC (permalink / raw) To: Thomas Zimmermann, dri-devel, linux-kernel Cc: Ryan Chen, airlied, 清水修(o-shimizu)-台灣NEC, Jenmin Yuan, airlied, Arc Sung, Luke Chen -----Original Message----- From: Kuo-Hsiang Chou Sent: Thursday, September 30, 2021 3:19 PM To: Thomas Zimmermann <tzimmermann@suse.de>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: RE: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi -----Original Message----- From: Thomas Zimmermann [mailto:tzimmermann@suse.de] Sent: Monday, September 20, 2021 4:17 PM To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi Am 17.09.21 um 09:22 schrieb KuoHsiang Chou: > 1. Avoid IO-index racing > 2. IO-index racing happened on resolustion switching > and mouse moving at the same time > 3. System hung while IO-index racing occurred. I'd say that there's something else going one here. Mode setting and cursor movement should be protected against each other by DRM locking. Changing these low-level functions would not solve the issues. I'll try to reproduce the problem ASAP. Hi Thomas, Sorry to interrupt you again! May I understand the review's situation? Thanks! Hi Tomas, Good day! May I understand the review status, or is there anything I can do to improve it? Thanks! Best Regards, Kuo-Hsiang Chou Best Regards, Kuo-Hsiang Chou Best regards Thomas > > Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> > --- > drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_main.c > b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, > uint8_t mask, uint8_t val) > { > - u8 tmp; > - ast_io_write8(ast, base, index); > - tmp = (ast_io_read8(ast, base + 1) & mask) | val; > - ast_set_index_reg(ast, base, index, tmp); > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + jData |= val; > + usData = ((uint16_t) jData << 8) | (uint16_t) index; > + ast_io_write16(ast, base, usData); > } > > uint8_t ast_get_index_reg(struct ast_private *ast, > uint32_t base, uint8_t index) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1); > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + > + return jData; > } > > uint8_t ast_get_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, uint8_t mask) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1) & mask; > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + > + return jData; > } > > static void ast_detect_config_mode(struct drm_device *dev, u32 > *scu_rev) > -- > 2.18.4 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/ast: Atomic CR/SR reg R/W @ 2021-12-03 1:23 ` Kuo-Hsiang Chou 0 siblings, 0 replies; 9+ messages in thread From: Kuo-Hsiang Chou @ 2021-12-03 1:23 UTC (permalink / raw) To: Thomas Zimmermann, dri-devel, linux-kernel Cc: airlied, airlied, daniel, Jenmin Yuan, Arc Sung, Luke Chen, 清水修(o-shimizu)-台灣NEC, Ryan Chen -----Original Message----- From: Kuo-Hsiang Chou Sent: Thursday, September 30, 2021 3:19 PM To: Thomas Zimmermann <tzimmermann@suse.de>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: RE: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi -----Original Message----- From: Thomas Zimmermann [mailto:tzimmermann@suse.de] Sent: Monday, September 20, 2021 4:17 PM To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi Am 17.09.21 um 09:22 schrieb KuoHsiang Chou: > 1. Avoid IO-index racing > 2. IO-index racing happened on resolustion switching > and mouse moving at the same time > 3. System hung while IO-index racing occurred. I'd say that there's something else going one here. Mode setting and cursor movement should be protected against each other by DRM locking. Changing these low-level functions would not solve the issues. I'll try to reproduce the problem ASAP. Hi Thomas, Sorry to interrupt you again! May I understand the review's situation? Thanks! Hi Tomas, Good day! May I understand the review status, or is there anything I can do to improve it? Thanks! Best Regards, Kuo-Hsiang Chou Best Regards, Kuo-Hsiang Chou Best regards Thomas > > Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> > --- > drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_main.c > b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, > uint8_t mask, uint8_t val) > { > - u8 tmp; > - ast_io_write8(ast, base, index); > - tmp = (ast_io_read8(ast, base + 1) & mask) | val; > - ast_set_index_reg(ast, base, index, tmp); > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + jData |= val; > + usData = ((uint16_t) jData << 8) | (uint16_t) index; > + ast_io_write16(ast, base, usData); > } > > uint8_t ast_get_index_reg(struct ast_private *ast, > uint32_t base, uint8_t index) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1); > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + > + return jData; > } > > uint8_t ast_get_index_reg_mask(struct ast_private *ast, > uint32_t base, uint8_t index, uint8_t mask) > { > - uint8_t ret; > - ast_io_write8(ast, base, index); > - ret = ast_io_read8(ast, base + 1) & mask; > - return ret; > + uint16_t volatile usData; > + uint8_t volatile jData; > + > + do { > + ast_io_write8(ast, base, index); > + usData = ast_io_read16(ast, base); > + } while ((uint8_t)(usData) != index); > + > + jData = (uint8_t)(usData >> 8); > + jData &= mask; > + > + return jData; > } > > static void ast_detect_config_mode(struct drm_device *dev, u32 > *scu_rev) > -- > 2.18.4 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/ast: Atomic CR/SR reg R/W 2021-12-03 1:23 ` Kuo-Hsiang Chou (?) @ 2021-12-03 8:46 ` Thomas Zimmermann 2021-12-06 1:38 ` Kuo-Hsiang Chou -1 siblings, 1 reply; 9+ messages in thread From: Thomas Zimmermann @ 2021-12-03 8:46 UTC (permalink / raw) To: Kuo-Hsiang Chou, dri-devel, linux-kernel Cc: Ryan Chen, airlied, 清水修(o-shimizu)-台灣NEC, Jenmin Yuan, airlied, Arc Sung, Luke Chen [-- Attachment #1.1: Type: text/plain, Size: 4748 bytes --] Hi Am 03.12.21 um 02:23 schrieb Kuo-Hsiang Chou: > > > -----Original Message----- > From: Kuo-Hsiang Chou > Sent: Thursday, September 30, 2021 3:19 PM > To: Thomas Zimmermann <tzimmermann@suse.de>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org > Subject: RE: [PATCH] drm/ast: Atomic CR/SR reg R/W > > Hi > > -----Original Message----- > From: Thomas Zimmermann [mailto:tzimmermann@suse.de] > Sent: Monday, September 20, 2021 4:17 PM > To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W > > Hi > > Am 17.09.21 um 09:22 schrieb KuoHsiang Chou: >> 1. Avoid IO-index racing >> 2. IO-index racing happened on resolustion switching >> and mouse moving at the same time >> 3. System hung while IO-index racing occurred. > > I'd say that there's something else going one here. Mode setting and cursor movement should be protected against each other by DRM locking. > Changing these low-level functions would not solve the issues. I'll try to reproduce the problem ASAP. > > Hi Thomas, > > Sorry to interrupt you again! > May I understand the review's situation? Thanks! Look, you really have to work with us during the review process. Don't just expect us to tell you what to do. I'm not going to merge this patch. As I said, I don't think it fixes the problem. Mouse movement and resolution switching should not interfere with each other. The DRM framework should guarantee that. I cannot reproduce the issue, but there's most likely something else happening here. How can the system switch resolution and change the mouse at the same time? Best regards Thomas > > Hi Tomas, > Good day! > May I understand the review status, or is there anything I can do to improve it? Thanks! > > Best Regards, > Kuo-Hsiang Chou > > Best Regards, > Kuo-Hsiang Chou > > Best regards > Thomas > >> >> Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> >> --- >> drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_main.c >> b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 >> --- a/drivers/gpu/drm/ast/ast_main.c >> +++ b/drivers/gpu/drm/ast/ast_main.c >> @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, >> uint32_t base, uint8_t index, >> uint8_t mask, uint8_t val) >> { >> - u8 tmp; >> - ast_io_write8(ast, base, index); >> - tmp = (ast_io_read8(ast, base + 1) & mask) | val; >> - ast_set_index_reg(ast, base, index, tmp); >> + uint16_t volatile usData; >> + uint8_t volatile jData; >> + >> + do { >> + ast_io_write8(ast, base, index); >> + usData = ast_io_read16(ast, base); >> + } while ((uint8_t)(usData) != index); >> + >> + jData = (uint8_t)(usData >> 8); >> + jData &= mask; >> + jData |= val; >> + usData = ((uint16_t) jData << 8) | (uint16_t) index; >> + ast_io_write16(ast, base, usData); >> } >> >> uint8_t ast_get_index_reg(struct ast_private *ast, >> uint32_t base, uint8_t index) >> { >> - uint8_t ret; >> - ast_io_write8(ast, base, index); >> - ret = ast_io_read8(ast, base + 1); >> - return ret; >> + uint16_t volatile usData; >> + uint8_t volatile jData; >> + >> + do { >> + ast_io_write8(ast, base, index); >> + usData = ast_io_read16(ast, base); >> + } while ((uint8_t)(usData) != index); >> + >> + jData = (uint8_t)(usData >> 8); >> + >> + return jData; >> } >> >> uint8_t ast_get_index_reg_mask(struct ast_private *ast, >> uint32_t base, uint8_t index, uint8_t mask) >> { >> - uint8_t ret; >> - ast_io_write8(ast, base, index); >> - ret = ast_io_read8(ast, base + 1) & mask; >> - return ret; >> + uint16_t volatile usData; >> + uint8_t volatile jData; >> + >> + do { >> + ast_io_write8(ast, base, index); >> + usData = ast_io_read16(ast, base); >> + } while ((uint8_t)(usData) != index); >> + >> + jData = (uint8_t)(usData >> 8); >> + jData &= mask; >> + >> + return jData; >> } >> >> static void ast_detect_config_mode(struct drm_device *dev, u32 >> *scu_rev) >> -- >> 2.18.4 >> > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/ast: Atomic CR/SR reg R/W 2021-12-03 8:46 ` Thomas Zimmermann @ 2021-12-06 1:38 ` Kuo-Hsiang Chou 2022-05-02 14:32 ` Thomas Zimmermann 0 siblings, 1 reply; 9+ messages in thread From: Kuo-Hsiang Chou @ 2021-12-06 1:38 UTC (permalink / raw) To: Thomas Zimmermann, dri-devel, linux-kernel Cc: Ryan Chen, airlied, 清水修(o-shimizu)-台灣NEC, Jenmin Yuan, airlied, Arc Sung, Luke Chen Hi -----Original Message----- From: Thomas Zimmermann [mailto:tzimmermann@suse.de] Sent: Friday, December 03, 2021 4:47 PM To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi Am 03.12.21 um 02:23 schrieb Kuo-Hsiang Chou: > > > -----Original Message----- > From: Kuo-Hsiang Chou > Sent: Thursday, September 30, 2021 3:19 PM > To: Thomas Zimmermann <tzimmermann@suse.de>; > dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org > Subject: RE: [PATCH] drm/ast: Atomic CR/SR reg R/W > > Hi > > -----Original Message----- > From: Thomas Zimmermann [mailto:tzimmermann@suse.de] > Sent: Monday, September 20, 2021 4:17 PM > To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; > dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W > > Hi > > Am 17.09.21 um 09:22 schrieb KuoHsiang Chou: >> 1. Avoid IO-index racing >> 2. IO-index racing happened on resolustion switching >> and mouse moving at the same time 3. System hung while IO-index >> racing occurred. > > I'd say that there's something else going one here. Mode setting and cursor movement should be protected against each other by DRM locking. > Changing these low-level functions would not solve the issues. I'll try to reproduce the problem ASAP. > > Hi Thomas, > > Sorry to interrupt you again! > May I understand the review's situation? Thanks! Hi Thomas, Look, you really have to work with us during the review process. Don't just expect us to tell you what to do. Thanks! Got it. I'm not going to merge this patch. As I said, I don't think it fixes the problem. Mouse movement and resolution switching should not interfere with each other. The DRM framework should guarantee that. OK, thanks for your confirmation. I cannot reproduce the issue, but there's most likely something else happening here. How can the system switch resolution and change the mouse at the same time? Sure, we will check if there is a 100 percent method to reproduce the issue. Thanks for your responses. Regards, Kuo-Hsiang Chou Best regards Thomas > > Hi Tomas, > Good day! > May I understand the review status, or is there anything I can do to improve it? Thanks! > > Best Regards, > Kuo-Hsiang Chou > > Best Regards, > Kuo-Hsiang Chou > > Best regards > Thomas > >> >> Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> >> --- >> drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_main.c >> b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 >> --- a/drivers/gpu/drm/ast/ast_main.c >> +++ b/drivers/gpu/drm/ast/ast_main.c >> @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, >> uint32_t base, uint8_t index, >> uint8_t mask, uint8_t val) >> { >> - u8 tmp; >> - ast_io_write8(ast, base, index); >> - tmp = (ast_io_read8(ast, base + 1) & mask) | val; >> - ast_set_index_reg(ast, base, index, tmp); >> + uint16_t volatile usData; >> + uint8_t volatile jData; >> + >> + do { >> + ast_io_write8(ast, base, index); >> + usData = ast_io_read16(ast, base); >> + } while ((uint8_t)(usData) != index); >> + >> + jData = (uint8_t)(usData >> 8); >> + jData &= mask; >> + jData |= val; >> + usData = ((uint16_t) jData << 8) | (uint16_t) index; >> + ast_io_write16(ast, base, usData); >> } >> >> uint8_t ast_get_index_reg(struct ast_private *ast, >> uint32_t base, uint8_t index) >> { >> - uint8_t ret; >> - ast_io_write8(ast, base, index); >> - ret = ast_io_read8(ast, base + 1); >> - return ret; >> + uint16_t volatile usData; >> + uint8_t volatile jData; >> + >> + do { >> + ast_io_write8(ast, base, index); >> + usData = ast_io_read16(ast, base); >> + } while ((uint8_t)(usData) != index); >> + >> + jData = (uint8_t)(usData >> 8); >> + >> + return jData; >> } >> >> uint8_t ast_get_index_reg_mask(struct ast_private *ast, >> uint32_t base, uint8_t index, uint8_t mask) >> { >> - uint8_t ret; >> - ast_io_write8(ast, base, index); >> - ret = ast_io_read8(ast, base + 1) & mask; >> - return ret; >> + uint16_t volatile usData; >> + uint8_t volatile jData; >> + >> + do { >> + ast_io_write8(ast, base, index); >> + usData = ast_io_read16(ast, base); >> + } while ((uint8_t)(usData) != index); >> + >> + jData = (uint8_t)(usData >> 8); >> + jData &= mask; >> + >> + return jData; >> } >> >> static void ast_detect_config_mode(struct drm_device *dev, u32 >> *scu_rev) >> -- >> 2.18.4 >> > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/ast: Atomic CR/SR reg R/W 2021-12-06 1:38 ` Kuo-Hsiang Chou @ 2022-05-02 14:32 ` Thomas Zimmermann 2022-05-03 0:38 ` Kuo-Hsiang Chou 0 siblings, 1 reply; 9+ messages in thread From: Thomas Zimmermann @ 2022-05-02 14:32 UTC (permalink / raw) To: Kuo-Hsiang Chou, dri-devel, linux-kernel Cc: Ryan Chen, airlied, 清水修(o-shimizu)-台灣NEC, Jenmin Yuan, airlied, Arc Sung, Luke Chen [-- Attachment #1.1: Type: text/plain, Size: 4439 bytes --] Hi Am 06.12.21 um 02:38 schrieb Kuo-Hsiang Chou: > I'm not going to merge this patch. As I said, I don't think it fixes the problem. Mouse movement and resolution switching should not interfere with each other. The DRM framework should guarantee that. > OK, thanks for your confirmation. > > I cannot reproduce the issue, but there's most likely something else happening here. How can the system switch resolution and change the mouse at the same time? > Sure, we will check if there is a 100 percent method to reproduce the issue. > Thanks for your responses. I've been away for a while; sorry for all this taking so long. I've meanwhile been able to reproduce the problem. It happens when GNOME concurrently tries to set the video mode and read the available video modes from EDID. Reading EDID is not protected against concurrent mode setting or cursor movement. I've posted a patchset that should fix the problem. See [1]. Best regards Thomas [1] https://lore.kernel.org/dri-devel/20220502142514.2174-1-tzimmermann@suse.de/T/#t > > Regards, > Kuo-Hsiang Chou > > Best regards > Thomas > >> >> Hi Tomas, >> Good day! >> May I understand the review status, or is there anything I can do to improve it? Thanks! >> >> Best Regards, >> Kuo-Hsiang Chou >> >> Best Regards, >> Kuo-Hsiang Chou >> >> Best regards >> Thomas >> >>> >>> Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> >>> --- >>> drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- >>> 1 file changed, 36 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ast/ast_main.c >>> b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 >>> --- a/drivers/gpu/drm/ast/ast_main.c >>> +++ b/drivers/gpu/drm/ast/ast_main.c >>> @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, >>> uint32_t base, uint8_t index, >>> uint8_t mask, uint8_t val) >>> { >>> - u8 tmp; >>> - ast_io_write8(ast, base, index); >>> - tmp = (ast_io_read8(ast, base + 1) & mask) | val; >>> - ast_set_index_reg(ast, base, index, tmp); >>> + uint16_t volatile usData; >>> + uint8_t volatile jData; >>> + >>> + do { >>> + ast_io_write8(ast, base, index); >>> + usData = ast_io_read16(ast, base); >>> + } while ((uint8_t)(usData) != index); >>> + >>> + jData = (uint8_t)(usData >> 8); >>> + jData &= mask; >>> + jData |= val; >>> + usData = ((uint16_t) jData << 8) | (uint16_t) index; >>> + ast_io_write16(ast, base, usData); >>> } >>> >>> uint8_t ast_get_index_reg(struct ast_private *ast, >>> uint32_t base, uint8_t index) >>> { >>> - uint8_t ret; >>> - ast_io_write8(ast, base, index); >>> - ret = ast_io_read8(ast, base + 1); >>> - return ret; >>> + uint16_t volatile usData; >>> + uint8_t volatile jData; >>> + >>> + do { >>> + ast_io_write8(ast, base, index); >>> + usData = ast_io_read16(ast, base); >>> + } while ((uint8_t)(usData) != index); >>> + >>> + jData = (uint8_t)(usData >> 8); >>> + >>> + return jData; >>> } >>> >>> uint8_t ast_get_index_reg_mask(struct ast_private *ast, >>> uint32_t base, uint8_t index, uint8_t mask) >>> { >>> - uint8_t ret; >>> - ast_io_write8(ast, base, index); >>> - ret = ast_io_read8(ast, base + 1) & mask; >>> - return ret; >>> + uint16_t volatile usData; >>> + uint8_t volatile jData; >>> + >>> + do { >>> + ast_io_write8(ast, base, index); >>> + usData = ast_io_read16(ast, base); >>> + } while ((uint8_t)(usData) != index); >>> + >>> + jData = (uint8_t)(usData >> 8); >>> + jData &= mask; >>> + >>> + return jData; >>> } >>> >>> static void ast_detect_config_mode(struct drm_device *dev, u32 >>> *scu_rev) >>> -- >>> 2.18.4 >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/ast: Atomic CR/SR reg R/W 2022-05-02 14:32 ` Thomas Zimmermann @ 2022-05-03 0:38 ` Kuo-Hsiang Chou 0 siblings, 0 replies; 9+ messages in thread From: Kuo-Hsiang Chou @ 2022-05-03 0:38 UTC (permalink / raw) To: Thomas Zimmermann, dri-devel, linux-kernel Cc: Ryan Chen, airlied, 清水修(o-shimizu)-台灣NEC, Jenmin Yuan, airlied, Arc Sung, Luke Chen Hi Thomas, Thanks for your kindly assistance. Regards Kuo-Hsiang Chou -----Original Message----- From: Thomas Zimmermann [mailto:tzimmermann@suse.de] Sent: Monday, May 02, 2022 10:33 PM To: Kuo-Hsiang Chou <kuohsiang_chou@aspeedtech.com>; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Cc: Ryan Chen <ryan_chen@aspeedtech.com>; airlied@linux.ie; 清水修(o-shimizu)-台灣NEC <o-shimizu@nec.com.tw>; Jenmin Yuan <jenmin_yuan@aspeedtech.com>; airlied@redhat.com; Arc Sung <arc_sung@aspeedtech.com>; Luke Chen <luke_chen@aspeedtech.com> Subject: Re: [PATCH] drm/ast: Atomic CR/SR reg R/W Hi Am 06.12.21 um 02:38 schrieb Kuo-Hsiang Chou: > I'm not going to merge this patch. As I said, I don't think it fixes the problem. Mouse movement and resolution switching should not interfere with each other. The DRM framework should guarantee that. > OK, thanks for your confirmation. > > I cannot reproduce the issue, but there's most likely something else happening here. How can the system switch resolution and change the mouse at the same time? > Sure, we will check if there is a 100 percent method to reproduce the issue. > Thanks for your responses. I've been away for a while; sorry for all this taking so long. I've meanwhile been able to reproduce the problem. It happens when GNOME concurrently tries to set the video mode and read the available video modes from EDID. Reading EDID is not protected against concurrent mode setting or cursor movement. I've posted a patchset that should fix the problem. See [1]. Best regards Thomas [1] https://lore.kernel.org/dri-devel/20220502142514.2174-1-tzimmermann@suse.de/T/#t > > Regards, > Kuo-Hsiang Chou > > Best regards > Thomas > >> >> Hi Tomas, >> Good day! >> May I understand the review status, or is there anything I can do to improve it? Thanks! >> >> Best Regards, >> Kuo-Hsiang Chou >> >> Best Regards, >> Kuo-Hsiang Chou >> >> Best regards >> Thomas >> >>> >>> Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com> >>> --- >>> drivers/gpu/drm/ast/ast_main.c | 48 +++++++++++++++++++++++++--------- >>> 1 file changed, 36 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ast/ast_main.c >>> b/drivers/gpu/drm/ast/ast_main.c index 79a361867..1d8fa70c5 100644 >>> --- a/drivers/gpu/drm/ast/ast_main.c >>> +++ b/drivers/gpu/drm/ast/ast_main.c >>> @@ -41,28 +41,52 @@ void ast_set_index_reg_mask(struct ast_private *ast, >>> uint32_t base, uint8_t index, >>> uint8_t mask, uint8_t val) >>> { >>> - u8 tmp; >>> - ast_io_write8(ast, base, index); >>> - tmp = (ast_io_read8(ast, base + 1) & mask) | val; >>> - ast_set_index_reg(ast, base, index, tmp); >>> + uint16_t volatile usData; >>> + uint8_t volatile jData; >>> + >>> + do { >>> + ast_io_write8(ast, base, index); >>> + usData = ast_io_read16(ast, base); >>> + } while ((uint8_t)(usData) != index); >>> + >>> + jData = (uint8_t)(usData >> 8); >>> + jData &= mask; >>> + jData |= val; >>> + usData = ((uint16_t) jData << 8) | (uint16_t) index; >>> + ast_io_write16(ast, base, usData); >>> } >>> >>> uint8_t ast_get_index_reg(struct ast_private *ast, >>> uint32_t base, uint8_t index) >>> { >>> - uint8_t ret; >>> - ast_io_write8(ast, base, index); >>> - ret = ast_io_read8(ast, base + 1); >>> - return ret; >>> + uint16_t volatile usData; >>> + uint8_t volatile jData; >>> + >>> + do { >>> + ast_io_write8(ast, base, index); >>> + usData = ast_io_read16(ast, base); >>> + } while ((uint8_t)(usData) != index); >>> + >>> + jData = (uint8_t)(usData >> 8); >>> + >>> + return jData; >>> } >>> >>> uint8_t ast_get_index_reg_mask(struct ast_private *ast, >>> uint32_t base, uint8_t index, uint8_t mask) >>> { >>> - uint8_t ret; >>> - ast_io_write8(ast, base, index); >>> - ret = ast_io_read8(ast, base + 1) & mask; >>> - return ret; >>> + uint16_t volatile usData; >>> + uint8_t volatile jData; >>> + >>> + do { >>> + ast_io_write8(ast, base, index); >>> + usData = ast_io_read16(ast, base); >>> + } while ((uint8_t)(usData) != index); >>> + >>> + jData = (uint8_t)(usData >> 8); >>> + jData &= mask; >>> + >>> + return jData; >>> } >>> >>> static void ast_detect_config_mode(struct drm_device *dev, u32 >>> *scu_rev) >>> -- >>> 2.18.4 >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-03 0:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-17 7:22 [PATCH] drm/ast: Atomic CR/SR reg R/W KuoHsiang Chou 2021-09-20 8:17 ` Thomas Zimmermann 2021-09-30 7:19 ` Kuo-Hsiang Chou 2021-12-03 1:23 ` Kuo-Hsiang Chou 2021-12-03 1:23 ` Kuo-Hsiang Chou 2021-12-03 8:46 ` Thomas Zimmermann 2021-12-06 1:38 ` Kuo-Hsiang Chou 2022-05-02 14:32 ` Thomas Zimmermann 2022-05-03 0:38 ` Kuo-Hsiang Chou
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.