linux-next: build failure after merge of the drm tree

linux-next: build failure after merge of the drm tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

Hi all,

After merging the drm tree, today's linux-next build (x86_64 allmodconfig)
failed like this:

drivers/gpu/drm/ingenic/ingenic-drm-drv.c: In function 'ingenic_drm_sync_data':
drivers/gpu/drm/ingenic/ingenic-drm-drv.c:478:4: error: implicit declaration of function 'dma_cache_sync'; did you mean 'regcache_sync'? [-Werror=implicit-function-declaration]
  478 |    dma_cache_sync(priv->dev, addr + offset,
      |    ^~~~~~~~~~~~~~
      |    regcache_sync
drivers/gpu/drm/ingenic/ingenic-drm-drv.c: In function 'ingenic_drm_gem_mmap':
drivers/gpu/drm/ingenic/ingenic-drm-drv.c:671:11: error: 'DMA_ATTR_NON_CONSISTENT' undeclared (first use in this function)
  671 |   attrs = DMA_ATTR_NON_CONSISTENT;
      |           ^~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/ingenic/ingenic-drm-drv.c:671:11: note: each undeclared identifier is reported only once for each function it appears in

Caused by commit

  37054fc81443 ("gpu/drm: ingenic: Add option to mmap GEM buffers cached")

interacting with commits

  5a8429227140 ("dma-mapping: remove dma_cache_sync")
  efa70f2fdc84 ("dma-mapping: add a new dma_alloc_pages API")

from the dma-mapping tree.

Its not immediately obvious how to fix this up, so I have just marked
CONFIG_DRM_INGENIC as BROKEN until a fix up is provided.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Mon, Sep 28, 2020 at 01:54:05PM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the drm tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:

The driver needs to switch do dma_alloc_noncoherent + dma_sync_single*
like the other drivers converted in the dma tree.  Paul, let me know if
you have any questions.

Re: linux-next: build failure after merge of the drm tree

[Dave Airlie]

On Mon, 28 Sep 2020 at 16:05, Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Sep 28, 2020 at 01:54:05PM +1000, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the drm tree, today's linux-next build (x86_64 allmodconfig)
> > failed like this:
>
> The driver needs to switch do dma_alloc_noncoherent + dma_sync_single*
> like the other drivers converted in the dma tree.  Paul, let me know if
> you have any questions.

Is this possible in drm-next now (it's 5.9.0-rc5 based)?

or will I need to get a stable shared git tree that goes into drm-next
and you send to Linus early in the MR?

Dave.

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Mon, Sep 28, 2020 at 04:08:36PM +1000, Dave Airlie wrote:
> Is this possible in drm-next now (it's 5.9.0-rc5 based)?
> 
> or will I need to get a stable shared git tree that goes into drm-next
> and you send to Linus early in the MR?

I think we'll need a stable branch.   Let me help Paul with the
conversion first, and once we are done I'll create a shared branch.

Re: linux-next: build failure after merge of the drm tree

[Paul Cercueil]

Hi Christoph,

Le lun. 28 sept. 2020 à 8:04, Christoph Hellwig <hch@lst.de> a écrit :
> On Mon, Sep 28, 2020 at 01:54:05PM +1000, Stephen Rothwell wrote:
>>  Hi all,
>> 
>>  After merging the drm tree, today's linux-next build (x86_64 
>> allmodconfig)
>>  failed like this:
> 
> The driver needs to switch do dma_alloc_noncoherent + dma_sync_single*
> like the other drivers converted in the dma tree.  Paul, let me know 
> if
> you have any questions.

I don't dma_alloc* anything, DRM core does. I use the 
DMA_ATTR_NON_CONSISTENT attr with dma_mmap_attrs(). Is there a 
replacement for that?

-Paul

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Mon, Sep 28, 2020 at 12:15:56PM +0200, Paul Cercueil wrote:
> Hi Christoph,
>
> Le lun. 28 sept. 2020 à 8:04, Christoph Hellwig <hch@lst.de> a écrit :
>> On Mon, Sep 28, 2020 at 01:54:05PM +1000, Stephen Rothwell wrote:
>>>  Hi all,
>>>
>>>  After merging the drm tree, today's linux-next build (x86_64 
>>> allmodconfig)
>>>  failed like this:
>>
>> The driver needs to switch do dma_alloc_noncoherent + dma_sync_single*
>> like the other drivers converted in the dma tree.  Paul, let me know if
>> you have any questions.
>
> I don't dma_alloc* anything, DRM core does. I use the 
> DMA_ATTR_NON_CONSISTENT attr with dma_mmap_attrs(). Is there a replacement 
> for that?

dma_mmap_attrs can only be used on allocations from dma_mmap_attrs with
the same attrs.  As there is no allocation using DMA_ATTR_NON_CONSISTENT
in the drm core, something looks very fishy here.

Where does the allocation you try to mmap come from?  All the allocations
in drivers/gpu/drm/drm_gem_cma_helper.c seems to use dma_alloc_wc (aka
dma_allloc_attrs with the DMA_ATTR_WRITE_COMBINE flag).

Re: linux-next: build failure after merge of the drm tree

[Paul Cercueil]

Le lun. 28 sept. 2020 à 13:34, Christoph Hellwig <hch@lst.de> a écrit 
:
> On Mon, Sep 28, 2020 at 12:15:56PM +0200, Paul Cercueil wrote:
>>  Hi Christoph,
>> 
>>  Le lun. 28 sept. 2020 à 8:04, Christoph Hellwig <hch@lst.de> a 
>> écrit :
>>>  On Mon, Sep 28, 2020 at 01:54:05PM +1000, Stephen Rothwell wrote:
>>>>   Hi all,
>>>> 
>>>>   After merging the drm tree, today's linux-next build (x86_64
>>>>  allmodconfig)
>>>>   failed like this:
>>> 
>>>  The driver needs to switch do dma_alloc_noncoherent + 
>>> dma_sync_single*
>>>  like the other drivers converted in the dma tree.  Paul, let me 
>>> know if
>>>  you have any questions.
>> 
>>  I don't dma_alloc* anything, DRM core does. I use the
>>  DMA_ATTR_NON_CONSISTENT attr with dma_mmap_attrs(). Is there a 
>> replacement
>>  for that?
> 
> dma_mmap_attrs can only be used on allocations from dma_mmap_attrs 
> with
> the same attrs.  As there is no allocation using 
> DMA_ATTR_NON_CONSISTENT
> in the drm core, something looks very fishy here.

Is that a fact? I don't see why you couldn't change the cache settings 
after allocation. In practice it works just fine.

> Where does the allocation you try to mmap come from?  All the 
> allocations
> in drivers/gpu/drm/drm_gem_cma_helper.c seems to use dma_alloc_wc (aka
> dma_allloc_attrs with the DMA_ATTR_WRITE_COMBINE flag).

It's the dma_alloc_wc.

-Paul

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Mon, Sep 28, 2020 at 01:46:55PM +0200, Paul Cercueil wrote:
>> dma_mmap_attrs can only be used on allocations from dma_mmap_attrs with
>> the same attrs.  As there is no allocation using DMA_ATTR_NON_CONSISTENT
>> in the drm core, something looks very fishy here.
>
> Is that a fact? I don't see why you couldn't change the cache settings 
> after allocation. In practice it works just fine.

Accessing the same physical address using different caching attributes
is undefined behavior and fairly dangerous on most architectures, and
thus not supported by the DMA API.

Re: linux-next: build failure after merge of the drm tree

[Paul Cercueil]

Le lun. 28 sept. 2020 à 14:10, Christoph Hellwig <hch@lst.de> a écrit 
:
> On Mon, Sep 28, 2020 at 01:46:55PM +0200, Paul Cercueil wrote:
>>>  dma_mmap_attrs can only be used on allocations from dma_mmap_attrs 
>>> with
>>>  the same attrs.  As there is no allocation using 
>>> DMA_ATTR_NON_CONSISTENT
>>>  in the drm core, something looks very fishy here.
>> 
>>  Is that a fact? I don't see why you couldn't change the cache 
>> settings
>>  after allocation. In practice it works just fine.
> 
> Accessing the same physical address using different caching attributes
> is undefined behavior and fairly dangerous on most architectures, and
> thus not supported by the DMA API.

It's allocated with dma_alloc_wc, but then it's only accessed as 
non-coherent.

Anyway, for the time being I guess you could revert 37054fc81443. But I 
have patches on top of it in drm-misc-next so it's going to be a mess.

