Re: [Dri-devel] i810 agpgart curiosity

[Bjorn H. <bjo...@hp...>]

On Monday 14 January 2002 9:57 am, Sottek, Matthew J wrote:
> Why do you think that isn't a virtual address? agp_alloc_page
> gets a page and returns page_addresss(page) which is the kernel
> virtual address for the page, right?

agp_alloc_page (agp_generic_alloc_page in the i810 case) indeed returns a 
kernel virtual address, but the next line I quoted:

                new->memory[0] =
                    agp_bridge.mask_memory(
                                   virt_to_phys((void *) new->memory[0]),
                                                  type);

converts it to a physical address, then intel_i810_mask_memory() ORs in 
the GATT "valid" bit.  The next line I quoted:

                new->physical = virt_to_phys((void *) new->memory[0]);

takes that masked physical address and applies virt_to_phys to it, which 
just seems wrong.  Like I said, I don't have an i810, and I haven't 
exhaustively analyzed it to see, for instance, whether new->physical is 
even used anywhere, but it still looks wrong to me.

intel_i830_alloc_by_type() contains very similar code that makes more 
sense to me:

        if (type == AGP_PHYS_MEMORY) {
                unsigned long physical;
                ...
                nw->memory[0] = agp_bridge.agp_alloc_page();
                physical = nw->memory[0];
                ...
                nw->memory[0] = agp_bridge.mask_memory(virt_to_phys((void 
*) nw->memory[0]),type);
                ...
                nw->physical = virt_to_phys((void *) physical);

Although the temporary "physical" is mis-named, since it actually contains 
a kernel virtual address :-)

Bjorn

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjo...@hp...]
> Sent: Friday, January 11, 2002 10:31 AM
> To: dri...@li...
> Subject: [Dri-devel] i810 agpgart curiosity
>
>
> The following code in agpgart_be.c looks bogus to me:
>
> static agp_memory *intel_i810_alloc_by_type(size_t pg_count, int type)
>         ...
>         if(type == AGP_PHYS_MEMORY) {
>                 ...
>                new->memory[0] = agp_bridge.agp_alloc_page();
>                 ...
>                new->memory[0] =
>                     agp_bridge.mask_memory(
>                                    virt_to_phys((void *)
> new->memory[0]), type);
>                 ...
>                 new->physical = virt_to_phys((void *) new->memory[0]);
>
> I don't have an i810, so I can't verify anything, but it looks like it
> puts garbage in new->physical.  At least, it's calling virt_to_phys() on
> something that is definitely NOT a virtual address!
>
> Any i810 gurus care to comment?

-- 
Bjorn Helgaas - bjo...@hp...
Linux Systems Operation R&D
Hewlett-Packard