Re: [PATCH] drm/vmwgfx: switch from ioremap_cache to memremap

[Dan Williams]

On Tue, Oct 13, 2015 at 11:37 AM, Thomas Hellstrom
<thellstrom@xxxxxxxxxx> wrote:
> On 10/13/2015 06:35 PM, Dan Williams wrote:
>> On Mon, Oct 12, 2015 at 10:18 PM, Thomas Hellstrom
>> <thellstrom@xxxxxxxxxx> wrote:
>>> Hi!
>>>
>>> On 10/13/2015 12:35 AM, Dan Williams wrote:
>>>> Per commit 2e586a7e017a "drm/vmwgfx: Map the fifo as cached" the driver
>>>> expects the fifo registers to be cacheable.  In preparation for
>>>> deprecating ioremap_cache() convert its usage in vmwgfx to memremap().
>>>>
>>>> Cc: David Airlie <airlied@xxxxxxxx>
>>>> Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
>>>> Cc: Sinclair Yeh <syeh@xxxxxxxxxx>
>>>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>>> While I have nothing against the conversion, what's stopping the
>>> compiler from reordering writes on a generic architecture and caching
>>> and reordering reads on x86 in particular? At the very least it looks to
>>> me like the memory accesses of the memremap'd memory needs to be
>>> encapsulated within READ_ONCE and WRITE_ONCE.
>> Hmm, currently the code is using ioread32/iowrite32 which only do
>> volatile accesses, whereas READ_ONCE / WRITE_ONCE have a memory
>> clobber on entry and exit.  So, I'm assuming all you need is the
>> guarantee of "no compiler re-ordering" and not the stronger
>> READ_ONCE/WRITE_ONCE guarantees, but that still seems broken compared
>> to explicit fencing where it matters.
>
> I'm not quite sure I follow you here, it looks to me like READ_ONCE()
> and WRITE_ONCE() are implemented as
> volatile accesses,

Ah, sorry, I was looking at the default case...

>
> http://lxr.free-electrons.com/source/include/linux/compiler.h#L215
>
> just like ioread32 and iowrite32
>
> http://lxr.free-electrons.com/source/include/asm-generic/io.h#L54
>
> which would minimize any potential impact of this change.
> IMO optimizing the memory accesses can be done as a later step.
>

Ok, I'll make local read_fifo() and write_fifo() macros to make this
explicit.  Are these names ok with you?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/