If we have time I can come up with a custom dumb_create() fonction, to 
make sure that the GEM buffers are allocated with 
dma_alloc_noncoherent(). Is there a dma_mmap_noncoherent() too?

-Paul

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Mon, Sep 28, 2020 at 03:31:28PM +0200, Paul Cercueil wrote:
> It's allocated with dma_alloc_wc, but then it's only accessed as 
> non-coherent.
>
> Anyway, for the time being I guess you could revert 37054fc81443. But I 
> have patches on top of it in drm-misc-next so it's going to be a mess.
>
> If we have time I can come up with a custom dumb_create() fonction, to make 
> sure that the GEM buffers are allocated with dma_alloc_noncoherent(). Is 
> there a dma_mmap_noncoherent() too?

Please use the lower-level dma_alloc_pages and then just insert the
pages directly using remap_pfn_range.  Although it might make sense
to eventually create a wrapper around remap_pfn_range for all the
vma sizing sanity checks.


>
> -Paul
>
---end quoted text---

Re: linux-next: build failure after merge of the drm tree

[Paul Cercueil]

Hi Christoph,

Le mer. 30 sept. 2020 à 11:02, Christoph Hellwig <hch@lst.de> a écrit 
:
> On Mon, Sep 28, 2020 at 03:31:28PM +0200, Paul Cercueil wrote:
>>  It's allocated with dma_alloc_wc, but then it's only accessed as
>>  non-coherent.
>> 
>>  Anyway, for the time being I guess you could revert 37054fc81443. 
>> But I
>>  have patches on top of it in drm-misc-next so it's going to be a 
>> mess.
>> 
>>  If we have time I can come up with a custom dumb_create() fonction, 
>> to make
>>  sure that the GEM buffers are allocated with 
>> dma_alloc_noncoherent(). Is
>>  there a dma_mmap_noncoherent() too?
> 
> Please use the lower-level dma_alloc_pages and then just insert the
> pages directly using remap_pfn_range.  Although it might make sense
> to eventually create a wrapper around remap_pfn_range for all the
> vma sizing sanity checks.

One thing missing for remap_pfn_range(), I have no alternative for this:

vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, 
DMA_ATTR_NON_CONSISTENT);

So I have to do:

vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
pgprot_val(vma->vm_page_prot) &= ~_CACHE_MASK;
pgprot_val(vma->vm_page_prot) |= _CACHE_CACHABLE_NONCOHERENT;

And that will only compile on MIPS, because these _CACHE_* macros are 
only defined there.

I would need something like a pgprot_noncoherent(), I think.

-Paul

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Wed, Sep 30, 2020 at 03:33:13PM +0200, Paul Cercueil wrote:
> One thing missing for remap_pfn_range(), I have no alternative for this:
>
> vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, 
> DMA_ATTR_NON_CONSISTENT);
>
> So I have to do:
>
> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> pgprot_val(vma->vm_page_prot) &= ~_CACHE_MASK;
> pgprot_val(vma->vm_page_prot) |= _CACHE_CACHABLE_NONCOHERENT;
>
> And that will only compile on MIPS, because these _CACHE_* macros are only 
> defined there.
>
> I would need something like a pgprot_noncoherent(), I think.

dma_alloc_pages gives you cached memory, so you can't just use an
uncached protection for the userspace mmap here.  If you want uncached
memory you need to use dma_alloc_coherent paired with dma_mmap_coherent.
Or dma_alloc_wc for a slightly different flavor of uncached. (both
of the map to dma_alloc_attrs / dma_mmap_attrs eventually).

Re: linux-next: build failure after merge of the drm tree

[Paul Cercueil]

Le mer. 30 sept. 2020 à 18:11, Christoph Hellwig <hch@lst.de> a écrit 
:
> On Wed, Sep 30, 2020 at 03:33:13PM +0200, Paul Cercueil wrote:
>>  One thing missing for remap_pfn_range(), I have no alternative for 
>> this:
>> 
>>  vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot,
>>  DMA_ATTR_NON_CONSISTENT);
>> 
>>  So I have to do:
>> 
>>  vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>>  pgprot_val(vma->vm_page_prot) &= ~_CACHE_MASK;
>>  pgprot_val(vma->vm_page_prot) |= _CACHE_CACHABLE_NONCOHERENT;
>> 
>>  And that will only compile on MIPS, because these _CACHE_* macros 
>> are only
>>  defined there.
>> 
>>  I would need something like a pgprot_noncoherent(), I think.
> 
> dma_alloc_pages gives you cached memory, so you can't just use an
> uncached protection for the userspace mmap here.  If you want uncached
> memory you need to use dma_alloc_coherent paired with 
> dma_mmap_coherent.
> Or dma_alloc_wc for a slightly different flavor of uncached. (both
> of the map to dma_alloc_attrs / dma_mmap_attrs eventually).

I don't want uncached memory, I want non-coherent cached memory.

-Paul

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Wed, Sep 30, 2020 at 06:39:18PM +0200, Paul Cercueil wrote:
>> dma_alloc_pages gives you cached memory, so you can't just use an
>> uncached protection for the userspace mmap here.  If you want uncached
>> memory you need to use dma_alloc_coherent paired with dma_mmap_coherent.
>> Or dma_alloc_wc for a slightly different flavor of uncached. (both
>> of the map to dma_alloc_attrs / dma_mmap_attrs eventually).
>
> I don't want uncached memory, I want non-coherent cached memory.

We don't have such a thing in the Linux API at all.

Re: linux-next: build failure after merge of the drm tree

[Paul Cercueil]

Le mer. 30 sept. 2020 à 18:40, Christoph Hellwig <hch@lst.de> a écrit 
:
> On Wed, Sep 30, 2020 at 06:39:18PM +0200, Paul Cercueil wrote:
>>>  dma_alloc_pages gives you cached memory, so you can't just use an
>>>  uncached protection for the userspace mmap here.  If you want 
>>> uncached
>>>  memory you need to use dma_alloc_coherent paired with 
>>> dma_mmap_coherent.
>>>  Or dma_alloc_wc for a slightly different flavor of uncached. (both
>>>  of the map to dma_alloc_attrs / dma_mmap_attrs eventually).
>> 
>>  I don't want uncached memory, I want non-coherent cached memory.
> 
> We don't have such a thing in the Linux API at all.

dma_pgprot(dev, vma->vm_page_prot, DMA_ATTR_NON_CONSISTENT);

That was giving me non-coherent cached memory, and now I don't have an 
alternative.

-Paul

Re: linux-next: build failure after merge of the drm tree

[Christoph Hellwig]

On Wed, Sep 30, 2020 at 06:45:02PM +0200, Paul Cercueil wrote:
>> We don't have such a thing in the Linux API at all.
>
> dma_pgprot(dev, vma->vm_page_prot, DMA_ATTR_NON_CONSISTENT);
>
> That was giving me non-coherent cached memory, and now I don't have an 
> alternative.

Looking at Linux 5.9-rc dma_pgprot is defined as:

pgprot_t dma_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs)
{
	if (force_dma_unencrypted(dev))
		prot = pgprot_decrypted(prot);
	if (dev_is_dma_coherent(dev) ||
	    (IS_ENABLED(CONFIG_DMA_NONCOHERENT_CACHE_SYNC) &&
             (attrs & DMA_ATTR_NON_CONSISTENT)))
		return prot;
#ifdef CONFIG_ARCH_HAS_DMA_WRITE_COMBINE
	if (attrs & DMA_ATTR_WRITE_COMBINE)
		return pgprot_writecombine(prot);
#endif
	return pgprot_dmacoherent(prot);
}

so it doesn't change vma->vm_page_prot at all.

The only place that uses _CACHE_CACHABLE_NONCOHERENT is the MIPS specific
kmap_noncoherent which ha sa single caller that doesn't leak anywhere
into driver code.

[PATCH 1/3] drm: Add and export function drm_gem_cma_create_noalloc

[Paul Cercueil]

Add and export the function drm_gem_cma_create_noalloc(), which is just
__drm_gem_cma_create() renamed.

This function can be used by drivers that need to create a GEM object
without allocating the backing memory.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 11 ++++++-----
 include/drm/drm_gem_cma_helper.h     |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 59b9ca207b42..6abc4b306832 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -34,7 +34,7 @@
  */
 
 /**
- * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
+ * drm_gem_cma_create_noalloc - Create a GEM CMA object without allocating memory
  * @drm: DRM device
  * @size: size of the object to allocate
  *
@@ -45,8 +45,8 @@
  * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
  * error code on failure.
  */
