[PATCH] drm/omap: fix tiled buffer stride calculations

[PATCH] drm/omap: fix tiled buffer stride calculations

[Tomi Valkeinen]

omap_gem uses page alignment for buffer stride. The related calculations
are a bit off, though, as byte stride of 4096 gets aligned to 8192,
instead of 4096. This patch fixes those calculations.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 13abf221d153..4c41000ff4c4 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
 	size_t size = PAGE_SIZE * n;
 	loff_t off = mmap_offset(obj) +
 			(entry->obj_pgoff << PAGE_SHIFT);
-	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
+	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
 
 	if (m > 1) {
 		int i;
@@ -424,7 +424,7 @@ static int fault_2d(struct drm_gem_object *obj,
 	 * into account in some of the math, so figure out virtual stride
 	 * in pages
 	 */
-	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
+	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
 
 	/* We don't use vmf->pgoff since that has the fake offset: */
 	pgoff = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
-- 
2.7.4

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

Re: [PATCH] drm/omap: fix tiled buffer stride calculations

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thursday 18 May 2017 13:28:26 Tomi Valkeinen wrote:
> omap_gem uses page alignment for buffer stride. The related calculations
> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
> instead of 4096. This patch fixes those calculations.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..4c41000ff4c4
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
>  	size_t size = PAGE_SIZE * n;
>  	loff_t off = mmap_offset(obj) +
>  			(entry->obj_pgoff << PAGE_SHIFT);
> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> +	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);

How about 

	int m = round_up(omap_obj->width << fmt, PAGE_SIZE);

instead of open-coding it ? I find that a bit easier to understand.

By the way, shifting left by fmt should be fine for TILFMT_8BIT, TILFMT_16BIT 
and TILFMT_32BIT that evaluate to 0, 1 and 2 respectively, but how does it 
work with TILFMT_PAGE ? fmt is computed by gem2fmt() in call cases, which 
returns TILFMT_PAGE in the default case (no tiled flag set). Can this happen 
in practice ?

> 
>  	if (m > 1) {
>  		int i;
> @@ -424,7 +424,7 @@ static int fault_2d(struct drm_gem_object *obj,
>  	 * into account in some of the math, so figure out virtual stride
>  	 * in pages
>  	 */
> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> +	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
> 
>  	/* We don't use vmf->pgoff since that has the fake offset: */
>  	pgoff = (vmf->address - vma->vm_start) >> PAGE_SHIFT;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] drm/omap: fix tiled buffer stride calculations

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1825 bytes --]

On 18/05/17 13:59, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thursday 18 May 2017 13:28:26 Tomi Valkeinen wrote:
>> omap_gem uses page alignment for buffer stride. The related calculations
>> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
>> instead of 4096. This patch fixes those calculations.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..4c41000ff4c4
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
>>  	size_t size = PAGE_SIZE * n;
>>  	loff_t off = mmap_offset(obj) +
>>  			(entry->obj_pgoff << PAGE_SHIFT);
>> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
>> +	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
> 
> How about 
> 
> 	int m = round_up(omap_obj->width << fmt, PAGE_SIZE);
> 
> instead of open-coding it ? I find that a bit easier to understand.

That should be:

round_up(omap_obj->width << fmt, PAGE_SIZE) / PAGE_SIZE;

Yes, I think that's more understandable.

> By the way, shifting left by fmt should be fine for TILFMT_8BIT, TILFMT_16BIT 
> and TILFMT_32BIT that evaluate to 0, 1 and 2 respectively, but how does it 
> work with TILFMT_PAGE ? fmt is computed by gem2fmt() in call cases, which 
> returns TILFMT_PAGE in the default case (no tiled flag set). Can this happen 
> in practice ?

These functions are only called for 2D buffers. I do find shifting by
'enum tiler_fmt' quite ugly, though...

 Tomi


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/omap: fix tiled buffer stride calculations

[Laurent Pinchart]

Hi Tomi,

On Thursday 18 May 2017 14:14:35 Tomi Valkeinen wrote:
> On 18/05/17 13:59, Laurent Pinchart wrote:
> > On Thursday 18 May 2017 13:28:26 Tomi Valkeinen wrote:
> >> omap_gem uses page alignment for buffer stride. The related calculations
> >> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
> >> instead of 4096. This patch fixes those calculations.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..4c41000ff4c4
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
> >>  	size_t size = PAGE_SIZE * n;
> >>  	loff_t off = mmap_offset(obj) +
> >>  			(entry->obj_pgoff << PAGE_SHIFT);
> >> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> >> +	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
> > 
> > How about
> > 
> > 	int m = round_up(omap_obj->width << fmt, PAGE_SIZE);
> > 
> > instead of open-coding it ? I find that a bit easier to understand.
> 
> That should be:
> 
> round_up(omap_obj->width << fmt, PAGE_SIZE) / PAGE_SIZE;

Sorry, I meant DIV_ROUND_UP, not round_up.

> Yes, I think that's more understandable.
>
> > By the way, shifting left by fmt should be fine for TILFMT_8BIT,
> > TILFMT_16BIT and TILFMT_32BIT that evaluate to 0, 1 and 2 respectively,
> > but how does it work with TILFMT_PAGE ? fmt is computed by gem2fmt() in
> > call cases, which returns TILFMT_PAGE in the default case (no tiled flag
> > set). Can this happen in practice ?
> 
> These functions are only called for 2D buffers. I do find shifting by
> 'enum tiler_fmt' quite ugly, though...

So do I. This should be added to the infinite todo list :-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] drm/omap: fix tiled buffer stride calculations

