Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703346 > unrolled thread

[PATCH v6 1/7] drm/tilcdc: ensure nonatomic iowrite64 is not used

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 1/7] drm/tilcdc: ensure nonatomic iowrite64 is not used Logan Gunthorpe <logang@deltatee.com> - 2017-08-03 20:40 +0200
    Re: [PATCH v6 1/7] drm/tilcdc: ensure nonatomic iowrite64 is not used Tomi Valkeinen <tomi.valkeinen@ti.com> - 2017-08-04 15:10 +0200
      Re: [PATCH v6 1/7] drm/tilcdc: ensure nonatomic iowrite64 is not used Logan Gunthorpe <logang@deltatee.com> - 2017-08-04 17:50 +0200
      Re: [PATCH v6 1/7] drm/tilcdc: ensure nonatomic iowrite64 is not used Jyri Sarha <jsarha@ti.com> - 2017-08-05 15:40 +0200

#1703346 — [PATCH v6 1/7] drm/tilcdc: ensure nonatomic iowrite64 is not used

Add a check to ensure iowrite64 is only used if it is atomic.

It was decided in [1] that the tilcdc driver should not be using an
atomic operation (so it was left out of this patchset). However, it turns
out that through the drm code, a nonatomic header is actually included:

include/linux/io-64-nonatomic-lo-hi.h
is included from include/drm/drm_os_linux.h:9:0,
            from include/drm/drmP.h:74,
            from include/drm/drm_modeset_helper.h:26,
            from include/drm/drm_atomic_helper.h:33,
            from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:19:

And thus, without this change, this patchset would inadvertantly
change the behaviour of the tilcdc driver.

[1] lkml.kernel.org/r/CAK8P3a2HhO_zCnsTzq7hmWSz5La5Thu19FWZpun16iMnyyNreQ@mail.gmail.com

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jyri Sarha <jsarha@ti.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
index 9d528c0a67a4..5048ebb86835 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
@@ -133,7 +133,7 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	volatile void __iomem *addr = priv->mmio + reg;
 
-#ifdef iowrite64
+#if defined(iowrite64) && !defined(iowrite64_is_nonatomic)
 	iowrite64(data, addr);
 #else
 	__iowmb();
-- 
2.11.0

[toc] | [next] | [standalone]

#1703959

[Multipart message — attachments visible in raw view] — view raw

On 03/08/17 21:30, Logan Gunthorpe wrote:
> Add a check to ensure iowrite64 is only used if it is atomic.
> 
> It was decided in [1] that the tilcdc driver should not be using an
> atomic operation (so it was left out of this patchset). However, it turns
> out that through the drm code, a nonatomic header is actually included:
> 
> include/linux/io-64-nonatomic-lo-hi.h
> is included from include/drm/drm_os_linux.h:9:0,
>             from include/drm/drmP.h:74,
>             from include/drm/drm_modeset_helper.h:26,
>             from include/drm/drm_atomic_helper.h:33,
>             from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:19:
> 
> And thus, without this change, this patchset would inadvertantly
> change the behaviour of the tilcdc driver.

I haven't really followed the discussion on this, but if the tilcdc's
use of iowrite64 causes (real) problems/complications elsewhere, I think
we could drop it.

The problem is that the HW has a race issue, and the two registers in
question should be written as close to each other as possible. We
thought a single 64bit write, writing to both registers in one go, would
improve that slightly, compared to two 32 bit writes.

Jyri, correct me if I'm wrong, but we have no proof that it actually
helps, and it might be that even if it helps, the difference is
theoretical. Probably if we ensure the irqs are off when we do two 32
bit writes, we're already close enough to the optimal case.

 Tomi

[toc] | [prev] | [next] | [standalone]

#1704086

On 04/08/17 07:03 AM, Tomi Valkeinen wrote:
> I haven't really followed the discussion on this, but if the tilcdc's
> use of iowrite64 causes (real) problems/complications elsewhere, I think
> we could drop it.

Well, that's up to you. The patch I submitted should still be correct
though, and if arm ever gets a proper atomic iowrite64 implementation it
would be good to use it. So in an annotative sense it's nice to keep the
function call in there.

Thanks,

Logan

[toc] | [prev] | [next] | [standalone]

#1704648

On 08/04/17 16:03, Tomi Valkeinen wrote:
> On 03/08/17 21:30, Logan Gunthorpe wrote:
>> Add a check to ensure iowrite64 is only used if it is atomic.
>>
>> It was decided in [1] that the tilcdc driver should not be using an
>> atomic operation (so it was left out of this patchset). However, it turns
>> out that through the drm code, a nonatomic header is actually included:
>>
>> include/linux/io-64-nonatomic-lo-hi.h
>> is included from include/drm/drm_os_linux.h:9:0,
>>             from include/drm/drmP.h:74,
>>             from include/drm/drm_modeset_helper.h:26,
>>             from include/drm/drm_atomic_helper.h:33,
>>             from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:19:
>>
>> And thus, without this change, this patchset would inadvertantly
>> change the behaviour of the tilcdc driver.
> 
> I haven't really followed the discussion on this, but if the tilcdc's
> use of iowrite64 causes (real) problems/complications elsewhere, I think
> we could drop it.
> 
> The problem is that the HW has a race issue, and the two registers in
> question should be written as close to each other as possible. We
> thought a single 64bit write, writing to both registers in one go, would
> improve that slightly, compared to two 32 bit writes.
> 
> Jyri, correct me if I'm wrong, but we have no proof that it actually
> helps, and it might be that even if it helps, the difference is
> theoretical. Probably if we ensure the irqs are off when we do two 32
> bit writes, we're already close enough to the optimal case.
> 

For the sake of this particular case you are right, the atomicity is
probably not that important here. But in general ARM7 has an atomic
64bit write and I think it is a shame if it can not be easily used in
linux kernel.

Best regards,
Jyri

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web