-static struct drm_gem_cma_object *
-__drm_gem_cma_create(struct drm_device *drm, size_t size)
+struct drm_gem_cma_object *
+drm_gem_cma_create_noalloc(struct drm_device *drm, size_t size)
 {
 	struct drm_gem_cma_object *cma_obj;
 	struct drm_gem_object *gem_obj;
@@ -76,6 +76,7 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
 	kfree(cma_obj);
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_GPL(drm_gem_cma_create_noalloc);
 
 /**
  * drm_gem_cma_create - allocate an object with the given size
@@ -98,7 +99,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 
 	size = round_up(size, PAGE_SIZE);
 
-	cma_obj = __drm_gem_cma_create(drm, size);
+	cma_obj = drm_gem_cma_create_noalloc(drm, size);
 	if (IS_ERR(cma_obj))
 		return cma_obj;
 
@@ -476,7 +477,7 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
 		return ERR_PTR(-EINVAL);
 
 	/* Create a CMA GEM buffer. */
-	cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size);
+	cma_obj = drm_gem_cma_create_noalloc(dev, attach->dmabuf->size);
 	if (IS_ERR(cma_obj))
 		return ERR_CAST(cma_obj);
 
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 2bfa2502607a..be2b8e3a8ab2 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -83,6 +83,9 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 					      size_t size);
 
+struct drm_gem_cma_object *
+drm_gem_cma_create_noalloc(struct drm_device *drm, size_t size);
+
 extern const struct vm_operations_struct drm_gem_cma_vm_ops;
 
 #ifndef CONFIG_MMU
-- 
2.28.0

Re: [PATCH 1/3] drm: Add and export function drm_gem_cma_create_noalloc

[Daniel Vetter]

On Wed, Sep 30, 2020 at 07:16:42PM +0200, Paul Cercueil wrote:
> Add and export the function drm_gem_cma_create_noalloc(), which is just
> __drm_gem_cma_create() renamed.
> 
> This function can be used by drivers that need to create a GEM object
> without allocating the backing memory.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c | 11 ++++++-----
>  include/drm/drm_gem_cma_helper.h     |  3 +++
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 59b9ca207b42..6abc4b306832 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -34,7 +34,7 @@
>   */
>  
>  /**
> - * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
> + * drm_gem_cma_create_noalloc - Create a GEM CMA object without allocating memory
>   * @drm: DRM device
>   * @size: size of the object to allocate
>   *
> @@ -45,8 +45,8 @@
>   * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
>   * error code on failure.
>   */
> -static struct drm_gem_cma_object *
> -__drm_gem_cma_create(struct drm_device *drm, size_t size)
> +struct drm_gem_cma_object *
> +drm_gem_cma_create_noalloc(struct drm_device *drm, size_t size)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	struct drm_gem_object *gem_obj;
> @@ -76,6 +76,7 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
>  	kfree(cma_obj);
>  	return ERR_PTR(ret);
>  }
> +EXPORT_SYMBOL_GPL(drm_gem_cma_create_noalloc);

This feels a bit awkward, since for drivers who want to roll their own we
can do that already.

I think the better approach is to export a cma function which allocates
non-coherent dma memory.
-Daniel

>  
>  /**
>   * drm_gem_cma_create - allocate an object with the given size
> @@ -98,7 +99,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  
>  	size = round_up(size, PAGE_SIZE);
>  
> -	cma_obj = __drm_gem_cma_create(drm, size);
> +	cma_obj = drm_gem_cma_create_noalloc(drm, size);
>  	if (IS_ERR(cma_obj))
>  		return cma_obj;
>  
> @@ -476,7 +477,7 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  		return ERR_PTR(-EINVAL);
>  
>  	/* Create a CMA GEM buffer. */
> -	cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size);
> +	cma_obj = drm_gem_cma_create_noalloc(dev, attach->dmabuf->size);
>  	if (IS_ERR(cma_obj))
>  		return ERR_CAST(cma_obj);
>  
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 2bfa2502607a..be2b8e3a8ab2 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -83,6 +83,9 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  					      size_t size);
>  
> +struct drm_gem_cma_object *
> +drm_gem_cma_create_noalloc(struct drm_device *drm, size_t size);
> +
>  extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>  
>  #ifndef CONFIG_MMU
> -- 
> 2.28.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 2/3] drm/ingenic: Update code to mmap GEM buffers cached

[Paul Cercueil]

