* [PATCH] drm/omap: fix tiled buffer stride calculations @ 2017-05-18 10:28 Tomi Valkeinen 2017-05-18 10:59 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Tomi Valkeinen @ 2017-05-18 10:28 UTC (permalink / raw) To: dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen, Jyri Sarha 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: fix tiled buffer stride calculations 2017-05-18 10:28 [PATCH] drm/omap: fix tiled buffer stride calculations Tomi Valkeinen @ 2017-05-18 10:59 ` Laurent Pinchart 2017-05-18 11:14 ` Tomi Valkeinen 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2017-05-18 10:59 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Jyri Sarha, dri-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: fix tiled buffer stride calculations 2017-05-18 10:59 ` Laurent Pinchart @ 2017-05-18 11:14 ` Tomi Valkeinen 2017-05-18 11:25 ` Laurent Pinchart 2017-05-18 11:32 ` Ville Syrjälä 0 siblings, 2 replies; 7+ messages in thread From: Tomi Valkeinen @ 2017-05-18 11:14 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Jyri Sarha, dri-devel [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: fix tiled buffer stride calculations 2017-05-18 11:14 ` Tomi Valkeinen @ 2017-05-18 11:25 ` Laurent Pinchart 2017-05-18 11:34 ` Tomi Valkeinen 2017-05-18 11:32 ` Ville Syrjälä 1 sibling, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2017-05-18 11:25 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Jyri Sarha, dri-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: fix tiled buffer stride calculations 2017-05-18 11:25 ` Laurent Pinchart @ 2017-05-18 11:34 ` Tomi Valkeinen 2017-05-18 12:32 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Tomi Valkeinen @ 2017-05-18 11:34 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Jyri Sarha, dri-devel [-- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: fix tiled buffer stride calculations 2017-05-18 11:34 ` Tomi Valkeinen @ 2017-05-18 12:32 ` Laurent Pinchart 0 siblings, 0 replies; 7+ messages in thread From: Laurent Pinchart @ 2017-05-18 12:32 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Jyri Sarha, dri-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/omap: fix tiled buffer stride calculations 2017-05-18 11:14 ` Tomi Valkeinen 2017-05-18 11:25 ` Laurent Pinchart @ 2017-05-18 11:32 ` Ville Syrjälä 1 sibling, 0 replies; 7+ messages in thread From: Ville Syrjälä @ 2017-05-18 11:32 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: dri-devel, Laurent Pinchart, Jyri Sarha 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-05-18 12:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-18 10:28 [PATCH] drm/omap: fix tiled buffer stride calculations Tomi Valkeinen 2017-05-18 10:59 ` Laurent Pinchart 2017-05-18 11:14 ` Tomi Valkeinen 2017-05-18 11:25 ` Laurent Pinchart 2017-05-18 11:34 ` Tomi Valkeinen 2017-05-18 12:32 ` Laurent Pinchart 2017-05-18 11:32 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).