* re: drm/amdgpu: implement cgs gpu memory callbacks
@ 2015-08-21 16:23 Dan Carpenter
2015-08-24 7:09 ` Zhou, David(ChunMing)
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-08-21 16:23 UTC (permalink / raw)
To: david1.zhou; +Cc: dri-devel
Hello Chunming Zhou,
The patch 57ff96cf471a: "drm/amdgpu: implement cgs gpu memory
callbacks" from Apr 24, 2015, leads to the following static checker
warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:274 amdgpu_cgs_gmap_gpu_mem()
warn: should 'obj->placements[0]->fpfn << 12' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:275 amdgpu_cgs_gmap_gpu_mem()
warn: should 'obj->placements[0]->lpfn << 12' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
265 static int amdgpu_cgs_gmap_gpu_mem(void *cgs_device, cgs_handle_t handle,
266 uint64_t *mcaddr)
267 {
268 int r;
269 u64 min_offset, max_offset;
270 struct amdgpu_bo *obj = (struct amdgpu_bo *)handle;
271
272 WARN_ON_ONCE(obj->placement.num_placement > 1);
273
274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Both of these.
276
277 r = amdgpu_bo_reserve(obj, false);
278 if (unlikely(r != 0))
279 return r;
280 r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT,
281 min_offset, max_offset, mcaddr);
282 amdgpu_bo_unreserve(obj);
283 return r;
284 }
There are actually a few of these warnings which were less clear
whether the warning was correct or not so I didn't send them.
drivers/gpu/drm/amd/amdgpu/cz_smc.c:463 cz_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/fiji_smc.c:404 fiji_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:724 amdgpu_cgs_get_firmware_info() warn: should '((header->jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/tonga_smc.c:406 tonga_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:615 amdgpu_gem_op_ioctl() warn: should 'robj->tbo.mem.page_alignment << 12' be a 64 bit type?
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: drm/amdgpu: implement cgs gpu memory callbacks
2015-08-21 16:23 drm/amdgpu: implement cgs gpu memory callbacks Dan Carpenter
@ 2015-08-24 7:09 ` Zhou, David(ChunMing)
2015-08-24 19:51 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Zhou, David(ChunMing) @ 2015-08-24 7:09 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel@lists.freedesktop.org
Hi Dan,
Thanks for figuring out that.
> 274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
> 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Maybe should be:
min_offset = obj->placements[0].fpfn;
min_offset <<= PAGE_SHIFT;
max_offset = obj->placements[0].lpfn;
max_offset <<= PAGE_SHIFT;
Regards,
David Zhou
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Saturday, August 22, 2015 12:24 AM
> To: Zhou, David(ChunMing)
> Cc: dri-devel@lists.freedesktop.org
> Subject: re: drm/amdgpu: implement cgs gpu memory callbacks
>
> Hello Chunming Zhou,
>
> The patch 57ff96cf471a: "drm/amdgpu: implement cgs gpu memory callbacks"
> from Apr 24, 2015, leads to the following static checker
> warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:274
> amdgpu_cgs_gmap_gpu_mem()
> warn: should 'obj->placements[0]->fpfn << 12' be a 64 bit type?
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:275
> amdgpu_cgs_gmap_gpu_mem()
> warn: should 'obj->placements[0]->lpfn << 12' be a 64 bit type?
>
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> 265 static int amdgpu_cgs_gmap_gpu_mem(void *cgs_device, cgs_handle_t
> handle,
> 266 uint64_t *mcaddr)
> 267 {
> 268 int r;
> 269 u64 min_offset, max_offset;
> 270 struct amdgpu_bo *obj = (struct amdgpu_bo *)handle;
> 271
> 272 WARN_ON_ONCE(obj->placement.num_placement > 1);
> 273
> 274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
> 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
>
> Both of these.
>
> 276
> 277 r = amdgpu_bo_reserve(obj, false);
> 278 if (unlikely(r != 0))
> 279 return r;
> 280 r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT,
> 281 min_offset, max_offset, mcaddr);
> 282 amdgpu_bo_unreserve(obj);
> 283 return r;
> 284 }
>
> There are actually a few of these warnings which were less clear whether the
> warning was correct or not so I didn't send them.
>
> drivers/gpu/drm/amd/amdgpu/cz_smc.c:463
> cz_smu_populate_single_firmware_entry() warn: should '((header->jt_offset))
> << 2' be a 64 bit type?
> drivers/gpu/drm/amd/amdgpu/fiji_smc.c:404
> fiji_smu_populate_single_firmware_entry() warn: should '((header->jt_offset))
> << 2' be a 64 bit type?
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:724
> amdgpu_cgs_get_firmware_info() warn: should '((header->jt_offset)) << 2' be a
> 64 bit type?
> drivers/gpu/drm/amd/amdgpu/tonga_smc.c:406
> tonga_smu_populate_single_firmware_entry() warn: should '((header-
> >jt_offset)) << 2' be a 64 bit type?
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:615 amdgpu_gem_op_ioctl()
> warn: should 'robj->tbo.mem.page_alignment << 12' be a 64 bit type?
>
> regards,
> dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drm/amdgpu: implement cgs gpu memory callbacks
2015-08-24 7:09 ` Zhou, David(ChunMing)
@ 2015-08-24 19:51 ` Dan Carpenter
2015-08-25 2:07 ` Zhou, David(ChunMing)
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-08-24 19:51 UTC (permalink / raw)
To: Zhou, David(ChunMing); +Cc: dri-devel@lists.freedesktop.org
On Mon, Aug 24, 2015 at 07:09:15AM +0000, Zhou, David(ChunMing) wrote:
> Hi Dan,
> Thanks for figuring out that.
> > 274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
> > 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
> Maybe should be:
> min_offset = obj->placements[0].fpfn;
> min_offset <<= PAGE_SHIFT;
> max_offset = obj->placements[0].lpfn;
> max_offset <<= PAGE_SHIFT;
It's probably just simpler to be:
min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT;
max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT;
But the larger questions aer why is min_offset a u64, and can the shift
actually wrap? I'm just looking at static checker warnings, and I'm
not very familiar with this code so I don't know the answers.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: drm/amdgpu: implement cgs gpu memory callbacks
2015-08-24 19:51 ` Dan Carpenter
@ 2015-08-25 2:07 ` Zhou, David(ChunMing)
2015-08-25 5:50 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Zhou, David(ChunMing) @ 2015-08-25 2:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel@lists.freedesktop.org
Inline...[DZ]
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Tuesday, August 25, 2015 3:51 AM
> To: Zhou, David(ChunMing)
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
>
> On Mon, Aug 24, 2015 at 07:09:15AM +0000, Zhou, David(ChunMing) wrote:
> > Hi Dan,
> > Thanks for figuring out that.
> > > 274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
> > > 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
> > Maybe should be:
> > min_offset = obj->placements[0].fpfn;
> > min_offset <<= PAGE_SHIFT;
> > max_offset = obj->placements[0].lpfn;
> > max_offset <<= PAGE_SHIFT;
>
>
> It's probably just simpler to be:
>
> min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT;
> max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT;
>
> But the larger questions aer why is min_offset a u64,
[DZ] max/min_offset is memory size.
>and can the shift actually
> wrap?
[DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so <<PAGE_SHIFT is to convert to memory size.
Regards,
David Zhou
> I'm just looking at static checker warnings, and I'm not very familiar
> with this code so I don't know the answers.
>
> regards,
> dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drm/amdgpu: implement cgs gpu memory callbacks
2015-08-25 2:07 ` Zhou, David(ChunMing)
@ 2015-08-25 5:50 ` Dan Carpenter
2015-08-25 6:00 ` Zhou, David(ChunMing)
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-08-25 5:50 UTC (permalink / raw)
To: Zhou, David(ChunMing); +Cc: dri-devel@lists.freedesktop.org
On Tue, Aug 25, 2015 at 02:07:21AM +0000, Zhou, David(ChunMing) wrote:
> >and can the shift actually
> > wrap?
> [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so <<PAGE_SHIFT is to convert to memory size.
By "Shift wrap" I meant how you shift beyond the end of a 32bit number
and it truncates, or if it's signed then it can wrap around (it's
undefined actually, I probably should use a different word?).
int main(void)
{
unsigned long long a = 0xf0000000U << 12;
unsigned long long b = 0xf0000000ULL << 12;
printf("%llx %llx\n", a, b);
return 0;
}
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: drm/amdgpu: implement cgs gpu memory callbacks
2015-08-25 5:50 ` Dan Carpenter
@ 2015-08-25 6:00 ` Zhou, David(ChunMing)
0 siblings, 0 replies; 6+ messages in thread
From: Zhou, David(ChunMing) @ 2015-08-25 6:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel@lists.freedesktop.org
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Tuesday, August 25, 2015 1:51 PM
> To: Zhou, David(ChunMing)
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
>
> On Tue, Aug 25, 2015 at 02:07:21AM +0000, Zhou, David(ChunMing) wrote:
> > >and can the shift actually
> > > wrap?
> > [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so
> <<PAGE_SHIFT is to convert to memory size.
>
> By "Shift wrap" I meant how you shift beyond the end of a 32bit number and it
> truncates, or if it's signed then it can wrap around (it's undefined actually, I
> probably should use a different word?).
>
> int main(void)
> {
> unsigned long long a = 0xf0000000U << 12;
> unsigned long long b = 0xf0000000ULL << 12;
>
> printf("%llx %llx\n", a, b);
>
> return 0;
> }
[DZ] oh, I understand your mean, with previous PAGE_SHIFT, I think it should be like 'b' in your example without truncates.
Thanks,
David Zhou
>
> regards,
> dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-25 6:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-21 16:23 drm/amdgpu: implement cgs gpu memory callbacks Dan Carpenter
2015-08-24 7:09 ` Zhou, David(ChunMing)
2015-08-24 19:51 ` Dan Carpenter
2015-08-25 2:07 ` Zhou, David(ChunMing)
2015-08-25 5:50 ` Dan Carpenter
2015-08-25 6:00 ` Zhou, David(ChunMing)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.