The DMA API changed at the same time commit 37054fc81443 ("gpu/drm:
ingenic: Add option to mmap GEM buffers cached") was added. Rework the
code to work with the new DMA API.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 24 +++++++----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 0225dc1f5eb8..07a1da7266e4 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -526,12 +526,10 @@ void ingenic_drm_sync_data(struct device *dev,
 			   struct drm_plane_state *state)
 {
 	const struct drm_format_info *finfo = state->fb->format;
-	struct ingenic_drm *priv = dev_get_drvdata(dev);
 	struct drm_atomic_helper_damage_iter iter;
 	unsigned int offset, i;
 	struct drm_rect clip;
 	dma_addr_t paddr;
-	void *addr;
 
 	if (!ingenic_drm_cached_gem_buf)
 		return;
@@ -541,12 +539,11 @@ void ingenic_drm_sync_data(struct device *dev,
 	drm_atomic_for_each_plane_damage(&iter, &clip) {
 		for (i = 0; i < finfo->num_planes; i++) {
 			paddr = drm_fb_cma_get_gem_addr(state->fb, state, i);
-			addr = phys_to_virt(paddr);
 
 			/* Ignore x1/x2 values, invalidate complete lines */
 			offset = clip.y1 * state->fb->pitches[i];
 
-			dma_cache_sync(priv->dev, addr + offset,
+			dma_sync_single_for_device(dev, paddr + offset,
 				       (clip.y2 - clip.y1) * state->fb->pitches[i],
 				       DMA_TO_DEVICE);
 		}
@@ -766,14 +763,6 @@ static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
 				struct vm_area_struct *vma)
 {
 	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
-	struct device *dev = cma_obj->base.dev->dev;
-	unsigned long attrs;
-	int ret;
-
-	if (ingenic_drm_cached_gem_buf)
-		attrs = DMA_ATTR_NON_CONSISTENT;
-	else
-		attrs = DMA_ATTR_WRITE_COMBINE;
 
 	/*
 	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -784,12 +773,13 @@ static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
 	vma->vm_pgoff = 0;
 	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 
-	ret = dma_mmap_attrs(dev, vma, cma_obj->vaddr, cma_obj->paddr,
-			     vma->vm_end - vma->vm_start, attrs);
-	if (ret)
-		drm_gem_vm_close(vma);
+	if (!ingenic_drm_cached_gem_buf)
+		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
 
-	return ret;
+	return remap_pfn_range(vma, vma->vm_start,
+			       cma_obj->paddr >> PAGE_SHIFT,
+			       vma->vm_end - vma->vm_start,
+			       vma->vm_page_prot);
 }
 
 static int ingenic_drm_gem_cma_mmap(struct file *filp,
-- 
2.28.0

Re: [PATCH 2/3] drm/ingenic: Update code to mmap GEM buffers cached

[Christoph Hellwig]

On Wed, Sep 30, 2020 at 07:16:43PM +0200, Paul Cercueil wrote:
> The DMA API changed at the same time commit 37054fc81443 ("gpu/drm:
> ingenic: Add option to mmap GEM buffers cached") was added. Rework the
> code to work with the new DMA API.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 24 +++++++----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 0225dc1f5eb8..07a1da7266e4 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -526,12 +526,10 @@ void ingenic_drm_sync_data(struct device *dev,
>  			   struct drm_plane_state *state)
>  {
>  	const struct drm_format_info *finfo = state->fb->format;
> -	struct ingenic_drm *priv = dev_get_drvdata(dev);
>  	struct drm_atomic_helper_damage_iter iter;
>  	unsigned int offset, i;
>  	struct drm_rect clip;
>  	dma_addr_t paddr;
> -	void *addr;
>  
>  	if (!ingenic_drm_cached_gem_buf)
>  		return;
> @@ -541,12 +539,11 @@ void ingenic_drm_sync_data(struct device *dev,
>  	drm_atomic_for_each_plane_damage(&iter, &clip) {
>  		for (i = 0; i < finfo->num_planes; i++) {
>  			paddr = drm_fb_cma_get_gem_addr(state->fb, state, i);
> -			addr = phys_to_virt(paddr);

No on the old code: drm_fb_cma_get_gem_addr returns a dma_addr_t, so
this was already pretty broken..

> @@ -766,14 +763,6 @@ static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
>  				struct vm_area_struct *vma)
>  {
>  	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>  
>  	/*
>  	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> @@ -784,12 +773,13 @@ static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
>  	vma->vm_pgoff = 0;
>  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  
> +	if (!ingenic_drm_cached_gem_buf)
> +		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>  
> +	return remap_pfn_range(vma, vma->vm_start,
> +			       cma_obj->paddr >> PAGE_SHIFT,
> +			       vma->vm_end - vma->vm_start,
> +			       vma->vm_page_prot);

both ->vaddr and ->paddr come from dma_alloc_wc as far as I can tell,
and despite the confusing name ->paddr is a dma_addr_t.  So this can't
work at all.  If you allocate memory using dma_alloc_wc you need to
map it using dma_alloc_wc.

[PATCH 3/3] drm/ingenic: Alloc cached GEM buffers with dma_alloc_noncoherent

[Paul Cercueil]

It turns out that if you want to mmap GEM buffers fully cached, then
they should be allocated as such as well. Who would have known?

Introduce a custom .dumb_create callback, that will behave just like
drm_gem_cma_dumb_create(), except that it will allocate the GEM buffer
using dma_alloc_noncoherent() if non-coherent memory is what we want.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 48 ++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 07a1da7266e4..8ece269c040f 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -794,6 +794,52 @@ static int ingenic_drm_gem_cma_mmap(struct file *filp,
 	return ingenic_drm_gem_mmap(vma->vm_private_data, vma);
 }
 
+static int ingenic_drm_gem_cma_dumb_create(struct drm_file *file_priv,
+					   struct drm_device *drm,
+					   struct drm_mode_create_dumb *args)
+{
+	/*
+	 * This is basically a copy of drm_gem_cma_dumb_create, which supports
+	 * creating fully cached GEM buffers.
+	 */
+	struct drm_gem_cma_object *cma_obj;
+	struct drm_gem_object *gem_obj;
+	size_t size;
+	int ret;
+
+	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	args->size = args->pitch * args->height;
+
+	size = PAGE_ALIGN(args->size);
+
+	cma_obj = drm_gem_cma_create_noalloc(drm, size);
+	if (IS_ERR(cma_obj))
+		return PTR_ERR(cma_obj);
+
+	if (ingenic_drm_cached_gem_buf) {
+		cma_obj->vaddr = dma_alloc_noncoherent(drm->dev, size,
+						       &cma_obj->paddr,
+						       DMA_TO_DEVICE,
+						       GFP_KERNEL | __GFP_NOWARN);
+	} else {
+		cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
+					      GFP_KERNEL | __GFP_NOWARN);
+	}
+	if (!cma_obj->vaddr) {
+		dev_err(drm->dev, "Failed to allocate buffer with size %zu\n", size);
+		ret = -ENOMEM;
+		goto out_gem_object_put;
+	}
+
+	gem_obj = &cma_obj->base;
+
+	ret = drm_gem_handle_create(file_priv, gem_obj, &args->handle);
+
+out_gem_object_put:
+	drm_gem_object_put(gem_obj);
+	return ret;
+}
+
 static const struct file_operations ingenic_drm_fops = {
 	.owner		= THIS_MODULE,
 	.open		= drm_open,
@@ -816,7 +862,7 @@ static struct drm_driver ingenic_drm_driver_data = {
 	.patchlevel		= 0,
 
 	.fops			= &ingenic_drm_fops,
-	DRM_GEM_CMA_DRIVER_OPS,
+	DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE(ingenic_drm_gem_cma_dumb_create),
 
 	.irq_handler		= ingenic_drm_irq_handler,
 };
-- 
2.28.0

Re: [PATCH 3/3] drm/ingenic: Alloc cached GEM buffers with dma_alloc_noncoherent

[Christoph Hellwig]

On Wed, Sep 30, 2020 at 07:16:44PM +0200, Paul Cercueil wrote:
> It turns out that if you want to mmap GEM buffers fully cached, then
> they should be allocated as such as well. Who would have known?
> 
> Introduce a custom .dumb_create callback, that will behave just like
> drm_gem_cma_dumb_create(), except that it will allocate the GEM buffer
> using dma_alloc_noncoherent() if non-coherent memory is what we want.

I think you want to merge this with patch 2, then change patch 2 to
still use dma_alloc_wc for the !ingenic_drm_cached_gem_buf and to
get the phys address using virt_to_phys for the
ingenic_drm_cached_gem_buf instead of abusing the dma address in
->paddr.  The free side also needs to use
dma_free_noncoherent.

> +static int ingenic_drm_gem_cma_dumb_create(struct drm_file *file_priv,
> +					   struct drm_device *drm,
> +					   struct drm_mode_create_dumb *args)
> +{
> +	/*
> +	 * This is basically a copy of drm_gem_cma_dumb_create, which supports
> +	 * creating fully cached GEM buffers.
> +	 */

What about adding this to the core GEM code instead?  We'll probaby
run into other potential users as well.

[PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Paul Cercueil]

This reverts commit 37054fc81443cc6a8c3a38395f384412b8373d82.

At the very moment this commit was created, the DMA API it relied on was
modified in the DMA tree, which caused the driver to break in
linux-next.

Revert it for now, and it will be resubmitted later to work with the new
DMA API.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 114 +---------------------
 drivers/gpu/drm/ingenic/ingenic-drm.h     |   4 -
 drivers/gpu/drm/ingenic/ingenic-ipu.c     |  12 +--
 3 files changed, 4 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 0225dc1f5eb8..7d8b0ad52979 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -9,8 +9,6 @@
 #include <linux/component.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
-#include <linux/dma-noncoherent.h>
-#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
@@ -24,7 +22,6 @@
 #include <drm/drm_color_mgmt.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
-#include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
@@ -100,11 +97,6 @@ struct ingenic_drm {
 	struct notifier_block clock_nb;
 };
 
-static bool ingenic_drm_cached_gem_buf;
-module_param_named(cached_gem_buffers, ingenic_drm_cached_gem_buf, bool, 0400);
-MODULE_PARM_DESC(cached_gem_buffers,
-		 "Enable fully cached GEM buffers [default=false]");
-
 static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -402,8 +394,6 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 	     plane->state->fb->format->format != state->fb->format->format))
 		crtc_state->mode_changed = true;
 
-	drm_atomic_helper_check_plane_damage(state->state, state);
-
 	return 0;
 }
 
@@ -521,38 +511,6 @@ void ingenic_drm_plane_config(struct device *dev,
 	}
 }
 
-void ingenic_drm_sync_data(struct device *dev,
-			   struct drm_plane_state *old_state,
-			   struct drm_plane_state *state)
-{
-	const struct drm_format_info *finfo = state->fb->format;
-	struct ingenic_drm *priv = dev_get_drvdata(dev);
-	struct drm_atomic_helper_damage_iter iter;
-	unsigned int offset, i;
-	struct drm_rect clip;
-	dma_addr_t paddr;
-	void *addr;
-
-	if (!ingenic_drm_cached_gem_buf)
-		return;
-
-	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
-
-	drm_atomic_for_each_plane_damage(&iter, &clip) {
-		for (i = 0; i < finfo->num_planes; i++) {
-			paddr = drm_fb_cma_get_gem_addr(state->fb, state, i);
-			addr = phys_to_virt(paddr);
-
-			/* Ignore x1/x2 values, invalidate complete lines */
-			offset = clip.y1 * state->fb->pitches[i];
-
-			dma_cache_sync(priv->dev, addr + offset,
-				       (clip.y2 - clip.y1) * state->fb->pitches[i],
-				       DMA_TO_DEVICE);
-		}
-	}
-}
-
 static void ingenic_drm_update_palette(struct ingenic_drm *priv,
 				       const struct drm_color_lut *lut)
 {
@@ -581,8 +539,6 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 	if (state && state->fb) {
 		crtc_state = state->crtc->state;
 
-		ingenic_drm_sync_data(priv->dev, oldstate, state);
-
 		addr = drm_fb_cma_get_gem_addr(state->fb, state, 0);
 		width = state->src_w >> 16;
 		height = state->src_h >> 16;
@@ -752,69 +708,7 @@ static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
 	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, 0);
 }
 
-static struct drm_framebuffer *
-ingenic_drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
-			  const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	if (ingenic_drm_cached_gem_buf)
-		return drm_gem_fb_create_with_dirty(dev, file, mode_cmd);
-
-	return drm_gem_fb_create(dev, file, mode_cmd);
-}
-
-static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
-				struct vm_area_struct *vma)
-{
-	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
-	struct device *dev = cma_obj->base.dev->dev;
-	unsigned long attrs;
-	int ret;
-
-	if (ingenic_drm_cached_gem_buf)
-		attrs = DMA_ATTR_NON_CONSISTENT;
-	else
-		attrs = DMA_ATTR_WRITE_COMBINE;
-
-	/*
-	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
-	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
-	 * the whole buffer.
-	 */
-	vma->vm_flags &= ~VM_PFNMAP;
-	vma->vm_pgoff = 0;
-	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
-
-	ret = dma_mmap_attrs(dev, vma, cma_obj->vaddr, cma_obj->paddr,
-			     vma->vm_end - vma->vm_start, attrs);
-	if (ret)
-		drm_gem_vm_close(vma);
-
-	return ret;
-}
-
-static int ingenic_drm_gem_cma_mmap(struct file *filp,
-				    struct vm_area_struct *vma)
-{
-	int ret;
-
-	ret = drm_gem_mmap(filp, vma);
-	if (ret)
-		return ret;
-
-	return ingenic_drm_gem_mmap(vma->vm_private_data, vma);
-}
-
-static const struct file_operations ingenic_drm_fops = {
-	.owner		= THIS_MODULE,
-	.open		= drm_open,
-	.release	= drm_release,
-	.unlocked_ioctl	= drm_ioctl,
-	.compat_ioctl	= drm_compat_ioctl,
-	.poll		= drm_poll,
-	.read		= drm_read,
-	.llseek		= noop_llseek,
-	.mmap		= ingenic_drm_gem_cma_mmap,
-};
+DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
 
 static struct drm_driver ingenic_drm_driver_data = {
 	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
@@ -878,7 +772,7 @@ static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs =
 };
 
 static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
