| Message ID | 1401021252-29006-4-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi Any chance you could give this a spin on an omap device? It shouldn't affect any 32bit devices, so this is mostly a cosmetic change. However, I'd really like to get rid of that 'TODO' item. So a "Tested-by:" would be really nice. Thanks David On Sun, May 25, 2014 at 2:34 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > OMAP requires bo-pages to be in the DMA32 zone. Explicitly request this by > setting __GFP_DMA32 as mapping-gfp-mask during shmem initialization. This > drops HIGHMEM from the gfp-mask and uses DMA32 instead. shmem-core takes > care to relocate pages during swap-in in case they have been loaded into > the wrong zone. > > It is _not_ possible to pass __GFP_DMA32 to shmem_read_mapping_page_gfp() > as the page might have already been swapped-in at that time. The zone-mask > must be set during initialization and be kept constant for now. > > Remove the now superfluous TODO in omap_gem.c. > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/omapdrm/omap_gem.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c > index 95dbce2..1331fd5 100644 > --- a/drivers/gpu/drm/omapdrm/omap_gem.c > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c > @@ -233,10 +233,6 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj) > > WARN_ON(omap_obj->pages); > > - /* TODO: __GFP_DMA32 .. but somehow GFP_HIGHMEM is coming from the > - * mapping_gfp_mask(mapping) which conflicts w/ GFP_DMA32.. probably > - * we actually want CMA memory for it all anyways.. > - */ > pages = drm_gem_get_pages(obj, GFP_KERNEL); > if (IS_ERR(pages)) { > dev_err(obj->dev->dev, "could not get pages: %ld\n", PTR_ERR(pages)); > @@ -1347,6 +1343,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, > struct omap_drm_private *priv = dev->dev_private; > struct omap_gem_object *omap_obj; > struct drm_gem_object *obj = NULL; > + struct address_space *mapping; > size_t size; > int ret; > > @@ -1404,14 +1401,16 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, > omap_obj->height = gsize.tiled.height; > } > > - ret = 0; > - if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) > + if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) { > drm_gem_private_object_init(dev, obj, size); > - else > + } else { > ret = drm_gem_object_init(dev, obj, size); > + if (ret) > + goto fail; > > - if (ret) > - goto fail; > + mapping = file_inode(obj->filp)->i_mapping; > + mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32); > + } > > return obj; > > -- > 1.9.3 >
Hi, On 02/06/14 16:03, David Herrmann wrote: > Hi Tomi > > Any chance you could give this a spin on an omap device? It shouldn't > affect any 32bit devices, so this is mostly a cosmetic change. > However, I'd really like to get rid of that 'TODO' item. So a > "Tested-by:" would be really nice. I made a quick test on omap4. I verified that the changed code is ran, and I can get a picture on my monitor with omapdrm. So tested by me, but I'm not familiar with shmem so I can't really say anything about the change itself without studying this further. Tomi
On Thu, Jun 5, 2014 at 9:52 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > Hi, > > On 02/06/14 16:03, David Herrmann wrote: >> Hi Tomi >> >> Any chance you could give this a spin on an omap device? It shouldn't >> affect any 32bit devices, so this is mostly a cosmetic change. >> However, I'd really like to get rid of that 'TODO' item. So a >> "Tested-by:" would be really nice. > > I made a quick test on omap4. I verified that the changed code is ran, > and I can get a picture on my monitor with omapdrm. > > So tested by me, but I'm not familiar with shmem so I can't really say > anything about the change itself without studying this further. Thanks for testing it. The change looks ok, I just had a nagging doubt about it because I had problems w/ GFP32 before (but was probably just missing mapping_set_gfp_mask()), so was hoping that someone could confirm that it actually did work as intended. Anyways, with that: Reviewed-by: Rob Clark <robdclark@gmail.com> BR, -R
Hi On Thu, Jun 5, 2014 at 4:04 PM, Rob Clark <robdclark@gmail.com> wrote: > On Thu, Jun 5, 2014 at 9:52 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >> Hi, >> >> On 02/06/14 16:03, David Herrmann wrote: >>> Hi Tomi >>> >>> Any chance you could give this a spin on an omap device? It shouldn't >>> affect any 32bit devices, so this is mostly a cosmetic change. >>> However, I'd really like to get rid of that 'TODO' item. So a >>> "Tested-by:" would be really nice. >> >> I made a quick test on omap4. I verified that the changed code is ran, >> and I can get a picture on my monitor with omapdrm. >> >> So tested by me, but I'm not familiar with shmem so I can't really say >> anything about the change itself without studying this further. > > Thanks for testing it. The change looks ok, I just had a nagging > doubt about it because I had problems w/ GFP32 before (but was > probably just missing mapping_set_gfp_mask()), so was hoping that > someone could confirm that it actually did work as intended. > > Anyways, with that: > > Reviewed-by: Rob Clark <robdclark@gmail.com> Thanks Tomi and Rob! I think to avoid conflicts with PATCH 5/5, Dave should take this through drm-next? In case there're no objections to 5/5.. Thanks David
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 95dbce2..1331fd5 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -233,10 +233,6 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj) WARN_ON(omap_obj->pages); - /* TODO: __GFP_DMA32 .. but somehow GFP_HIGHMEM is coming from the - * mapping_gfp_mask(mapping) which conflicts w/ GFP_DMA32.. probably - * we actually want CMA memory for it all anyways.. - */ pages = drm_gem_get_pages(obj, GFP_KERNEL); if (IS_ERR(pages)) { dev_err(obj->dev->dev, "could not get pages: %ld\n", PTR_ERR(pages)); @@ -1347,6 +1343,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, struct omap_drm_private *priv = dev->dev_private; struct omap_gem_object *omap_obj; struct drm_gem_object *obj = NULL; + struct address_space *mapping; size_t size; int ret; @@ -1404,14 +1401,16 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, omap_obj->height = gsize.tiled.height; } - ret = 0; - if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) + if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) { drm_gem_private_object_init(dev, obj, size); - else + } else { ret = drm_gem_object_init(dev, obj, size); + if (ret) + goto fail; - if (ret) - goto fail; + mapping = file_inode(obj->filp)->i_mapping; + mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32); + } return obj;
OMAP requires bo-pages to be in the DMA32 zone. Explicitly request this by setting __GFP_DMA32 as mapping-gfp-mask during shmem initialization. This drops HIGHMEM from the gfp-mask and uses DMA32 instead. shmem-core takes care to relocate pages during swap-in in case they have been loaded into the wrong zone. It is _not_ possible to pass __GFP_DMA32 to shmem_read_mapping_page_gfp() as the page might have already been swapped-in at that time. The zone-mask must be set during initialization and be kept constant for now. Remove the now superfluous TODO in omap_gem.c. Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/omapdrm/omap_gem.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)