[PATCH 0/3] ExynosDRM - rework GEM internals

[PATCH 0/3] ExynosDRM - rework GEM internals

[Marek Szyprowski]

Hi

The recent discussion under the 'drm/prime: fix extracting of the DMA
addresses from a scatterlist' [1] patch inspired me to take a look again
into the Exynos DRM GEM internals. I've made a little cleanup and
reworked some parts to make them more error proof for the various
corner-cases.

[1] https://patchwork.freedesktop.org/patch/359081/ patch 

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Patch summary:

Marek Szyprowski (3):
  drm/exynos: gem: Remove dead-code
  drm/exynos: gem: Rework scatter-list contiguity check on Prime import
  drm/exynos: gem: Get rid of the internal 'pages' array

 drivers/gpu/drm/exynos/exynos_drm_drv.c   |   1 -
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  28 +---
 drivers/gpu/drm/exynos/exynos_drm_gem.c   | 178 ++++++++--------------
 drivers/gpu/drm/exynos/exynos_drm_gem.h   |  16 +-
 4 files changed, 66 insertions(+), 157 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/exynos: gem: Remove dead-code

[Marek Szyprowski]

The ExynosDRM page fault handler is never used, drm_gem_mmap()
always calls exynos_drm_gem_mmap() function, which perform
complete mapping for the given virtual address-space area.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  1 -
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 20 --------------------
 drivers/gpu/drm/exynos/exynos_drm_gem.h |  3 ---
 3 files changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 57defeb44522..dbd80f1e4c78 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -76,7 +76,6 @@ static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
 }
 
 static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
-	.fault = exynos_drm_gem_fault,
 	.open = drm_gem_vm_open,
 	.close = drm_gem_vm_close,
 };
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index d734d9d51762..40514d3dcf60 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -381,26 +381,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
 	return 0;
 }
 
-vm_fault_t exynos_drm_gem_fault(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct drm_gem_object *obj = vma->vm_private_data;
-	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
-	unsigned long pfn;
-	pgoff_t page_offset;
-
-	page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
-
-	if (page_offset >= (exynos_gem->size >> PAGE_SHIFT)) {
-		DRM_ERROR("invalid page offset\n");
-		return VM_FAULT_SIGBUS;
-	}
-
-	pfn = page_to_pfn(exynos_gem->pages[page_offset]);
-	return vmf_insert_mixed(vma, vmf->address,
-			__pfn_to_pfn_t(pfn, PFN_DEV));
-}
-
 static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
 				   struct vm_area_struct *vma)
 {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
index 42ec67bc262d..f00044c0b688 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
@@ -101,9 +101,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
 			       struct drm_device *dev,
 			       struct drm_mode_create_dumb *args);
 
-/* page fault handler and mmap fault address(virtual) to physical memory. */
-vm_fault_t exynos_drm_gem_fault(struct vm_fault *vmf);
-
 /* set vm_flags and we can change the vm attribute to other one at here. */
 int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/exynos: gem: Remove dead-code

[Inki Dae]

20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
> The ExynosDRM page fault handler is never used, drm_gem_mmap()
> always calls exynos_drm_gem_mmap() function, which perform
> complete mapping for the given virtual address-space area.

Right, never used. Picked it up.

Thanks,
Inki Dae

> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |  1 -
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 20 --------------------
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |  3 ---
>  3 files changed, 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 57defeb44522..dbd80f1e4c78 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -76,7 +76,6 @@ static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
>  }
>  
>  static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
> -	.fault = exynos_drm_gem_fault,
>  	.open = drm_gem_vm_open,
>  	.close = drm_gem_vm_close,
>  };
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index d734d9d51762..40514d3dcf60 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -381,26 +381,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  	return 0;
>  }
>  
> -vm_fault_t exynos_drm_gem_fault(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct drm_gem_object *obj = vma->vm_private_data;
> -	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
> -	unsigned long pfn;
> -	pgoff_t page_offset;
> -
> -	page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
> -
> -	if (page_offset >= (exynos_gem->size >> PAGE_SHIFT)) {
> -		DRM_ERROR("invalid page offset\n");
> -		return VM_FAULT_SIGBUS;
> -	}
> -
> -	pfn = page_to_pfn(exynos_gem->pages[page_offset]);
> -	return vmf_insert_mixed(vma, vmf->address,
> -			__pfn_to_pfn_t(pfn, PFN_DEV));
> -}
> -
>  static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>  				   struct vm_area_struct *vma)
>  {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index 42ec67bc262d..f00044c0b688 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -101,9 +101,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  			       struct drm_device *dev,
>  			       struct drm_mode_create_dumb *args);
>  
> -/* page fault handler and mmap fault address(virtual) to physical memory. */
> -vm_fault_t exynos_drm_gem_fault(struct vm_fault *vmf);
> -
>  /* set vm_flags and we can change the vm attribute to other one at here. */
>  int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Marek Szyprowski]

Explicitly check if the imported buffer has been mapped as contiguous in
the DMA address space, what is required by all Exynos DRM CRTC drivers.
While touching this, set buffer flags depending on the availability of
the IOMMU.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 40514d3dcf60..9d4e4d321bda 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 	int npages;
 	int ret;
 
+	if (sgt->nents != 1) {
+		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
+		struct scatterlist *s;
+		unsigned int i;
+
+		for_each_sg(sgt->sgl, s, sgt->nents, i) {
+			if (!sg_dma_len(s))
+				continue;
+			if (sg_dma_address(s) != next_addr) {
+				DRM_ERROR("buffer chunks must be mapped contiguously");
+				return ERR_PTR(-EINVAL);
+			}
+			next_addr = sg_dma_address(s) + sg_dma_len(s);
+		}
+		return ERR_PTR(-EINVAL);
+	}
+
 	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
 	if (IS_ERR(exynos_gem)) {
 		ret = PTR_ERR(exynos_gem);
@@ -480,18 +497,15 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 
 	exynos_gem->sgt = sgt;
 
-	if (sgt->nents == 1) {
-		/* always physically continuous memory if sgt->nents is 1. */
-		exynos_gem->flags |= EXYNOS_BO_CONTIG;
-	} else {
-		/*
-		 * this case could be CONTIG or NONCONTIG type but for now
-		 * sets NONCONTIG.
-		 * TODO. we have to find a way that exporter can notify
-		 * the type of its own buffer to importer.
-		 */
+	/*
+	 * Buffer has been mapped as contiguous into DMA address space,
+	 * but if there is IOMMU, it can be either CONTIG or NONCONTIG.
+	 * We assume a simplified logic below:
+	 */
+	if (is_drm_iommu_supported(dev))
 		exynos_gem->flags |= EXYNOS_BO_NONCONTIG;
-	}
+	else
+		exynos_gem->flags |= EXYNOS_BO_CONTIG;
 
 	return &exynos_gem->base;
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Inki Dae]

