[FFmpeg-devel] [PATCH] lavu: Add DRM hwcontext

[Mark Thompson]

On 01/09/17 18:48, Jorge Ramirez wrote:
> On 09/01/2017 05:44 PM, LongChair . wrote:
>> +static int drm_map_frame(AVHWFramesContext *hwfc,
>> +                         AVFrame *dst, const AVFrame *src, int flags)
>> +{
>> +    const AVDRMFrameDescriptor*desc = (AVDRMFrameDescriptor*)src->data[0];
>> +    DRMMapping *map;
>> +    int err, i, p, plane;
>> +    int mmap_prot;
>> +    void *addr;
>> +
>> +    map = av_mallocz(sizeof(*map));
>> +    if (!map)
>> +        return AVERROR(ENOMEM);
>> +
>> +    mmap_prot = 0;
>> +    if (flags & AV_HWFRAME_MAP_READ)
>> +        mmap_prot |= PROT_READ;
>> +    if (flags & AV_HWFRAME_MAP_WRITE)
>> +        mmap_prot |= PROT_WRITE;
>> +
>> +    av_assert0(desc->nb_objects <= AV_DRM_MAX_PLANES);
>> +    for (i = 0; i < desc->nb_objects; i++) {
>> +        addr = mmap(NULL, desc->objects[i].size, mmap_prot, MAP_SHARED,
>> +                    desc->objects[i].fd, 0);
>> +        if (addr == MAP_FAILED) {
>> +            err = AVERROR(errno);
>> +            av_log(hwfc, AV_LOG_ERROR, "Failed to map DRM object %d to "
>> +                   "memory: %d.\n", desc->objects[i].fd, errno);
>> +            goto fail;
>> +        }
>> +
>> +        map->address[i] = addr;
>> +        map->length[i]  = desc->objects[i].size;
>> +    }
>> +    map->nb_regions = i;
>> +
>> +    plane = 0;
>> +    for (i = 0; i < desc->nb_layers; i++) {
>> +        const AVDRMLayerDescriptor *layer = &desc->layers[i];
>> +        for (p = 0; p < layer->nb_planes; p++) {
>> +            dst->data[plane] =
>> +                (uint8_t*)map->address[layer->planes[p].object_index] +
>> +                                       layer->planes[p].offset;
>> +            dst->linesize[plane] =     layer->planes[p].pitch;
>> +            ++plane;
>> +        }
>> +    }
>> +    av_assert0(plane <= AV_DRM_MAX_PLANES);
>> +
>> +    dst->width  = src->width;
>> +    dst->height = src->height;
>> +
>> +    err = ff_hwframe_map_create(src->hw_frames_ctx, dst, src,
>> +                                &drm_unmap_frame, map);
>> +    if (err < 0)
> shouldn't we unmap and free the map as well?

Yes, fixed to goto fail as well.

>> +        return err;
>> +
>> +    return 0;
>> +
>> +fail:
>> +    for (i = 0; i < desc->nb_objects; i++) {
>> +        if (map->address[i])
>> +            munmap(map->address[i], map->length[i]);
>> +    }
>> +    av_free(map);
>> +    return err;
>> +}
> 
> 

On 01/09/17 18:59, Jorge Ramirez wrote:
> On 09/01/2017 05:44 PM, LongChair . wrote:
>> +
>> +static void drm_unmap_frame(AVHWFramesContext *hwfc,
>> +                            HWMapDescriptor *hwmap)
>> +{
>> +    DRMMapping *map = hwmap->priv;
>> +    int i;
>> +
>> +    for (i = 0; i < map->nb_regions; i++) {
>> +        if (map->address[i])
>> +            munmap(map->address[i], map->length[i]);
> 
> do we need to check the return value of the call?

And do what with it?  close(2)-type functions are easier to regard as void - I don't think there are any non-undefined-behaviour failures possible here.

> also if map->address[i] == NULL,  is that not a warning of something that went wrong?

Yeah, it appears to be impossible - will remove.  (I think that was probably copied from the cleanup unmap in map below.)

>> +    }
>> +
>> +    av_free(map);
>> +}
> 

On 01/09/17 19:01, Jorge Ramirez wrote:
> On 09/01/2017 05:44 PM, LongChair . wrote:
>> +}
>> +
>> +static int drm_device_create(AVHWDeviceContext *hwdev, const char *device,
>> +                             AVDictionary *opts, int flags)
>> +{
>> +    AVDRMDeviceContext *hwctx = hwdev->hwctx;
>> +    drmVersionPtr version;
>> +
>> +    hwctx->fd = open(device, O_RDWR);
>> +    if (hwctx->fd < 0)
>> +        return AVERROR_UNKNOWN;
> return errno?

Yep, fixed to AVERROR(errno).

On 01/09/17 21:05, Jorge Ramirez wrote:
> On 09/01/2017 05:44 PM, LongChair . wrote:
>> + * @file
>> + * API-specific header for AV_HWDEVICE_TYPE_DRM.
>> + *
>> + * Internal frame allocation is not currently supported - all frames
>> + * must be allocated by the user.  Thus AVHWFramesContext is always
>> + * NULL, though this may change if support for frame allocation is
>> + * added in future.
>> + */
>> +
>> +enum {
>> +    /**
>> +     * The maximum number of layers/planes in a DRM frame.
>> +     */
>> +    AV_DRM_MAX_PLANES = 4
>> +};
> 
> why this limit?
> 
> dont you have to call drmModeGetPlaneResources(fd) then use planes->count_planes?

No - these are image planes, not scanout planes.

The limit of four was decided on as the largest number which might be supported by DRM in the future.  Currently there are at most three image planes in any  DRM format (the three-plane YUV types), but four is certainly possible with planar YUVA or RGBA, so it seemed sensible to set four here just in case.

Thanks,

- Mark