-	.fb_create		= ingenic_drm_gem_fb_create,
+	.fb_create		= drm_gem_fb_create,
 	.output_poll_changed	= drm_fb_helper_output_poll_changed,
 	.atomic_check		= drm_atomic_helper_check,
 	.atomic_commit		= drm_atomic_helper_commit,
@@ -1032,8 +926,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 		return ret;
 	}
 
-	drm_plane_enable_fb_damage_clips(&priv->f1);
-
 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
 
 	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
@@ -1062,8 +954,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			return ret;
 		}
 
-		drm_plane_enable_fb_damage_clips(&priv->f0);
-
 		if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU) && has_components) {
 			ret = component_bind_all(dev, drm);
 			if (ret) {
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
index ee3a892c0383..9b48ce02803d 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm.h
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
@@ -171,10 +171,6 @@ void ingenic_drm_plane_config(struct device *dev,
 			      struct drm_plane *plane, u32 fourcc);
 void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane);
 
-void ingenic_drm_sync_data(struct device *dev,
-			   struct drm_plane_state *old_state,
-			   struct drm_plane_state *state);
-
 extern struct platform_driver *ingenic_ipu_driver_ptr;
 
 #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 38c83e8cc6a5..fc8c6e970ee3 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -20,7 +20,6 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
-#include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fourcc.h>
@@ -317,8 +316,6 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
 				JZ_IPU_CTRL_CHIP_EN | JZ_IPU_CTRL_LCDC_SEL);
 	}
 
-	ingenic_drm_sync_data(ipu->master, oldstate, state);
-
 	/* New addresses will be committed in vblank handler... */
 	ipu->addr_y = drm_fb_cma_get_gem_addr(state->fb, state, 0);
 	if (finfo->num_planes > 1)
@@ -537,7 +534,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 
 	if (!state->crtc ||
 	    !crtc_state->mode.hdisplay || !crtc_state->mode.vdisplay)
-		goto out_check_damage;
+		return 0;
 
 	/* Plane must be fully visible */
 	if (state->crtc_x < 0 || state->crtc_y < 0 ||
@@ -554,7 +551,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 
 	if (!osd_changed(state, plane->state))
-		goto out_check_damage;
+		return 0;
 
 	crtc_state->mode_changed = true;
 
@@ -581,9 +578,6 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 	ipu->denom_w = denom_w;
 	ipu->denom_h = denom_h;
 
-out_check_damage:
-	drm_atomic_helper_check_plane_damage(state->state, state);
-
 	return 0;
 }
 
@@ -765,8 +759,6 @@ static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
 		return err;
 	}
 
