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