[Ville Syrjälä]

On Thu, May 18, 2017 at 02:14:35PM +0300, Tomi Valkeinen wrote:
> On 18/05/17 13:59, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > Thank you for the patch.
> > 
> > On Thursday 18 May 2017 13:28:26 Tomi Valkeinen wrote:
> >> omap_gem uses page alignment for buffer stride. The related calculations
> >> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
> >> instead of 4096. This patch fixes those calculations.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..4c41000ff4c4
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
> >>  	size_t size = PAGE_SIZE * n;
> >>  	loff_t off = mmap_offset(obj) +
> >>  			(entry->obj_pgoff << PAGE_SHIFT);
> >> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> >> +	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
> > 
> > How about 
> > 
> > 	int m = round_up(omap_obj->width << fmt, PAGE_SIZE);
> > 
> > instead of open-coding it ? I find that a bit easier to understand.
> 
> That should be:
> 
> round_up(omap_obj->width << fmt, PAGE_SIZE) / PAGE_SIZE;

Or a bit more optimally just DIV_ROUND_UP()

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omap: fix tiled buffer stride calculations

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2993 bytes --]

On 18/05/17 14:25, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Thursday 18 May 2017 14:14:35 Tomi Valkeinen wrote:
>> On 18/05/17 13:59, Laurent Pinchart wrote:
>>> On Thursday 18 May 2017 13:28:26 Tomi Valkeinen wrote:
>>>> omap_gem uses page alignment for buffer stride. The related calculations
>>>> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
>>>> instead of 4096. This patch fixes those calculations.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>>>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..4c41000ff4c4
>>>> 100644
>>>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>>>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>>>> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
>>>>  	size_t size = PAGE_SIZE * n;
>>>>  	loff_t off = mmap_offset(obj) +
>>>>  			(entry->obj_pgoff << PAGE_SHIFT);
>>>> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
>>>> +	const int m = 1 + (((omap_obj->width - 1) << fmt) / PAGE_SIZE);
>>>
>>> How about
>>>
>>> 	int m = round_up(omap_obj->width << fmt, PAGE_SIZE);
>>>
>>> instead of open-coding it ? I find that a bit easier to understand.
>>
>> That should be:
>>
>> round_up(omap_obj->width << fmt, PAGE_SIZE) / PAGE_SIZE;
> 
> Sorry, I meant DIV_ROUND_UP, not round_up.

Ah, of course. Not my day... =)

From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Thu, 18 May 2017 11:51:51 +0300
Subject: [PATCH] drm/omap: fix tiled buffer stride calculations

omap_gem uses page alignment for buffer stride. The related calculations
are a bit off, though, as byte stride of 4096 gets aligned to 8192,
instead of 4096.