20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
> Explicitly check if the imported buffer has been mapped as contiguous in
> the DMA address space, what is required by all Exynos DRM CRTC drivers.
> While touching this, set buffer flags depending on the availability of
> the IOMMU.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 40514d3dcf60..9d4e4d321bda 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  	int npages;
>  	int ret;
>  

(Optional) could you leave one comment here?
i.e., /* Check if DMA memory region from a exporter are mapped contiguously or not. */ 

> +	if (sgt->nents != 1) {

How about using below condition instead?
if (sgt->nents > 0) {

> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
> +		struct scatterlist *s;
> +		unsigned int i;
> +
> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
> +			if (!sg_dma_len(s))
> +				continue;

Isn't it an error case if sg_dma_len(s) is 0? I think length of s is 0 then it would be invalid because all entries of sgt should be mapped before sg_dma_len() is called.

Thanks,
Inki Dae

> +			if (sg_dma_address(s) != next_addr) {
> +				DRM_ERROR("buffer chunks must be mapped contiguously");
> +				return ERR_PTR(-EINVAL);
> +			}
> +			next_addr = sg_dma_address(s) + sg_dma_len(s);
> +		}
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
>  	if (IS_ERR(exynos_gem)) {
>  		ret = PTR_ERR(exynos_gem);
> @@ -480,18 +497,15 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  
>  	exynos_gem->sgt = sgt;
>  
> -	if (sgt->nents == 1) {
> -		/* always physically continuous memory if sgt->nents is 1. */
> -		exynos_gem->flags |= EXYNOS_BO_CONTIG;
> -	} else {
> -		/*
> -		 * this case could be CONTIG or NONCONTIG type but for now
> -		 * sets NONCONTIG.
> -		 * TODO. we have to find a way that exporter can notify
> -		 * the type of its own buffer to importer.
> -		 */
> +	/*
> +	 * Buffer has been mapped as contiguous into DMA address space,
> +	 * but if there is IOMMU, it can be either CONTIG or NONCONTIG.
> +	 * We assume a simplified logic below:
> +	 */
> +	if (is_drm_iommu_supported(dev))
>  		exynos_gem->flags |= EXYNOS_BO_NONCONTIG;
> -	}
> +	else
> +		exynos_gem->flags |= EXYNOS_BO_CONTIG;
>  
>  	return &exynos_gem->base;
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Marek Szyprowski]

Hi Inki,

On 21.04.2020 09:38, Inki Dae wrote:
> 20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
>> Explicitly check if the imported buffer has been mapped as contiguous in
>> the DMA address space, what is required by all Exynos DRM CRTC drivers.
>> While touching this, set buffer flags depending on the availability of
>> the IOMMU.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> index 40514d3dcf60..9d4e4d321bda 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> @@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>   	int npages;
>>   	int ret;
>>   
> (Optional) could you leave one comment here?
> i.e., /* Check if DMA memory region from a exporter are mapped contiguously or not. */

Okay.

>> +	if (sgt->nents != 1) {
> How about using below condition instead?
> if (sgt->nents > 0) {

This is not the same. My check for != 1 is intended. Checking contiguity 
of the scatterlist if it has only 1 element doesn't have much sense.

>> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
>> +		struct scatterlist *s;
>> +		unsigned int i;
>> +
>> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
>> +			if (!sg_dma_len(s))
>> +				continue;
> Isn't it an error case if sg_dma_len(s) is 0? I think length of s is 0 then it would be invalid because all entries of sgt should be mapped before sg_dma_len() is called.

Well, it is a grey area. Some code incorrectly sets nents as orig_nents, 
thus this version is simply safe for both approaches. sg entries unused 
for DMA chunks have sg_dma_len() == 0.

The above check is more or less copied from 
drm_gem_cma_prime_import_sg_table() (drivers/gpu/drm/drm_gem_cma_helper.c).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Inki Dae]

Hi Marek,

20. 4. 21. 오후 5:09에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 21.04.2020 09:38, Inki Dae wrote:
>> 20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
>>> Explicitly check if the imported buffer has been mapped as contiguous in
>>> the DMA address space, what is required by all Exynos DRM CRTC drivers.
>>> While touching this, set buffer flags depending on the availability of
>>> the IOMMU.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
>>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> index 40514d3dcf60..9d4e4d321bda 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> @@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>>   	int npages;
>>>   	int ret;
>>>   
>> (Optional) could you leave one comment here?
>> i.e., /* Check if DMA memory region from a exporter are mapped contiguously or not. */
> 
> Okay.
> 
>>> +	if (sgt->nents != 1) {
>> How about using below condition instead?
>> if (sgt->nents > 0) {
> 
> This is not the same. My check for != 1 is intended. Checking contiguity 
> of the scatterlist if it has only 1 element doesn't have much sense.

Oops sorry. My intention was 'if (sgt->nents > 1)' because if (sgt->nents != 1) allows
- sgt->nents < 1
- sgt->nents > 1

I think the checking would be valid only in case of having multiple entries - sgt->nents > 1.

Thanks,
Inki Dae

> 
>>> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
>>> +		struct scatterlist *s;
>>> +		unsigned int i;
>>> +
>>> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
>>> +			if (!sg_dma_len(s))
>>> +				continue;
>> Isn't it an error case if sg_dma_len(s) is 0? I think length of s is 0 then it would be invalid because all entries of sgt should be mapped before sg_dma_len() is called.
> 
> Well, it is a grey area. Some code incorrectly sets nents as orig_nents, 
> thus this version is simply safe for both approaches. sg entries unused 
> for DMA chunks have sg_dma_len() == 0.
> 
> The above check is more or less copied from 
> drm_gem_cma_prime_import_sg_table() (drivers/gpu/drm/drm_gem_cma_helper.c).
> 
> Best regards
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Inki Dae]

20. 4. 22. 오후 12:37에 Inki Dae 이(가) 쓴 글:
> Hi Marek,
> 
> 20. 4. 21. 오후 5:09에 Marek Szyprowski 이(가) 쓴 글:
>> Hi Inki,
>>
>> On 21.04.2020 09:38, Inki Dae wrote:
>>> 20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
>>>> Explicitly check if the imported buffer has been mapped as contiguous in
>>>> the DMA address space, what is required by all Exynos DRM CRTC drivers.
>>>> While touching this, set buffer flags depending on the availability of
>>>> the IOMMU.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>   drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
>>>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>> index 40514d3dcf60..9d4e4d321bda 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>> @@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>>>   	int npages;
>>>>   	int ret;
>>>>   
>>> (Optional) could you leave one comment here?
>>> i.e., /* Check if DMA memory region from a exporter are mapped contiguously or not. */
>>
>> Okay.
>>
>>>> +	if (sgt->nents != 1) {
>>> How about using below condition instead?
>>> if (sgt->nents > 0) {
>>
>> This is not the same. My check for != 1 is intended. Checking contiguity 
>> of the scatterlist if it has only 1 element doesn't have much sense.
> 
> Oops sorry. My intention was 'if (sgt->nents > 1)' because if (sgt->nents != 1) allows
> - sgt->nents < 1
> - sgt->nents > 1
> 
> I think the checking would be valid only in case of having multiple entries - sgt->nents > 1.
> 
> Thanks,
> Inki Dae
> 
>>
>>>> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
>>>> +		struct scatterlist *s;
>>>> +		unsigned int i;
>>>> +
>>>> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
>>>> +			if (!sg_dma_len(s))
>>>> +				continue;
>>> Isn't it an error case if sg_dma_len(s) is 0? I think length of s is 0 then it would be invalid because all entries of sgt should be mapped before sg_dma_len() is called.
>>
>> Well, it is a grey area. Some code incorrectly sets nents as orig_nents, 
>> thus this version is simply safe for both approaches. sg entries unused 
>> for DMA chunks have sg_dma_len() == 0.
>>
>> The above check is more or less copied from 
>> drm_gem_cma_prime_import_sg_table() (drivers/gpu/drm/drm_gem_cma_helper.c).

I looked into above original code but it seems that it allows sgt->nents to have negative value, 'sgt->nents < 1', which could incur some bugs.
So I think the original code can be fixed like below,
	if (sgt->nents > 1) {  // <- here
		/* check if the entries in the sg_table are contiguous */
		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
		struct scatterlist *s;
		unsigned int i;

		for_each_sg(sgt->sgl, s, sgt->nents, i) {
			/*
			 * sg_dma_address(s) is only valid for entries
			 * that have sg_dma_len(s) > 0 // <- here
			 */
			if (!sg_dma_len(s))
				continue;

			if (sg_dma_address(s) != next_addr)
				return ERR_PTR(-EINVAL);

			next_addr = sg_dma_address(s) + sg_dma_len(s);
		}
	}

So if you agree with me then we don't have to copy and paste this code as-is.

Regarding 'if (!sg_dma_len(s))' condition here, I'm not clear whether we are using it correctly because scatterlist.h header description says,
/*                                                                              
 * These macros should be used after a dma_map_sg call has been done            
 * to get bus addresses of each of the SG entries and their lengths.            
 * You should only work with the number of sg entries dma_map_sg                
 * returns, or alternatively stop on the first sg_dma_len(sg) which             
 * is 0.                                                                        
 */      

According to above description, sg_dma_len() should be called after dma_map_sg() call. Even it says to stop on the first sg_dma_len(sg) which is 0.
And we could avoid the checking function code from being duplicated by introducing one function which checks if the entries in the sg_table are contiguous or not as a separate patch later.

Thanks,
Inki Dae

>>
>> Best regards
>>
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Marek Szyprowski]

Hi Inki,

On 22.04.2020 06:36, Inki Dae wrote:
> 20. 4. 22. 오후 12:37에 Inki Dae 이(가) 쓴 글:
>> 20. 4. 21. 오후 5:09에 Marek Szyprowski 이(가) 쓴 글:
>>> On 21.04.2020 09:38, Inki Dae wrote:
>>>> 20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
>>>>> Explicitly check if the imported buffer has been mapped as contiguous in
>>>>> the DMA address space, what is required by all Exynos DRM CRTC drivers.
>>>>> While touching this, set buffer flags depending on the availability of
>>>>> the IOMMU.
>>>>>
>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> ---
>>>>>    drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
>>>>>    1 file changed, 25 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>>> index 40514d3dcf60..9d4e4d321bda 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>>> @@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>>>>    	int npages;
>>>>>    	int ret;
>>>>>    
>>>> (Optional) could you leave one comment here?
>>>> i.e., /* Check if DMA memory region from a exporter are mapped contiguously or not. */
>>> Okay.
>>>
>>>>> +	if (sgt->nents != 1) {
>>>> How about using below condition instead?
>>>> if (sgt->nents > 0) {
>>> This is not the same. My check for != 1 is intended. Checking contiguity
>>> of the scatterlist if it has only 1 element doesn't have much sense.
>> Oops sorry. My intention was 'if (sgt->nents > 1)' because if (sgt->nents != 1) allows
>> - sgt->nents < 1
>> - sgt->nents > 1
>>
>> I think the checking would be valid only in case of having multiple entries - sgt->nents > 1.

Okay, I can change the check to  sgt->nents > 1.

>>>>> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
>>>>> +		struct scatterlist *s;
>>>>> +		unsigned int i;
>>>>> +
>>>>> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
>>>>> +			if (!sg_dma_len(s))
>>>>> +				continue;
>>>> Isn't it an error case if sg_dma_len(s) is 0? I think length of s is 0 then it would be invalid because all entries of sgt should be mapped before sg_dma_len() is called.
>>> Well, it is a grey area. Some code incorrectly sets nents as orig_nents,
>>> thus this version is simply safe for both approaches. sg entries unused
>>> for DMA chunks have sg_dma_len() == 0.
>>>
>>> The above check is more or less copied from
>>> drm_gem_cma_prime_import_sg_table() (drivers/gpu/drm/drm_gem_cma_helper.c).
> I looked into above original code but it seems that it allows sgt->nents to have negative value, 'sgt->nents < 1', which could incur some bugs.

Okay, I will add a check for negative or zero nents.

> So I think the original code can be fixed like below,
> 	if (sgt->nents > 1) {  // <- here
> 		/* check if the entries in the sg_table are contiguous */
> 		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
> 		struct scatterlist *s;
> 		unsigned int i;
>
> 		for_each_sg(sgt->sgl, s, sgt->nents, i) {
> 			/*
> 			 * sg_dma_address(s) is only valid for entries
> 			 * that have sg_dma_len(s) > 0 // <- here
> 			 */
> 			if (!sg_dma_len(s))
> 				continue;
>
> 			if (sg_dma_address(s) != next_addr)
> 				return ERR_PTR(-EINVAL);
>
> 			next_addr = sg_dma_address(s) + sg_dma_len(s);
> 		}
> 	}
>
> So if you agree with me then we don't have to copy and paste this code as-is.
>
> Regarding 'if (!sg_dma_len(s))' condition here, I'm not clear whether we are using it correctly because scatterlist.h header description says,
> /*
>   * These macros should be used after a dma_map_sg call has been done
>   * to get bus addresses of each of the SG entries and their lengths.
>   * You should only work with the number of sg entries dma_map_sg
>   * returns, or alternatively stop on the first sg_dma_len(sg) which
>   * is 0.
>   */
>
> According to above description, sg_dma_len() should be called after dma_map_sg() call. Even it says to stop on the first sg_dma_len(sg) which is 0.
> And we could avoid the checking function code from being duplicated by introducing one function which checks if the entries in the sg_table are contiguous or not as a separate patch later.

Okay, I will rework this, then I will prepare a patch which will unify 
scatterlist contiguity checks for all DRM drivers.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/exynos: gem: rework scatter-list contiguity check on prime import

[Marek Szyprowski]

Explicitly check if the imported buffer has been mapped as contiguous in
the DMA address space, what is required by all Exynos DRM CRTC drivers.
While touching this, set buffer flags depending on the availability of
the IOMMU.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
v2:
- reworked a check for sgt->nents
- added a comment about a scatter-list contiguity check
- removed additional 'return ERR_PTR(-EINVAL)' accidentaly left after debugging
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 42 ++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 40514d3..f136efb 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -458,6 +458,29 @@ struct drm_gem_object *
 	int npages;
 	int ret;
 
+	if (sgt->nents < 1)
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * Check if the provided buffer has been mapped as contiguous
+	 * into DMA address space.
+	 */
+	if (sgt->nents > 1) {
+		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
+		struct scatterlist *s;
+		unsigned int i;
+
+		for_each_sg(sgt->sgl, s, sgt->nents, i) {
+			if (!sg_dma_len(s))
+				break;
+			if (sg_dma_address(s) != next_addr) {
+				DRM_ERROR("buffer chunks must be mapped contiguously");
+				return ERR_PTR(-EINVAL);
+			}
+			next_addr = sg_dma_address(s) + sg_dma_len(s);
+		}
+	}
+
 	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
 	if (IS_ERR(exynos_gem)) {
 		ret = PTR_ERR(exynos_gem);
@@ -480,18 +503,15 @@ struct drm_gem_object *
 
 	exynos_gem->sgt = sgt;
 
-	if (sgt->nents == 1) {
-		/* always physically continuous memory if sgt->nents is 1. */
-		exynos_gem->flags |= EXYNOS_BO_CONTIG;
-	} else {
-		/*
-		 * this case could be CONTIG or NONCONTIG type but for now
-		 * sets NONCONTIG.
-		 * TODO. we have to find a way that exporter can notify
-		 * the type of its own buffer to importer.
-		 */
+	/*
+	 * Buffer has been mapped as contiguous into DMA address space,
+	 * but if there is IOMMU, it can be either CONTIG or NONCONTIG.
+	 * We assume a simplified logic below:
+	 */
+	if (is_drm_iommu_supported(dev))
 		exynos_gem->flags |= EXYNOS_BO_NONCONTIG;
-	}
+	else
+		exynos_gem->flags |= EXYNOS_BO_CONTIG;
 
 	return &exynos_gem->base;
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/exynos: gem: rework scatter-list contiguity check on prime import

[Inki Dae]

20. 4. 22. 오후 8:40에 Marek Szyprowski 이(가) 쓴 글:
> Explicitly check if the imported buffer has been mapped as contiguous in
> the DMA address space, what is required by all Exynos DRM CRTC drivers.
> While touching this, set buffer flags depending on the availability of
> the IOMMU.

Picked it up.

Thanks,
Inki Dae

> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> v2:
> - reworked a check for sgt->nents
> - added a comment about a scatter-list contiguity check
> - removed additional 'return ERR_PTR(-EINVAL)' accidentaly left after debugging
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 42 ++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 40514d3..f136efb 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -458,6 +458,29 @@ struct drm_gem_object *
>  	int npages;
>  	int ret;
>  
> +	if (sgt->nents < 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	/*
> +	 * Check if the provided buffer has been mapped as contiguous
> +	 * into DMA address space.
> +	 */
> +	if (sgt->nents > 1) {
> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
> +		struct scatterlist *s;
> +		unsigned int i;
> +
> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
> +			if (!sg_dma_len(s))
> +				break;
> +			if (sg_dma_address(s) != next_addr) {
> +				DRM_ERROR("buffer chunks must be mapped contiguously");
> +				return ERR_PTR(-EINVAL);
> +			}
> +			next_addr = sg_dma_address(s) + sg_dma_len(s);
> +		}
> +	}
> +
>  	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
>  	if (IS_ERR(exynos_gem)) {
>  		ret = PTR_ERR(exynos_gem);
> @@ -480,18 +503,15 @@ struct drm_gem_object *
>  
>  	exynos_gem->sgt = sgt;
>  
> -	if (sgt->nents == 1) {
> -		/* always physically continuous memory if sgt->nents is 1. */
> -		exynos_gem->flags |= EXYNOS_BO_CONTIG;
> -	} else {
> -		/*
> -		 * this case could be CONTIG or NONCONTIG type but for now
> -		 * sets NONCONTIG.
> -		 * TODO. we have to find a way that exporter can notify
> -		 * the type of its own buffer to importer.
> -		 */
> +	/*
> +	 * Buffer has been mapped as contiguous into DMA address space,
> +	 * but if there is IOMMU, it can be either CONTIG or NONCONTIG.
> +	 * We assume a simplified logic below:
> +	 */
> +	if (is_drm_iommu_supported(dev))
>  		exynos_gem->flags |= EXYNOS_BO_NONCONTIG;
> -	}
> +	else
> +		exynos_gem->flags |= EXYNOS_BO_CONTIG;
>  
>  	return &exynos_gem->base;
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/exynos: gem: Get rid of the internal 'pages' array

[Marek Szyprowski]

Internal pages array and scatter-list for them is not really needed for
anything. FBDev emulation can simply rely on the DMA-mapping framework
to create a proper kernel mapping for the buffer, while all other buffer
use cases don't really need that array at all.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  28 +----
 drivers/gpu/drm/exynos/exynos_drm_gem.c   | 124 +++++++---------------
 drivers/gpu/drm/exynos/exynos_drm_gem.h   |  13 +--
 3 files changed, 42 insertions(+), 123 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index e6ceaf36fb04..56a2b47e1af7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -76,7 +76,6 @@ static int exynos_drm_fbdev_update(struct drm_fb_helper *helper,
 	struct fb_info *fbi;
 	struct drm_framebuffer *fb = helper->fb;
 	unsigned int size = fb->width * fb->height * fb->format->cpp[0];
-	unsigned int nr_pages;
 	unsigned long offset;
 
 	fbi = drm_fb_helper_alloc_fbi(helper);
@@ -90,16 +89,6 @@ static int exynos_drm_fbdev_update(struct drm_fb_helper *helper,
 
 	drm_fb_helper_fill_info(fbi, helper, sizes);
 
-	nr_pages = exynos_gem->size >> PAGE_SHIFT;
-
-	exynos_gem->kvaddr = (void __iomem *) vmap(exynos_gem->pages, nr_pages,
-				VM_MAP, pgprot_writecombine(PAGE_KERNEL));
-	if (!exynos_gem->kvaddr) {
-		DRM_DEV_ERROR(to_dma_dev(helper->dev),
-			      "failed to map pages to kernel space.\n");
-		return -EIO;
-	}
-
 	offset = fbi->var.xoffset * fb->format->cpp[0];
 	offset += fbi->var.yoffset * fb->pitches[0];
 
@@ -133,18 +122,7 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper,
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 
-	exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size);
-	/*
-	 * If physically contiguous memory allocation fails and if IOMMU is
-	 * supported then try to get buffer from non physically contiguous
-	 * memory area.
-	 */
-	if (IS_ERR(exynos_gem) && is_drm_iommu_supported(dev)) {
-		dev_warn(dev->dev, "contiguous FB allocation failed, falling back to non-contiguous\n");
-		exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_NONCONTIG,
-						   size);
-	}
-
+	exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_WC, size, true);
 	if (IS_ERR(exynos_gem))
 		return PTR_ERR(exynos_gem);
 
@@ -229,12 +207,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 static void exynos_drm_fbdev_destroy(struct drm_device *dev,
 				      struct drm_fb_helper *fb_helper)
 {
-	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(fb_helper);
-	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
 	struct drm_framebuffer *fb;
 
-	vunmap(exynos_gem->kvaddr);
-
 	/* release drm framebuffer and real buffer */
 	if (fb_helper->fb && fb_helper->fb->funcs) {
 		fb = fb_helper->fb;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 9d4e4d321bda..3c2732380517 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -17,28 +17,23 @@
 #include "exynos_drm_drv.h"
 #include "exynos_drm_gem.h"
 
-static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem)
+static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
 {
 	struct drm_device *dev = exynos_gem->base.dev;
-	unsigned long attr;
-	unsigned int nr_pages;
-	struct sg_table sgt;
-	int ret = -ENOMEM;
+	unsigned long attr = 0;
 
 	if (exynos_gem->dma_addr) {
 		DRM_DEV_DEBUG_KMS(to_dma_dev(dev), "already allocated.\n");
 		return 0;
 	}
 
-	exynos_gem->dma_attrs = 0;
-
 	/*
 	 * if EXYNOS_BO_CONTIG, fully physically contiguous memory
 	 * region will be allocated else physically contiguous
 	 * as possible.
 	 */
 	if (!(exynos_gem->flags & EXYNOS_BO_NONCONTIG))
-		exynos_gem->dma_attrs |= DMA_ATTR_FORCE_CONTIGUOUS;
+		attr |= DMA_ATTR_FORCE_CONTIGUOUS;
 
 	/*
 	 * if EXYNOS_BO_WC or EXYNOS_BO_NONCACHABLE, writecombine mapping
@@ -46,61 +41,29 @@ static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem)
 	 */
 	if (exynos_gem->flags & EXYNOS_BO_WC ||
 			!(exynos_gem->flags & EXYNOS_BO_CACHABLE))
-		attr = DMA_ATTR_WRITE_COMBINE;
+		attr |= DMA_ATTR_WRITE_COMBINE;
 	else
-		attr = DMA_ATTR_NON_CONSISTENT;
-
-	exynos_gem->dma_attrs |= attr;
-	exynos_gem->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
-
-	nr_pages = exynos_gem->size >> PAGE_SHIFT;
+		attr |= DMA_ATTR_NON_CONSISTENT;
 
-	exynos_gem->pages = kvmalloc_array(nr_pages, sizeof(struct page *),
-			GFP_KERNEL | __GFP_ZERO);
-	if (!exynos_gem->pages) {
-		DRM_DEV_ERROR(to_dma_dev(dev), "failed to allocate pages.\n");
-		return -ENOMEM;
-	}
+	/* FBDev emulation requires kernel mapping */
+	if (!kvmap)
+		attr |= DMA_ATTR_NO_KERNEL_MAPPING;
 
+	exynos_gem->dma_attrs = attr;
 	exynos_gem->cookie = dma_alloc_attrs(to_dma_dev(dev), exynos_gem->size,
 					     &exynos_gem->dma_addr, GFP_KERNEL,
 					     exynos_gem->dma_attrs);
 	if (!exynos_gem->cookie) {
 		DRM_DEV_ERROR(to_dma_dev(dev), "failed to allocate buffer.\n");
-		goto err_free;
-	}
-
-	ret = dma_get_sgtable_attrs(to_dma_dev(dev), &sgt, exynos_gem->cookie,
-				    exynos_gem->dma_addr, exynos_gem->size,
-				    exynos_gem->dma_attrs);
-	if (ret < 0) {
-		DRM_DEV_ERROR(to_dma_dev(dev), "failed to get sgtable.\n");
-		goto err_dma_free;
-	}
-
-	if (drm_prime_sg_to_page_addr_arrays(&sgt, exynos_gem->pages, NULL,
-					     nr_pages)) {
-		DRM_DEV_ERROR(to_dma_dev(dev), "invalid sgtable.\n");
-		ret = -EINVAL;
-		goto err_sgt_free;
+		return -ENOMEM;
 	}
 
-	sg_free_table(&sgt);
+	if (kvmap)
+		exynos_gem->kvaddr = exynos_gem->cookie;
 
 	DRM_DEV_DEBUG_KMS(to_dma_dev(dev), "dma_addr(0x%lx), size(0x%lx)\n",
 			(unsigned long)exynos_gem->dma_addr, exynos_gem->size);
-
 	return 0;
-
-err_sgt_free:
-	sg_free_table(&sgt);
-err_dma_free:
-	dma_free_attrs(to_dma_dev(dev), exynos_gem->size, exynos_gem->cookie,
-		       exynos_gem->dma_addr, exynos_gem->dma_attrs);
-err_free:
-	kvfree(exynos_gem->pages);
-
-	return ret;
 }
 
 static void exynos_drm_free_buf(struct exynos_drm_gem *exynos_gem)
@@ -118,8 +81,6 @@ static void exynos_drm_free_buf(struct exynos_drm_gem *exynos_gem)
 	dma_free_attrs(to_dma_dev(dev), exynos_gem->size, exynos_gem->cookie,
 			(dma_addr_t)exynos_gem->dma_addr,
 			exynos_gem->dma_attrs);
-
-	kvfree(exynos_gem->pages);
 }
 
 static int exynos_drm_gem_handle_create(struct drm_gem_object *obj,
@@ -203,7 +164,8 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev,
 
 struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
 					     unsigned int flags,
-					     unsigned long size)
+					     unsigned long size,
+					     bool kvmap)
 {
 	struct exynos_drm_gem *exynos_gem;
 	int ret;
@@ -237,7 +199,7 @@ struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
 	/* set memory type and cache attribute from user side. */
 	exynos_gem->flags = flags;
 
-	ret = exynos_drm_alloc_buf(exynos_gem);
+	ret = exynos_drm_alloc_buf(exynos_gem, kvmap);
 	if (ret < 0) {
 		drm_gem_object_release(&exynos_gem->base);
 		kfree(exynos_gem);
@@ -254,7 +216,7 @@ int exynos_drm_gem_create_ioctl(struct drm_device *dev, void *data,
 	struct exynos_drm_gem *exynos_gem;
 	int ret;
 
-	exynos_gem = exynos_drm_gem_create(dev, args->flags, args->size);
+	exynos_gem = exynos_drm_gem_create(dev, args->flags, args->size, false);
 	if (IS_ERR(exynos_gem))
 		return PTR_ERR(exynos_gem);
 
@@ -365,7 +327,7 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
 	else
 		flags = EXYNOS_BO_CONTIG | EXYNOS_BO_WC;
 
-	exynos_gem = exynos_drm_gem_create(dev, flags, args->size);
+	exynos_gem = exynos_drm_gem_create(dev, flags, args->size, false);
 	if (IS_ERR(exynos_gem)) {
 		dev_warn(dev->dev, "FB allocation failed.\n");
 		return PTR_ERR(exynos_gem);
@@ -442,11 +404,24 @@ struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
 struct sg_table *exynos_drm_gem_prime_get_sg_table(struct drm_gem_object *obj)
 {
 	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
-	int npages;
+	struct drm_device *drm_dev = obj->dev;
+	struct sg_table *sgt;
+	int ret;
+
+	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt)
+		return ERR_PTR(-ENOMEM);
 
-	npages = exynos_gem->size >> PAGE_SHIFT;
+	ret = dma_get_sgtable_attrs(to_dma_dev(drm_dev), sgt, exynos_gem->cookie,
+				    exynos_gem->dma_addr, exynos_gem->size,
+				    exynos_gem->dma_attrs);
+	if (ret) {
+		DRM_ERROR("failed to get sgtable, %d\n", ret);
+		kfree(sgt);
+		return ERR_PTR(ret);
+	}
 
-	return drm_prime_pages_to_sg(exynos_gem->pages, npages);
+	return sgt;
 }
 
 struct drm_gem_object *
@@ -455,8 +430,6 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 				     struct sg_table *sgt)
 {
 	struct exynos_drm_gem *exynos_gem;
-	int npages;
-	int ret;
 
 	if (sgt->nents != 1) {
 		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
@@ -476,26 +449,8 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 	}
 
 	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
-	if (IS_ERR(exynos_gem)) {
-		ret = PTR_ERR(exynos_gem);
-		return ERR_PTR(ret);
-	}
-
-	exynos_gem->dma_addr = sg_dma_address(sgt->sgl);
-
-	npages = exynos_gem->size >> PAGE_SHIFT;
-	exynos_gem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
-	if (!exynos_gem->pages) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	ret = drm_prime_sg_to_page_addr_arrays(sgt, exynos_gem->pages, NULL,
-					       npages);
-	if (ret < 0)
-		goto err_free_large;
-
-	exynos_gem->sgt = sgt;
+	if (IS_ERR(exynos_gem))
+		return ERR_CAST(exynos_gem);
 
 	/*
 	 * Buffer has been mapped as contiguous into DMA address space,
@@ -507,14 +462,9 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 	else
 		exynos_gem->flags |= EXYNOS_BO_CONTIG;
 
+	exynos_gem->dma_addr = sg_dma_address(sgt->sgl);
+	exynos_gem->sgt = sgt;
 	return &exynos_gem->base;
-
-err_free_large:
-	kvfree(exynos_gem->pages);
-err:
-	drm_gem_object_release(&exynos_gem->base);
-	kfree(exynos_gem);
-	return ERR_PTR(ret);
 }
 
 void *exynos_drm_gem_prime_vmap(struct drm_gem_object *obj)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
index f00044c0b688..6ef001f890aa 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
@@ -21,20 +21,15 @@
  * @base: a gem object.
  *	- a new handle to this gem object would be created
  *	by drm_gem_handle_create().
- * @buffer: a pointer to exynos_drm_gem_buffer object.
- *	- contain the information to memory region allocated
- *	by user request or at framebuffer creation.
- *	continuous memory region allocated by user request
- *	or at framebuffer creation.
  * @flags: indicate memory type to allocated buffer and cache attruibute.
  * @size: size requested from user, in bytes and this size is aligned
  *	in page unit.
  * @cookie: cookie returned by dma_alloc_attrs
- * @kvaddr: kernel virtual address to allocated memory region.
+ * @kvaddr: kernel virtual address to allocated memory region (for fbdev)
  * @dma_addr: bus address(accessed by dma) to allocated memory region.
  *	- this address could be physical address without IOMMU and
  *	device address with IOMMU.
- * @pages: Array of backing pages.
+ * @dma_attrs: attrs passed dma mapping framework
  * @sgt: Imported sg_table.
  *
  * P.S. this object would be transferred to user as kms_bo.handle so
@@ -48,7 +43,6 @@ struct exynos_drm_gem {
 	void __iomem		*kvaddr;
 	dma_addr_t		dma_addr;
 	unsigned long		dma_attrs;
-	struct page		**pages;
 	struct sg_table		*sgt;
 };
 
@@ -58,7 +52,8 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem *exynos_gem);
 /* create a new buffer with gem object */
 struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
 					     unsigned int flags,
-					     unsigned long size);
+					     unsigned long size,
+					     bool kvmap);
 
 /*
  * request gem object creation and buffer allocation as the size
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/exynos: gem: Get rid of the internal 'pages' array

[Inki Dae]

20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
> Internal pages array and scatter-list for them is not really needed for
> anything. FBDev emulation can simply rely on the DMA-mapping framework
> to create a proper kernel mapping for the buffer, while all other buffer
> use cases don't really need that array at all.

Picked it up.

Thanks,
Inki Dae

> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  28 +----
>  drivers/gpu/drm/exynos/exynos_drm_gem.c   | 124 +++++++---------------
>  drivers/gpu/drm/exynos/exynos_drm_gem.h   |  13 +--
>  3 files changed, 42 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index e6ceaf36fb04..56a2b47e1af7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -76,7 +76,6 @@ static int exynos_drm_fbdev_update(struct drm_fb_helper *helper,
>  	struct fb_info *fbi;
>  	struct drm_framebuffer *fb = helper->fb;
>  	unsigned int size = fb->width * fb->height * fb->format->cpp[0];
> -	unsigned int nr_pages;
>  	unsigned long offset;
>  
>  	fbi = drm_fb_helper_alloc_fbi(helper);
> @@ -90,16 +89,6 @@ static int exynos_drm_fbdev_update(struct drm_fb_helper *helper,
>  
>  	drm_fb_helper_fill_info(fbi, helper, sizes);
>  
> -	nr_pages = exynos_gem->size >> PAGE_SHIFT;
> -
> -	exynos_gem->kvaddr = (void __iomem *) vmap(exynos_gem->pages, nr_pages,
> -				VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> -	if (!exynos_gem->kvaddr) {
> -		DRM_DEV_ERROR(to_dma_dev(helper->dev),
> -			      "failed to map pages to kernel space.\n");
> -		return -EIO;
> -	}
> -
>  	offset = fbi->var.xoffset * fb->format->cpp[0];
>  	offset += fbi->var.yoffset * fb->pitches[0];
>  
> @@ -133,18 +122,7 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper,
>  
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  
> -	exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size);
> -	/*
> -	 * If physically contiguous memory allocation fails and if IOMMU is
> -	 * supported then try to get buffer from non physically contiguous
> -	 * memory area.
> -	 */
> -	if (IS_ERR(exynos_gem) && is_drm_iommu_supported(dev)) {
> -		dev_warn(dev->dev, "contiguous FB allocation failed, falling back to non-contiguous\n");
> -		exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_NONCONTIG,
> -						   size);
> -	}
> -
> +	exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_WC, size, true);
>  	if (IS_ERR(exynos_gem))
>  		return PTR_ERR(exynos_gem);
>  
> @@ -229,12 +207,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>  static void exynos_drm_fbdev_destroy(struct drm_device *dev,
>  				      struct drm_fb_helper *fb_helper)
>  {
> -	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(fb_helper);
> -	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
>  	struct drm_framebuffer *fb;
>  
> -	vunmap(exynos_gem->kvaddr);
> -
>  	/* release drm framebuffer and real buffer */
>  	if (fb_helper->fb && fb_helper->fb->funcs) {
>  		fb = fb_helper->fb;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 9d4e4d321bda..3c2732380517 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -17,28 +17,23 @@
>  #include "exynos_drm_drv.h"
>  #include "exynos_drm_gem.h"
>  
> -static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem)
> +static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
>  {
>  	struct drm_device *dev = exynos_gem->base.dev;
> -	unsigned long attr;
> -	unsigned int nr_pages;
> -	struct sg_table sgt;
> -	int ret = -ENOMEM;
> +	unsigned long attr = 0;
>  
>  	if (exynos_gem->dma_addr) {
>  		DRM_DEV_DEBUG_KMS(to_dma_dev(dev), "already allocated.\n");
>  		return 0;
>  	}
>  
> -	exynos_gem->dma_attrs = 0;
> -
>  	/*
>  	 * if EXYNOS_BO_CONTIG, fully physically contiguous memory
>  	 * region will be allocated else physically contiguous
>  	 * as possible.
>  	 */
>  	if (!(exynos_gem->flags & EXYNOS_BO_NONCONTIG))
> -		exynos_gem->dma_attrs |= DMA_ATTR_FORCE_CONTIGUOUS;
> +		attr |= DMA_ATTR_FORCE_CONTIGUOUS;
>  
>  	/*
>  	 * if EXYNOS_BO_WC or EXYNOS_BO_NONCACHABLE, writecombine mapping
> @@ -46,61 +41,29 @@ static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem)
>  	 */
>  	if (exynos_gem->flags & EXYNOS_BO_WC ||
>  			!(exynos_gem->flags & EXYNOS_BO_CACHABLE))
> -		attr = DMA_ATTR_WRITE_COMBINE;
> +		attr |= DMA_ATTR_WRITE_COMBINE;
>  	else
> -		attr = DMA_ATTR_NON_CONSISTENT;
> -
> -	exynos_gem->dma_attrs |= attr;
> -	exynos_gem->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> -
> -	nr_pages = exynos_gem->size >> PAGE_SHIFT;
> +		attr |= DMA_ATTR_NON_CONSISTENT;
>  
> -	exynos_gem->pages = kvmalloc_array(nr_pages, sizeof(struct page *),
> -			GFP_KERNEL | __GFP_ZERO);
> -	if (!exynos_gem->pages) {
> -		DRM_DEV_ERROR(to_dma_dev(dev), "failed to allocate pages.\n");
> -		return -ENOMEM;
> -	}
> +	/* FBDev emulation requires kernel mapping */
> +	if (!kvmap)
> +		attr |= DMA_ATTR_NO_KERNEL_MAPPING;
>  
> +	exynos_gem->dma_attrs = attr;
>  	exynos_gem->cookie = dma_alloc_attrs(to_dma_dev(dev), exynos_gem->size,
>  					     &exynos_gem->dma_addr, GFP_KERNEL,
>  					     exynos_gem->dma_attrs);
>  	if (!exynos_gem->cookie) {
>  		DRM_DEV_ERROR(to_dma_dev(dev), "failed to allocate buffer.\n");
> -		goto err_free;
> -	}
> -
> -	ret = dma_get_sgtable_attrs(to_dma_dev(dev), &sgt, exynos_gem->cookie,
> -				    exynos_gem->dma_addr, exynos_gem->size,
> -				    exynos_gem->dma_attrs);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(to_dma_dev(dev), "failed to get sgtable.\n");
> -		goto err_dma_free;
> -	}
> -
> -	if (drm_prime_sg_to_page_addr_arrays(&sgt, exynos_gem->pages, NULL,
> -					     nr_pages)) {
> -		DRM_DEV_ERROR(to_dma_dev(dev), "invalid sgtable.\n");
> -		ret = -EINVAL;
> -		goto err_sgt_free;
> +		return -ENOMEM;
>  	}
>  
> -	sg_free_table(&sgt);
> +	if (kvmap)
> +		exynos_gem->kvaddr = exynos_gem->cookie;
>  
>  	DRM_DEV_DEBUG_KMS(to_dma_dev(dev), "dma_addr(0x%lx), size(0x%lx)\n",
>  			(unsigned long)exynos_gem->dma_addr, exynos_gem->size);
> -
>  	return 0;
> -
> -err_sgt_free:
> -	sg_free_table(&sgt);
> -err_dma_free:
> -	dma_free_attrs(to_dma_dev(dev), exynos_gem->size, exynos_gem->cookie,
> -		       exynos_gem->dma_addr, exynos_gem->dma_attrs);
> -err_free:
> -	kvfree(exynos_gem->pages);
> -
> -	return ret;
>  }
>  
>  static void exynos_drm_free_buf(struct exynos_drm_gem *exynos_gem)
> @@ -118,8 +81,6 @@ static void exynos_drm_free_buf(struct exynos_drm_gem *exynos_gem)
>  	dma_free_attrs(to_dma_dev(dev), exynos_gem->size, exynos_gem->cookie,
>  			(dma_addr_t)exynos_gem->dma_addr,
>  			exynos_gem->dma_attrs);
> -
> -	kvfree(exynos_gem->pages);
>  }
>  
>  static int exynos_drm_gem_handle_create(struct drm_gem_object *obj,
> @@ -203,7 +164,8 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev,
>  
>  struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
>  					     unsigned int flags,
> -					     unsigned long size)
> +					     unsigned long size,
> +					     bool kvmap)
>  {
>  	struct exynos_drm_gem *exynos_gem;
>  	int ret;
> @@ -237,7 +199,7 @@ struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
>  	/* set memory type and cache attribute from user side. */
>  	exynos_gem->flags = flags;
>  
> -	ret = exynos_drm_alloc_buf(exynos_gem);
> +	ret = exynos_drm_alloc_buf(exynos_gem, kvmap);
>  	if (ret < 0) {
>  		drm_gem_object_release(&exynos_gem->base);
>  		kfree(exynos_gem);
> @@ -254,7 +216,7 @@ int exynos_drm_gem_create_ioctl(struct drm_device *dev, void *data,
>  	struct exynos_drm_gem *exynos_gem;
>  	int ret;
>  
> -	exynos_gem = exynos_drm_gem_create(dev, args->flags, args->size);
> +	exynos_gem = exynos_drm_gem_create(dev, args->flags, args->size, false);
>  	if (IS_ERR(exynos_gem))
>  		return PTR_ERR(exynos_gem);
>  
> @@ -365,7 +327,7 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  	else
>  		flags = EXYNOS_BO_CONTIG | EXYNOS_BO_WC;
>  
> -	exynos_gem = exynos_drm_gem_create(dev, flags, args->size);
> +	exynos_gem = exynos_drm_gem_create(dev, flags, args->size, false);
>  	if (IS_ERR(exynos_gem)) {
>  		dev_warn(dev->dev, "FB allocation failed.\n");
>  		return PTR_ERR(exynos_gem);
> @@ -442,11 +404,24 @@ struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>  struct sg_table *exynos_drm_gem_prime_get_sg_table(struct drm_gem_object *obj)
>  {
>  	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
> -	int npages;
> +	struct drm_device *drm_dev = obj->dev;
> +	struct sg_table *sgt;
> +	int ret;
> +
> +	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> +	if (!sgt)
> +		return ERR_PTR(-ENOMEM);
>  
> -	npages = exynos_gem->size >> PAGE_SHIFT;
> +	ret = dma_get_sgtable_attrs(to_dma_dev(drm_dev), sgt, exynos_gem->cookie,
> +				    exynos_gem->dma_addr, exynos_gem->size,
> +				    exynos_gem->dma_attrs);
> +	if (ret) {
> +		DRM_ERROR("failed to get sgtable, %d\n", ret);
> +		kfree(sgt);
> +		return ERR_PTR(ret);
> +	}
>  
> -	return drm_prime_pages_to_sg(exynos_gem->pages, npages);
> +	return sgt;
>  }
>  
>  struct drm_gem_object *
> @@ -455,8 +430,6 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  				     struct sg_table *sgt)
>  {
>  	struct exynos_drm_gem *exynos_gem;
> -	int npages;
> -	int ret;
>  
>  	if (sgt->nents != 1) {
>  		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
> @@ -476,26 +449,8 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  	}
>  
>  	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
> -	if (IS_ERR(exynos_gem)) {
> -		ret = PTR_ERR(exynos_gem);
> -		return ERR_PTR(ret);
> -	}
> -
> -	exynos_gem->dma_addr = sg_dma_address(sgt->sgl);
> -
> -	npages = exynos_gem->size >> PAGE_SHIFT;
> -	exynos_gem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> -	if (!exynos_gem->pages) {
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> -
> -	ret = drm_prime_sg_to_page_addr_arrays(sgt, exynos_gem->pages, NULL,
> -					       npages);
> -	if (ret < 0)
> -		goto err_free_large;
> -
> -	exynos_gem->sgt = sgt;
> +	if (IS_ERR(exynos_gem))
> +		return ERR_CAST(exynos_gem);
>  
>  	/*
>  	 * Buffer has been mapped as contiguous into DMA address space,
> @@ -507,14 +462,9 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  	else
>  		exynos_gem->flags |= EXYNOS_BO_CONTIG;
>  
> +	exynos_gem->dma_addr = sg_dma_address(sgt->sgl);
> +	exynos_gem->sgt = sgt;
>  	return &exynos_gem->base;
> -
> -err_free_large:
> -	kvfree(exynos_gem->pages);
> -err:
> -	drm_gem_object_release(&exynos_gem->base);
> -	kfree(exynos_gem);
> -	return ERR_PTR(ret);
>  }
>  
>  void *exynos_drm_gem_prime_vmap(struct drm_gem_object *obj)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index f00044c0b688..6ef001f890aa 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -21,20 +21,15 @@
>   * @base: a gem object.
>   *	- a new handle to this gem object would be created
>   *	by drm_gem_handle_create().
> - * @buffer: a pointer to exynos_drm_gem_buffer object.
> - *	- contain the information to memory region allocated
> - *	by user request or at framebuffer creation.
> - *	continuous memory region allocated by user request
> - *	or at framebuffer creation.
>   * @flags: indicate memory type to allocated buffer and cache attruibute.
>   * @size: size requested from user, in bytes and this size is aligned
>   *	in page unit.
>   * @cookie: cookie returned by dma_alloc_attrs
> - * @kvaddr: kernel virtual address to allocated memory region.
> + * @kvaddr: kernel virtual address to allocated memory region (for fbdev)
>   * @dma_addr: bus address(accessed by dma) to allocated memory region.
>   *	- this address could be physical address without IOMMU and
>   *	device address with IOMMU.
> - * @pages: Array of backing pages.
> + * @dma_attrs: attrs passed dma mapping framework
>   * @sgt: Imported sg_table.
>   *
>   * P.S. this object would be transferred to user as kms_bo.handle so
> @@ -48,7 +43,6 @@ struct exynos_drm_gem {
>  	void __iomem		*kvaddr;
>  	dma_addr_t		dma_addr;
>  	unsigned long		dma_attrs;
> -	struct page		**pages;
>  	struct sg_table		*sgt;
>  };
>  
> @@ -58,7 +52,8 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem *exynos_gem);
>  /* create a new buffer with gem object */
>  struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
>  					     unsigned int flags,
> -					     unsigned long size);
> +					     unsigned long size,
> +					     bool kvmap);
>  
>  /*
>   * request gem object creation and buffer allocation as the size
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel