Hi Tomasz, first of all, thanks for sending this patch! Much appreciated. Comments below.
On Fri, 29 Oct 2010 19:20:17 +0200, Tomasz Rybak <bogom...@post.pl> wrote: > The biggest problem is that IMO (I might be wrong - do > not know PyCUDA to such intimate details) it introduces > non-backward-compatible change. Why do this? Why not introduce a new interface and leave the old one in place? That way, we can compile both interfaces as long as they're available, and it's up to the user which one to use. We shouldn't be in the business of imposing API use restrictions on PyCUDA users. > I have split BufferObject into two: > BufferObject responsible for buffers (VBO, etc.) > ImageObject responsible for textures, etc. > > Previously BufferObject was responsible for both those > cases - now anyone who wants to use CUDA on images > must use ImageObject (which is API-breaking change). Can you explain the reasoning for this split? A brief glance over the code didn't seem to reveal many differences. If nothing else, they should probably inherit from a common base that absorbs the shared code. Also, I noticed that your use of cudaGrahpicsXXX flags with cuGrahpicsGLRegisterBuffer() is likely erroneous, as is visible in this bit of documentation. There are equivalent CU_... flags which we should use instead. See docs here (or in the header): http://developer.download.nvidia.com/compute/cuda/3_0-Beta1/toolkit/docs/online/group__CUGL_ge148ed92fe0728be19c9281302b5922c.html Also fishy: image_object, if it is different code, should probably be using cuGraphicsGLRegisterImage() (rather than ... Buffer()). > I also attach problem sample code (Qt+OpenGL+PyCUDA) > that uses new BufferObject. > I have checked and it works after applying my patch; > I only had to change initialisation to get program > using VBO running after applying my patch. I still don't like that we need to change the init sequence--why is this necessary? > I have not checked ImageObject. > > I have also started writing documentation for new pycuda.gl > functions. > > I have removed gl_init as documentation says that cuGLInit() > is deprecated and is not doing anything (Reference 4.41.4.1). Again, as above, we shouldn't be making API use decisions for our users. I assume it did do something at some point in the past, so as long as feasible we should still wrap it. > Instead I have created gl_select_device; this function however > must be called as the first (before all other CUDA functions), > is cuda*, not cu* function, does not cooperate with > CUDAPP_CALL_GUARDED - so I do not know what to do with it. > I was not able to test it - I do not have setup with two devices. As above, I don't like mixing in cuda* (run-time API) functionality with cu* (driver API) functionality. > Other problems: > > Function pycuda.tools.get_default_device is depreciated, > but pycuda.gl.autoinit uses it. I have changed autoinit > to use Device(0), but it is not elegant and should be changed. > > Function pycuda.tools.make_default_context contains > repeated code checking for presence of CUDA device: > > def make_default_context(): > ndevices = cuda.Device.count() > if ndevices == 0: > errmsg = "No CUDA enabled device found. Please check your > installation." > raise RuntimeError, errmsg > > ndevices = cuda.Device.count() > if ndevices == 0: > raise RuntimeError("No CUDA enabled device found. " > "Please check your installation.") > > Why? Is that omission, or does this double checking serve > some purpose? Whoops. Fixed. Andreas
pgpWcF3ZFrtNT.pgp
Description: PGP signature
_______________________________________________ PyCUDA mailing list PyCUDA@tiker.net http://lists.tiker.net/listinfo/pycuda