This patch changes the code to use DIV_ROUND_UP(), which fixes those
calculations and makes them more readable.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 13abf221d153..5c5c86ddd6f4 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
 	size_t size = PAGE_SIZE * n;
 	loff_t off = mmap_offset(obj) +
 			(entry->obj_pgoff << PAGE_SHIFT);
-	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
+	const int m = DIV_ROUND_UP(omap_obj->width << fmt, PAGE_SIZE);
 
 	if (m > 1) {
 		int i;
@@ -424,7 +424,7 @@ static int fault_2d(struct drm_gem_object *obj,
 	 * into account in some of the math, so figure out virtual stride
 	 * in pages
 	 */
-	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
+	const int m = DIV_ROUND_UP(omap_obj->width << fmt, PAGE_SIZE);
 
 	/* We don't use vmf->pgoff since that has the fake offset: */
 	pgoff = (vmf->address - vma->vm_start) >> PAGE_SHIFT;


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/omap: fix tiled buffer stride calculations

[Laurent Pinchart]

Hi Tomi,

On Thursday 18 May 2017 14:34:09 Tomi Valkeinen wrote:
> On 18/05/17 14:25, Laurent Pinchart wrote:
> > On Thursday 18 May 2017 14:14:35 Tomi Valkeinen wrote:
> >> On 18/05/17 13:59, Laurent Pinchart wrote:
> >>> On Thursday 18 May 2017 13:28:26 Tomi Valkeinen wrote:
> >>>> omap_gem uses page alignment for buffer stride. The related
> >>>> calculations
> >>>> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
> >>>> instead of 4096. This patch fixes those calculations.
> >>>> 
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >>>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..4c41000ff4c4
> >>>> 100644
> >>>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >>>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >>>> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
> >>>>  	size_t size = PAGE_SIZE * n;
> >>>>  	loff_t off = mmap_offset(obj) +
> >>>>  			(entry->obj_pgoff << PAGE_SHIFT);
> >>>> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> >>>> +	const int m = 1 + (((omap_obj->width - 1) << fmt) /
> >>>> PAGE_SIZE);
> >>> 
> >>> How about
> >>> 
> >>> 	int m = round_up(omap_obj->width << fmt, PAGE_SIZE);
> >>> 
> >>> instead of open-coding it ? I find that a bit easier to understand.
> >> 
> >> That should be:
> >> 
> >> round_up(omap_obj->width << fmt, PAGE_SIZE) / PAGE_SIZE;
> > 
> > Sorry, I meant DIV_ROUND_UP, not round_up.
> 
> Ah, of course. Not my day... =)
> 
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Thu, 18 May 2017 11:51:51 +0300
> Subject: [PATCH] drm/omap: fix tiled buffer stride calculations
> 
> omap_gem uses page alignment for buffer stride. The related calculations
> are a bit off, though, as byte stride of 4096 gets aligned to 8192,
> instead of 4096.
> 
> This patch changes the code to use DIV_ROUND_UP(), which fixes those
> calculations and makes them more readable.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 13abf221d153..5c5c86ddd6f4
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -182,7 +182,7 @@ static void evict_entry(struct drm_gem_object *obj,
>  	size_t size = PAGE_SIZE * n;
>  	loff_t off = mmap_offset(obj) +
>  			(entry->obj_pgoff << PAGE_SHIFT);
> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> +	const int m = DIV_ROUND_UP(omap_obj->width << fmt, PAGE_SIZE);
> 
>  	if (m > 1) {
>  		int i;
> @@ -424,7 +424,7 @@ static int fault_2d(struct drm_gem_object *obj,
>  	 * into account in some of the math, so figure out virtual stride
>  	 * in pages
>  	 */
> -	const int m = 1 + ((omap_obj->width << fmt) / PAGE_SIZE);
> +	const int m = DIV_ROUND_UP(omap_obj->width << fmt, PAGE_SIZE);
> 
>  	/* We don't use vmf->pgoff since that has the fake offset: */
>  	pgoff = (vmf->address - vma->vm_start) >> PAGE_SHIFT;

-- 
Regards,

Laurent Pinchart

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