-	drm_plane_enable_fb_damage_clips(plane);
-
 	/*
 	 * Sharpness settings range is [0,32]
 	 * 0       : nearest-neighbor
-- 
2.28.0

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Sam Ravnborg]

Hi Paul.

On Sun, Oct 04, 2020 at 04:17:58PM +0200, Paul Cercueil wrote:
> This reverts commit 37054fc81443cc6a8c3a38395f384412b8373d82.

In the changelog please refer to commits like this:
37054fc81443 ("gpu/drm: ingenic: Add option to mmap GEM buffers cached")

Use "dim cite 37054fc81443cc6a8c3a38395f384412b8373d82" to get the right format.

> 
> At the very moment this commit was created, the DMA API it relied on was
> modified in the DMA tree, which caused the driver to break in
> linux-next.
> 
> Revert it for now, and it will be resubmitted later to work with the new
> DMA API.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

With the changelog updated:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 114 +---------------------
>  drivers/gpu/drm/ingenic/ingenic-drm.h     |   4 -
>  drivers/gpu/drm/ingenic/ingenic-ipu.c     |  12 +--
>  3 files changed, 4 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 0225dc1f5eb8..7d8b0ad52979 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -9,8 +9,6 @@
>  #include <linux/component.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
> -#include <linux/dma-noncoherent.h>
> -#include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> @@ -24,7 +22,6 @@
>  #include <drm/drm_color_mgmt.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> -#include <drm/drm_damage_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
> @@ -100,11 +97,6 @@ struct ingenic_drm {
>  	struct notifier_block clock_nb;
>  };
>  
> -static bool ingenic_drm_cached_gem_buf;
> -module_param_named(cached_gem_buffers, ingenic_drm_cached_gem_buf, bool, 0400);
> -MODULE_PARM_DESC(cached_gem_buffers,
> -		 "Enable fully cached GEM buffers [default=false]");
> -
>  static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -402,8 +394,6 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>  	     plane->state->fb->format->format != state->fb->format->format))
>  		crtc_state->mode_changed = true;
>  
> -	drm_atomic_helper_check_plane_damage(state->state, state);
> -
>  	return 0;
>  }
>  
> @@ -521,38 +511,6 @@ void ingenic_drm_plane_config(struct device *dev,
>  	}
>  }
>  
> -void ingenic_drm_sync_data(struct device *dev,
> -			   struct drm_plane_state *old_state,
> -			   struct drm_plane_state *state)
> -{
> -	const struct drm_format_info *finfo = state->fb->format;
> -	struct ingenic_drm *priv = dev_get_drvdata(dev);
> -	struct drm_atomic_helper_damage_iter iter;
> -	unsigned int offset, i;
> -	struct drm_rect clip;
> -	dma_addr_t paddr;
> -	void *addr;
> -
> -	if (!ingenic_drm_cached_gem_buf)
> -		return;
> -
> -	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
> -
> -	drm_atomic_for_each_plane_damage(&iter, &clip) {
> -		for (i = 0; i < finfo->num_planes; i++) {
> -			paddr = drm_fb_cma_get_gem_addr(state->fb, state, i);
> -			addr = phys_to_virt(paddr);
> -
> -			/* Ignore x1/x2 values, invalidate complete lines */
> -			offset = clip.y1 * state->fb->pitches[i];
> -
> -			dma_cache_sync(priv->dev, addr + offset,
> -				       (clip.y2 - clip.y1) * state->fb->pitches[i],
> -				       DMA_TO_DEVICE);
> -		}
> -	}
> -}
> -
>  static void ingenic_drm_update_palette(struct ingenic_drm *priv,
>  				       const struct drm_color_lut *lut)
>  {
> @@ -581,8 +539,6 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  	if (state && state->fb) {
>  		crtc_state = state->crtc->state;
>  
> -		ingenic_drm_sync_data(priv->dev, oldstate, state);
> -
>  		addr = drm_fb_cma_get_gem_addr(state->fb, state, 0);
>  		width = state->src_w >> 16;
>  		height = state->src_h >> 16;
> @@ -752,69 +708,7 @@ static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
>  	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, 0);
>  }
>  
> -static struct drm_framebuffer *
> -ingenic_drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
> -			  const struct drm_mode_fb_cmd2 *mode_cmd)
> -{
> -	if (ingenic_drm_cached_gem_buf)
> -		return drm_gem_fb_create_with_dirty(dev, file, mode_cmd);
> -
> -	return drm_gem_fb_create(dev, file, mode_cmd);
> -}
> -
> -static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
> -				struct vm_area_struct *vma)
> -{
> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> -	struct device *dev = cma_obj->base.dev->dev;
> -	unsigned long attrs;
> -	int ret;
> -
> -	if (ingenic_drm_cached_gem_buf)
> -		attrs = DMA_ATTR_NON_CONSISTENT;
> -	else
> -		attrs = DMA_ATTR_WRITE_COMBINE;
> -
> -	/*
> -	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> -	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> -	 * the whole buffer.
> -	 */
> -	vma->vm_flags &= ~VM_PFNMAP;
> -	vma->vm_pgoff = 0;
> -	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> -
> -	ret = dma_mmap_attrs(dev, vma, cma_obj->vaddr, cma_obj->paddr,
> -			     vma->vm_end - vma->vm_start, attrs);
> -	if (ret)
> -		drm_gem_vm_close(vma);
> -
> -	return ret;
> -}
> -
> -static int ingenic_drm_gem_cma_mmap(struct file *filp,
> -				    struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret)
> -		return ret;
> -
> -	return ingenic_drm_gem_mmap(vma->vm_private_data, vma);
> -}
> -
> -static const struct file_operations ingenic_drm_fops = {
> -	.owner		= THIS_MODULE,
> -	.open		= drm_open,
> -	.release	= drm_release,
> -	.unlocked_ioctl	= drm_ioctl,
> -	.compat_ioctl	= drm_compat_ioctl,
> -	.poll		= drm_poll,
> -	.read		= drm_read,
> -	.llseek		= noop_llseek,
> -	.mmap		= ingenic_drm_gem_cma_mmap,
> -};
> +DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
>  
>  static struct drm_driver ingenic_drm_driver_data = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> @@ -878,7 +772,7 @@ static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs =
>  };
>  
>  static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> -	.fb_create		= ingenic_drm_gem_fb_create,
> +	.fb_create		= drm_gem_fb_create,
>  	.output_poll_changed	= drm_fb_helper_output_poll_changed,
>  	.atomic_check		= drm_atomic_helper_check,
>  	.atomic_commit		= drm_atomic_helper_commit,
> @@ -1032,8 +926,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  		return ret;
>  	}
>  
> -	drm_plane_enable_fb_damage_clips(&priv->f1);
> -
>  	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>  
>  	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
> @@ -1062,8 +954,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  			return ret;
>  		}
>  
> -		drm_plane_enable_fb_damage_clips(&priv->f0);
> -
>  		if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU) && has_components) {
>  			ret = component_bind_all(dev, drm);
>  			if (ret) {
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
> index ee3a892c0383..9b48ce02803d 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
> @@ -171,10 +171,6 @@ void ingenic_drm_plane_config(struct device *dev,
>  			      struct drm_plane *plane, u32 fourcc);
>  void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane);
>  
> -void ingenic_drm_sync_data(struct device *dev,
> -			   struct drm_plane_state *old_state,
> -			   struct drm_plane_state *state);
> -
>  extern struct platform_driver *ingenic_ipu_driver_ptr;
>  
>  #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
> diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> index 38c83e8cc6a5..fc8c6e970ee3 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> @@ -20,7 +20,6 @@
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> -#include <drm/drm_damage_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_fourcc.h>
> @@ -317,8 +316,6 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
>  				JZ_IPU_CTRL_CHIP_EN | JZ_IPU_CTRL_LCDC_SEL);
>  	}
>  
> -	ingenic_drm_sync_data(ipu->master, oldstate, state);
> -
>  	/* New addresses will be committed in vblank handler... */
>  	ipu->addr_y = drm_fb_cma_get_gem_addr(state->fb, state, 0);
>  	if (finfo->num_planes > 1)
> @@ -537,7 +534,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>  
>  	if (!state->crtc ||
>  	    !crtc_state->mode.hdisplay || !crtc_state->mode.vdisplay)
> -		goto out_check_damage;
> +		return 0;
>  
>  	/* Plane must be fully visible */
>  	if (state->crtc_x < 0 || state->crtc_y < 0 ||
> @@ -554,7 +551,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>  		return -EINVAL;
>  
>  	if (!osd_changed(state, plane->state))
> -		goto out_check_damage;
> +		return 0;
>  
>  	crtc_state->mode_changed = true;
>  
> @@ -581,9 +578,6 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>  	ipu->denom_w = denom_w;
>  	ipu->denom_h = denom_h;
>  
> -out_check_damage:
> -	drm_atomic_helper_check_plane_damage(state->state, state);
> -
>  	return 0;
>  }
>  
> @@ -765,8 +759,6 @@ static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
>  		return err;
>  	}
>  
> -	drm_plane_enable_fb_damage_clips(plane);
> -
>  	/*
>  	 * Sharpness settings range is [0,32]
>  	 * 0       : nearest-neighbor
> -- 
> 2.28.0

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Paul Cercueil]

Hi,

Le dim. 4 oct. 2020 à 21:59, Sam Ravnborg <sam@ravnborg.org> a écrit :
> Hi Paul.
> 
> On Sun, Oct 04, 2020 at 04:17:58PM +0200, Paul Cercueil wrote:
>>  This reverts commit 37054fc81443cc6a8c3a38395f384412b8373d82.
> 
> In the changelog please refer to commits like this:
> 37054fc81443 ("gpu/drm: ingenic: Add option to mmap GEM buffers 
> cached")
> 
> Use "dim cite 37054fc81443cc6a8c3a38395f384412b8373d82" to get the 
> right format.
> 
>> 
>>  At the very moment this commit was created, the DMA API it relied 
>> on was
>>  modified in the DMA tree, which caused the driver to break in
>>  linux-next.
>> 
>>  Revert it for now, and it will be resubmitted later to work with 
>> the new
>>  DMA API.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> With the changelog updated:
> Acked-by: Sam Ravnborg <sam@ravnborg.org>

Pushed to drm-misc-next with the changelog fix, thanks.

Stephen:
Now it should build fine again. Could you remove the BROKEN flag?

Cheers,
-Paul

>>  ---
>>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 114 
>> +---------------------
>>   drivers/gpu/drm/ingenic/ingenic-drm.h     |   4 -
>>   drivers/gpu/drm/ingenic/ingenic-ipu.c     |  12 +--
>>   3 files changed, 4 insertions(+), 126 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index 0225dc1f5eb8..7d8b0ad52979 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -9,8 +9,6 @@
>>   #include <linux/component.h>
>>   #include <linux/clk.h>
>>   #include <linux/dma-mapping.h>
>>  -#include <linux/dma-noncoherent.h>
>>  -#include <linux/io.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>>   #include <linux/of_device.h>
>>  @@ -24,7 +22,6 @@
>>   #include <drm/drm_color_mgmt.h>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_crtc_helper.h>
>>  -#include <drm/drm_damage_helper.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_gem_cma_helper.h>
>>   #include <drm/drm_fb_cma_helper.h>
>>  @@ -100,11 +97,6 @@ struct ingenic_drm {
>>   	struct notifier_block clock_nb;
>>   };
>> 
>>  -static bool ingenic_drm_cached_gem_buf;
>>  -module_param_named(cached_gem_buffers, ingenic_drm_cached_gem_buf, 
>> bool, 0400);
>>  -MODULE_PARM_DESC(cached_gem_buffers,
>>  -		 "Enable fully cached GEM buffers [default=false]");
>>  -
>>   static bool ingenic_drm_writeable_reg(struct device *dev, unsigned 
>> int reg)
>>   {
>>   	switch (reg) {
>>  @@ -402,8 +394,6 @@ static int 
>> ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>   	     plane->state->fb->format->format != 
>> state->fb->format->format))
>>   		crtc_state->mode_changed = true;
>> 
>>  -	drm_atomic_helper_check_plane_damage(state->state, state);
>>  -
>>   	return 0;
>>   }
>> 
>>  @@ -521,38 +511,6 @@ void ingenic_drm_plane_config(struct device 
>> *dev,
>>   	}
>>   }
>> 
>>  -void ingenic_drm_sync_data(struct device *dev,
>>  -			   struct drm_plane_state *old_state,
>>  -			   struct drm_plane_state *state)
>>  -{
>>  -	const struct drm_format_info *finfo = state->fb->format;
>>  -	struct ingenic_drm *priv = dev_get_drvdata(dev);
>>  -	struct drm_atomic_helper_damage_iter iter;
>>  -	unsigned int offset, i;
>>  -	struct drm_rect clip;
>>  -	dma_addr_t paddr;
>>  -	void *addr;
>>  -
>>  -	if (!ingenic_drm_cached_gem_buf)
>>  -		return;
>>  -
>>  -	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>>  -
>>  -	drm_atomic_for_each_plane_damage(&iter, &clip) {
>>  -		for (i = 0; i < finfo->num_planes; i++) {
>>  -			paddr = drm_fb_cma_get_gem_addr(state->fb, state, i);
>>  -			addr = phys_to_virt(paddr);
>>  -
>>  -			/* Ignore x1/x2 values, invalidate complete lines */
>>  -			offset = clip.y1 * state->fb->pitches[i];
>>  -
>>  -			dma_cache_sync(priv->dev, addr + offset,
>>  -				       (clip.y2 - clip.y1) * state->fb->pitches[i],
>>  -				       DMA_TO_DEVICE);
>>  -		}
>>  -	}
>>  -}
>>  -
>>   static void ingenic_drm_update_palette(struct ingenic_drm *priv,
>>   				       const struct drm_color_lut *lut)
>>   {
>>  @@ -581,8 +539,6 @@ static void 
>> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>   	if (state && state->fb) {
>>   		crtc_state = state->crtc->state;
>> 
>>  -		ingenic_drm_sync_data(priv->dev, oldstate, state);
>>  -
>>   		addr = drm_fb_cma_get_gem_addr(state->fb, state, 0);
>>   		width = state->src_w >> 16;
>>   		height = state->src_h >> 16;
>>  @@ -752,69 +708,7 @@ static void ingenic_drm_disable_vblank(struct 
>> drm_crtc *crtc)
>>   	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, 
>> JZ_LCD_CTRL_EOF_IRQ, 0);
>>   }
>> 
>>  -static struct drm_framebuffer *
>>  -ingenic_drm_gem_fb_create(struct drm_device *dev, struct drm_file 
>> *file,
>>  -			  const struct drm_mode_fb_cmd2 *mode_cmd)
>>  -{
>>  -	if (ingenic_drm_cached_gem_buf)
>>  -		return drm_gem_fb_create_with_dirty(dev, file, mode_cmd);
>>  -
>>  -	return drm_gem_fb_create(dev, file, mode_cmd);
>>  -}
>>  -
>>  -static int ingenic_drm_gem_mmap(struct drm_gem_object *obj,
>>  -				struct vm_area_struct *vma)
>>  -{
>>  -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>>  -	struct device *dev = cma_obj->base.dev->dev;
>>  -	unsigned long attrs;
>>  -	int ret;
>>  -
>>  -	if (ingenic_drm_cached_gem_buf)
>>  -		attrs = DMA_ATTR_NON_CONSISTENT;
>>  -	else
>>  -		attrs = DMA_ATTR_WRITE_COMBINE;
>>  -
>>  -	/*
>>  -	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and 
>> set the
>>  -	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want 
>> to map
>>  -	 * the whole buffer.
>>  -	 */
>>  -	vma->vm_flags &= ~VM_PFNMAP;
>>  -	vma->vm_pgoff = 0;
>>  -	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>>  -
>>  -	ret = dma_mmap_attrs(dev, vma, cma_obj->vaddr, cma_obj->paddr,
>>  -			     vma->vm_end - vma->vm_start, attrs);
>>  -	if (ret)
>>  -		drm_gem_vm_close(vma);
>>  -
>>  -	return ret;
>>  -}
>>  -
>>  -static int ingenic_drm_gem_cma_mmap(struct file *filp,
>>  -				    struct vm_area_struct *vma)
>>  -{
>>  -	int ret;
>>  -
>>  -	ret = drm_gem_mmap(filp, vma);
>>  -	if (ret)
>>  -		return ret;
>>  -
>>  -	return ingenic_drm_gem_mmap(vma->vm_private_data, vma);
>>  -}
>>  -
>>  -static const struct file_operations ingenic_drm_fops = {
>>  -	.owner		= THIS_MODULE,
>>  -	.open		= drm_open,
>>  -	.release	= drm_release,
>>  -	.unlocked_ioctl	= drm_ioctl,
>>  -	.compat_ioctl	= drm_compat_ioctl,
>>  -	.poll		= drm_poll,
>>  -	.read		= drm_read,
>>  -	.llseek		= noop_llseek,
>>  -	.mmap		= ingenic_drm_gem_cma_mmap,
>>  -};
>>  +DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
>> 
>>   static struct drm_driver ingenic_drm_driver_data = {
>>   	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>>  @@ -878,7 +772,7 @@ static const struct drm_encoder_helper_funcs 
>> ingenic_drm_encoder_helper_funcs =
>>   };
>> 
>>   static const struct drm_mode_config_funcs 
>> ingenic_drm_mode_config_funcs = {
>>  -	.fb_create		= ingenic_drm_gem_fb_create,
>>  +	.fb_create		= drm_gem_fb_create,
>>   	.output_poll_changed	= drm_fb_helper_output_poll_changed,
>>   	.atomic_check		= drm_atomic_helper_check,
>>   	.atomic_commit		= drm_atomic_helper_commit,
>>  @@ -1032,8 +926,6 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>   		return ret;
>>   	}
>> 
>>  -	drm_plane_enable_fb_damage_clips(&priv->f1);
>>  -
>>   	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>> 
>>   	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>  @@ -1062,8 +954,6 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>   			return ret;
>>   		}
>> 
>>  -		drm_plane_enable_fb_damage_clips(&priv->f0);
>>  -
>>   		if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU) && has_components) {
>>   			ret = component_bind_all(dev, drm);
>>   			if (ret) {
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h 
>> b/drivers/gpu/drm/ingenic/ingenic-drm.h
>>  index ee3a892c0383..9b48ce02803d 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
>>  @@ -171,10 +171,6 @@ void ingenic_drm_plane_config(struct device 
>> *dev,
>>   			      struct drm_plane *plane, u32 fourcc);
>>   void ingenic_drm_plane_disable(struct device *dev, struct 
>> drm_plane *plane);
>> 
>>  -void ingenic_drm_sync_data(struct device *dev,
>>  -			   struct drm_plane_state *old_state,
>>  -			   struct drm_plane_state *state);
>>  -
>>   extern struct platform_driver *ingenic_ipu_driver_ptr;
>> 
>>   #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c 
>> b/drivers/gpu/drm/ingenic/ingenic-ipu.c
>>  index 38c83e8cc6a5..fc8c6e970ee3 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
>>  @@ -20,7 +20,6 @@
>> 
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>>  -#include <drm/drm_damage_helper.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fb_cma_helper.h>
>>   #include <drm/drm_fourcc.h>
>>  @@ -317,8 +316,6 @@ static void 
>> ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
>>   				JZ_IPU_CTRL_CHIP_EN | JZ_IPU_CTRL_LCDC_SEL);
>>   	}
>> 
>>  -	ingenic_drm_sync_data(ipu->master, oldstate, state);
>>  -
>>   	/* New addresses will be committed in vblank handler... */
>>   	ipu->addr_y = drm_fb_cma_get_gem_addr(state->fb, state, 0);
>>   	if (finfo->num_planes > 1)
>>  @@ -537,7 +534,7 @@ static int 
>> ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>> 
>>   	if (!state->crtc ||
>>   	    !crtc_state->mode.hdisplay || !crtc_state->mode.vdisplay)
>>  -		goto out_check_damage;
>>  +		return 0;
>> 
>>   	/* Plane must be fully visible */
>>   	if (state->crtc_x < 0 || state->crtc_y < 0 ||
>>  @@ -554,7 +551,7 @@ static int 
>> ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>>   		return -EINVAL;
>> 
>>   	if (!osd_changed(state, plane->state))
>>  -		goto out_check_damage;
>>  +		return 0;
>> 
>>   	crtc_state->mode_changed = true;
>> 
>>  @@ -581,9 +578,6 @@ static int 
>> ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>>   	ipu->denom_w = denom_w;
>>   	ipu->denom_h = denom_h;
>> 
>>  -out_check_damage:
>>  -	drm_atomic_helper_check_plane_damage(state->state, state);
>>  -
>>   	return 0;
>>   }
>> 
>>  @@ -765,8 +759,6 @@ static int ingenic_ipu_bind(struct device *dev, 
>> struct device *master, void *d)
>>   		return err;
>>   	}
>> 
>>  -	drm_plane_enable_fb_damage_clips(plane);
>>  -
>>   	/*
>>   	 * Sharpness settings range is [0,32]
>>   	 * 0       : nearest-neighbor
>>  --
>>  2.28.0

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

Hi Paul,

On Sun, 04 Oct 2020 22:11:23 +0200 Paul Cercueil <paul@crapouillou.net> wrote:
>
> Pushed to drm-misc-next with the changelog fix, thanks.
> 
> Stephen:
> Now it should build fine again. Could you remove the BROKEN flag?

Thanks for letting me know, but the fix has not appeared in any drm
tree included in linux-next yet ...

If it doesn't show up by the time I will merge the drm tree tomorrow, I
will apply this revert patch myself (instead of the patch marking the
driver BROKEN).
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Daniel Vetter]

On Mon, Oct 05, 2020 at 11:01:50PM +1100, Stephen Rothwell wrote:
> Hi Paul,
> 
> On Sun, 04 Oct 2020 22:11:23 +0200 Paul Cercueil <paul@crapouillou.net> wrote:
> >
> > Pushed to drm-misc-next with the changelog fix, thanks.
> > 
> > Stephen:
> > Now it should build fine again. Could you remove the BROKEN flag?
> 
> Thanks for letting me know, but the fix has not appeared in any drm
> tree included in linux-next yet ...
> 
> If it doesn't show up by the time I will merge the drm tree tomorrow, I
> will apply this revert patch myself (instead of the patch marking the
> driver BROKEN).

Yeah it should have been pushed to drm-misc-next-fixes per

https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#where-do-i-apply-my-patch

Paul, can you pls git cherry-pick -x this over to drm-misc-next-fixes?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Paul Cercueil]

Hi,

Le lun. 5 oct. 2020 à 16:05, Daniel Vetter <daniel@ffwll.ch> a écrit :
> On Mon, Oct 05, 2020 at 11:01:50PM +1100, Stephen Rothwell wrote:
>>  Hi Paul,
>> 
>>  On Sun, 04 Oct 2020 22:11:23 +0200 Paul Cercueil 
>> <paul@crapouillou.net> wrote:
>>  >
>>  > Pushed to drm-misc-next with the changelog fix, thanks.
>>  >
>>  > Stephen:
>>  > Now it should build fine again. Could you remove the BROKEN flag?
>> 
>>  Thanks for letting me know, but the fix has not appeared in any drm
>>  tree included in linux-next yet ...
>> 
>>  If it doesn't show up by the time I will merge the drm tree 
>> tomorrow, I
>>  will apply this revert patch myself (instead of the patch marking 
>> the
>>  driver BROKEN).
> 
> Yeah it should have been pushed to drm-misc-next-fixes per
> 
> https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#where-do-i-apply-my-patch
> 
> Paul, can you pls git cherry-pick -x this over to drm-misc-next-fixes?

I had a few commits on top of it in drm-misc-next, so the revert 
doesn't apply cleanly in drm-misc-next-fixes... I can revert it there, 
but then we'd have a different revert commit in drm-misc-next and 
drm-misc-next-next.

Sorry for the mess. What should I do?

-Paul

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Daniel Vetter]

On Mon, Oct 5, 2020 at 4:47 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi,
>
> Le lun. 5 oct. 2020 à 16:05, Daniel Vetter <daniel@ffwll.ch> a écrit :
> > On Mon, Oct 05, 2020 at 11:01:50PM +1100, Stephen Rothwell wrote:
> >>  Hi Paul,
> >>
> >>  On Sun, 04 Oct 2020 22:11:23 +0200 Paul Cercueil
> >> <paul@crapouillou.net> wrote:
> >>  >
> >>  > Pushed to drm-misc-next with the changelog fix, thanks.
> >>  >
> >>  > Stephen:
> >>  > Now it should build fine again. Could you remove the BROKEN flag?
> >>
> >>  Thanks for letting me know, but the fix has not appeared in any drm
> >>  tree included in linux-next yet ...
> >>
> >>  If it doesn't show up by the time I will merge the drm tree
> >> tomorrow, I
> >>  will apply this revert patch myself (instead of the patch marking
> >> the
> >>  driver BROKEN).
> >
> > Yeah it should have been pushed to drm-misc-next-fixes per
> >
> > https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#where-do-i-apply-my-patch
> >
> > Paul, can you pls git cherry-pick -x this over to drm-misc-next-fixes?
>
> I had a few commits on top of it in drm-misc-next, so the revert
> doesn't apply cleanly in drm-misc-next-fixes... I can revert it there,
> but then we'd have a different revert commit in drm-misc-next and
> drm-misc-next-next.
>
> Sorry for the mess. What should I do?

We need the revert in drm-misc-next-fixes or the drm-next pull request
doesn't work out. So cherry-pick over, fix up conflicts, push the
tree, and don't forget to fix up the conflicts when dim rebuilds
drm-tip. Also tell drm-misc maintainers what you've done, they
probably want to do a backmerge to clean this up a bit once the
drm-next pull request has landed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Daniel Vetter]

On Mon, Oct 5, 2020 at 4:47 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi,
>
> Le lun. 5 oct. 2020 à 16:05, Daniel Vetter <daniel@ffwll.ch> a écrit :
> > On Mon, Oct 05, 2020 at 11:01:50PM +1100, Stephen Rothwell wrote:
> >>  Hi Paul,
> >>
> >>  On Sun, 04 Oct 2020 22:11:23 +0200 Paul Cercueil
> >> <paul@crapouillou.net> wrote:
> >>  >
> >>  > Pushed to drm-misc-next with the changelog fix, thanks.
> >>  >
> >>  > Stephen:
> >>  > Now it should build fine again. Could you remove the BROKEN flag?
> >>
> >>  Thanks for letting me know, but the fix has not appeared in any drm
> >>  tree included in linux-next yet ...
> >>
> >>  If it doesn't show up by the time I will merge the drm tree
> >> tomorrow, I
> >>  will apply this revert patch myself (instead of the patch marking
> >> the
> >>  driver BROKEN).
> >
> > Yeah it should have been pushed to drm-misc-next-fixes per
> >
> > https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#where-do-i-apply-my-patch
> >
> > Paul, can you pls git cherry-pick -x this over to drm-misc-next-fixes?
>
> I had a few commits on top of it in drm-misc-next, so the revert
> doesn't apply cleanly in drm-misc-next-fixes... I can revert it there,
> but then we'd have a different revert commit in drm-misc-next and
> drm-misc-next-next.
>
> Sorry for the mess. What should I do?

Hm not sure why, but the reply I thought I've typed didn't seem to
have gone out.

Cherry pick up, fix up conflict and then fix up the conflict when
rebuilding drm-tip. Please tell drm-misc maintainers, they probably
want to do a backmerge once the drm-next merge window pull is merged
in Linus tree.

If we don't fix this up then the drm-next pull goes nowhere.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] Revert "gpu/drm: ingenic: Add option to mmap GEM buffers cached"

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

Hi all,

On Mon, 5 Oct 2020 23:01:50 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Sun, 04 Oct 2020 22:11:23 +0200 Paul Cercueil <paul@crapouillou.net> wrote:
> >
> > Pushed to drm-misc-next with the changelog fix, thanks.
> > 
> > Stephen:
> > Now it should build fine again. Could you remove the BROKEN flag?  
> 
> Thanks for letting me know, but the fix has not appeared in any drm
> tree included in linux-next yet ...
> 
> If it doesn't show up by the time I will merge the drm tree tomorrow, I
> will apply this revert patch myself (instead of the patch marking the
> driver BROKEN).

Unfortunately, the revert patch does not apply to the drm tree merge,
so I have just marked the driver BROKEN again today.
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]