[7/9] drm/gma500: use gem get/put page helpers

Commit Message

Rob Clark Aug. 7, 2013, 5:41 p.m. UTC

Signed-off-by: Rob Clark <[e-mail]>
---
 drivers/gpu/drm/gma500/gtt.c | 38 ++++++--------------------------------
 1 file changed, 6 insertions(+), 32 deletions(-)

Comments

Guillaume Clement Oct. 8, 2013, 7:57 p.m. UTC | #1

On Wed, Aug 07, 2013 at 01:41:25PM -0400, Rob Clark wrote:
> Signed-off-by: Rob Clark <[e-mail]>
> ---
>  drivers/gpu/drm/gma500/gtt.c | 38 ++++++--------------------------------
>  1 file changed, 6 insertions(+), 32 deletions(-)
> [ snip ]

This is quite late to report, but I've just begun testing 3.12, and this
patch makes my screen garbled when using the modesetting xorg driver.

So far it looks like the buffer that the xorg driver gets is
not the one that is actually mapped by the kernel.

I'm currently trying to know what exactly causes the problem, but I don't
know anything about the internals here...


I doubt it will help, but the fbdev driver still works with this patch.


- Guillaume

Patrik Jakobsson Oct. 8, 2013, 8:22 p.m. UTC | #2

On Tue, Oct 8, 2013 at 10:19 PM, Rob Clark <[e-mail]> wrote:
> On Tue, Oct 8, 2013 at 3:57 PM, Guillaume CLÉMENT <[e-mail]> wrote:
>> On Wed, Aug 07, 2013 at 01:41:25PM -0400, Rob Clark wrote:
>>> Signed-off-by: Rob Clark <[e-mail]>
>>> ---
>>>  drivers/gpu/drm/gma500/gtt.c | 38 ++++++--------------------------------
>>>  1 file changed, 6 insertions(+), 32 deletions(-)
>>> [ snip ]
>>
>> This is quite late to report, but I've just begun testing 3.12, and this
>> patch makes my screen garbled when using the modesetting xorg driver.
>>
>> So far it looks like the buffer that the xorg driver gets is
>> not the one that is actually mapped by the kernel.
>>
>> I'm currently trying to know what exactly causes the problem, but I don't
>> know anything about the internals here...
>>
>
> could you try this small patch.. I think I missed something w/ my
> original patch (sorry, no hw to test on here):
>
> ----------
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 92babac..2db731f 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -204,6 +204,7 @@ static int psb_gtt_attach_pages(struct gtt_range *gt)
>      if (IS_ERR(pages))
>          return PTR_ERR(pages);
>
> +    gt->npage = gt->gem.size / PAGE_SIZE;
>      gt->pages = pages;
>
>      return 0;
> ----------
>

Rob, if this works, can you please make sure it goes into Dave's tree.
I'm currently travelling and I'll be away for at least one week.

Thanks
Patrik

Guillaume Clement Oct. 8, 2013, 8:31 p.m. UTC | #3

> This is quite late to report, but I've just begun testing 3.12, and this
> patch makes my screen garbled when using the modesetting xorg driver.
>

Alright, after trying to notice what was different between the old and
new code, I've found the culprit :

-   gt->npage = pages;

There is no equivalent of this line with the new code. This must mess
things up.


By writing:

   gt->npage = gt->gem.size / PAGE_SIZE;

after:
   gt->npage = pages;

I don't experience the problem anymore. If this looks good to you, I'll
create a patch for this.



Thanks,

- Guillaume

Patch

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 1f82183..92babac 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -196,37 +196,17 @@  void psb_gtt_roll(struct drm_device *dev, struct gtt_range *r, int roll)
  */
 static int psb_gtt_attach_pages(struct gtt_range *gt)
 {
-	struct inode *inode;
-	struct address_space *mapping;
-	int i;
-	struct page *p;
-	int pages = gt->gem.size / PAGE_SIZE;
+	struct page **pages;
 
 	WARN_ON(gt->pages);
 
-	/* This is the shared memory object that backs the GEM resource */
-	inode = file_inode(gt->gem.filp);
-	mapping = inode->i_mapping;
+	pages = drm_gem_get_pages(&gt->gem, 0);
+	if (IS_ERR(pages))
+		return PTR_ERR(pages);
 
-	gt->pages = kmalloc(pages * sizeof(struct page *), GFP_KERNEL);
-	if (gt->pages == NULL)
-		return -ENOMEM;
-	gt->npage = pages;
+	gt->pages = pages;
 
-	for (i = 0; i < pages; i++) {
-		p = shmem_read_mapping_page(mapping, i);
-		if (IS_ERR(p))
-			goto err;
-		gt->pages[i] = p;
-	}
 	return 0;
-
-err:
-	while (i--)
-		page_cache_release(gt->pages[i]);
-	kfree(gt->pages);
-	gt->pages = NULL;
-	return PTR_ERR(p);
 }
 
 /**
@@ -240,13 +220,7 @@  err:
  */
 static void psb_gtt_detach_pages(struct gtt_range *gt)
 {
-	int i;
-	for (i = 0; i < gt->npage; i++) {
-		/* FIXME: do we need to force dirty */
-		set_page_dirty(gt->pages[i]);
-		page_cache_release(gt->pages[i]);
-	}
-	kfree(gt->pages);
+	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
 	gt->pages = NULL;
 }