[PATCH libdrm] omap: add omapdrm support

[Rob Clark]

On Wed, Mar 28, 2012 at 9:39 AM, Luca Tettamanti <kronos.it at gmail.com> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel