On Wed, Mar 28, 2012 at 9:39 AM, Luca Tettamanti <kronos.it@xxxxxxxxx> wrote:
> Hi Rob,
> I've a couple of (minor) comments:
>
> On Wed, Mar 21, 2012 at 01:00:36PM -0500, Rob Clark wrote:
>> --- /dev/null
>> +++ b/omap/omap_drm.c
> [...]
>> +/* allocate a new (un-tiled) buffer object */
>> +struct omap_bo * omap_bo_new(struct omap_device *dev,
>> + uint32_t size, uint32_t flags)
>> +{
>> + if (flags & OMAP_BO_TILED) {
>> + return NULL;
>> + }
>> + return omap_bo_new_impl(dev, (union omap_gem_size){
>> + .bytes = size,
>> + }, flags);
>
> Hum, the indentation of the anonymous union looks weird (but maybe it's just
> me...)
>
>> +}
>> +
>> +/* allocate a new buffer object */
>> +struct omap_bo * omap_bo_new_tiled(struct omap_device *dev,
>> + uint32_t width, uint32_t height, uint32_t flags)
>> +{
>> + if (!(flags & OMAP_BO_TILED)) {
>> + return NULL;
>> + }
>> + return omap_bo_new_impl(dev, (union omap_gem_size){
>> + .tiled = {
>> + .width = width,
>> + .height = height,
>> + },
>> + }, flags);
>> +}
>
> Here too :-) What about this:
>
> return omap_bo_new_impl(dev, (union omap_gem_size)
> {
> .stuff = blah,
> });
>
> Or just use a temp var?
ok, open brace on same line seemed somehow more consistent with coding
style for open brace not on a new line, but I think I should just
change to temp var if that is less strange looking
>> +
>> +/* get buffer info */
>> +static int get_buffer_info(struct omap_bo *bo)
>> +{
>> + struct drm_omap_gem_info req = {
>> + .handle = bo->handle,
>> + };
>> + int ret = drmCommandWriteRead(bo->dev->fd, DRM_OMAP_GEM_INFO,
>> + &req, sizeof(req));
>> + if (ret) {
>> + return ret;
>> + }
>> +
>> + /* really all we need for now is mmap offset */
>> + bo->offset = req.offset;
>> + bo->size = req.size;
>> +
>> + return 0;
>> +}
>> +
>> +/* import a buffer object from DRI2 name */
>> +struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name)
>> +{
>> + struct omap_bo *bo = calloc(sizeof(*bo), 1);
>> + struct drm_gem_open req = {
>> + .name = name,
>> + };
>> +
>> + if (drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req)) {
>> + goto fail;
>> + }
>
> bo may be NULL here:
ok, good point, I'll fix this
BR,
-R
>> +
>> + bo->dev = dev;
>> + bo->name = name;
>> + bo->handle = req.handle;
>
> I also woundn't use the calloc in the initialization block, I prefer to
> keep the allocation and the check together:
>
> bo = alloc_stuff();
> if (!bo)
> oh_crap();
>
> I find it more easy to check visually.
>
> Luca
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
