Hello,

On 2015-11-12 16:20, Tobias Jakobi wrote:
> Hello,
>
> Marek Szyprowski wrote:
>> From: Seung-Woo Kim <sw0312.kim at samsung.com>
>>
>> NV12 and YUV420 formats are need to calculate offset of each plane
>> for ipp fimc in a gem buffer. Without proper offset, only Y plane
>> can be processed, so result shows green frame.
>> This patch fixes to calculate offset for cbcr planes for NV12 and
>> YUV420 formats.
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim at samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_fimc.c | 106 
>> +++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/exynos/exynos_drm_ipp.c  |  15 ++++-
>>   drivers/gpu/drm/exynos/exynos_drm_ipp.h  |   2 +
>>   3 files changed, 121 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> index c747824f3c98..72a7ca188be5 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> @@ -403,6 +403,97 @@ static void fimc_handle_lastend(struct fimc_context 
>> *ctx, bool enable)
>>      fimc_write(ctx, cfg, EXYNOS_CIOCTRL);
>>   }
>>   
>> +static int fimc_set_planar_addr(struct drm_exynos_ipp_buf_info *buf_info,
>> +                            u32 fmt, struct drm_exynos_sz *sz)
>> +{
>> +    dma_addr_t *base[EXYNOS_DRM_PLANAR_MAX];
> I think I've seen this at several other places already, but I never
> fully understood it.
>
> Why are we using dma_addr_t* here, instead of just dma_addr_t? I mean in
> the following code the pointer is just in the dereferenced form anyway,
> so why this added indirection?

This function updates dma addresses stored in struct drm_exynos_ipp_buf_info
*buf_info (it adds offset related to specific pixel format), so using a 
pointer
makes all formulas shorter.

>> +    uint64_t size[EXYNOS_DRM_PLANAR_MAX];
>> +    uint64_t ofs[EXYNOS_DRM_PLANAR_MAX];
>> +    bool bypass = false;
>> +    uint64_t tsize = 0;
>> +    int i;
>> +
>> +    for_each_ipp_planar(i) {
>> +            base[i] = &buf_info->base[i];
>> +            size[i] = buf_info->size[i];
>> +            ofs[i] = 0;
>> +            tsize += size[i];
>> +    }
>> +
>> +    if (!tsize) {
>> +            DRM_INFO("%s:failed to get buffer size.\n", __func__);
>> +            return 0;
>> +    }
>> +
>> +    switch (fmt) {
>> +    case DRM_FORMAT_NV12:
>> +    case DRM_FORMAT_NV21:
>> +    case DRM_FORMAT_NV16:
>> +    case DRM_FORMAT_NV61:
>> +            ofs[0] = sz->hsize * sz->vsize;
>> +            ofs[1] = ofs[0] >> 1;
>> +            if (*base[0] && *base[1]) {
>> +                    if (size[0] + size[1] < ofs[0] + ofs[1])
>> +                            goto err_info;
>> +                    bypass = true;
>> +            }
>> +            break;
>> +    case DRM_FORMAT_YUV410:
>> +    case DRM_FORMAT_YVU410:
>> +    case DRM_FORMAT_YUV411:
>> +    case DRM_FORMAT_YVU411:
>> +    case DRM_FORMAT_YUV420:
>> +    case DRM_FORMAT_YVU420:
>> +    case DRM_FORMAT_YUV422:
>> +    case DRM_FORMAT_YVU422:
>> +    case DRM_FORMAT_YUV444:
>> +    case DRM_FORMAT_YVU444:
>> +            ofs[0] = sz->hsize * sz->vsize;
>> +            ofs[1] = ofs[2] = ofs[0] >> 2;
>> +            if (*base[0] && *base[1] && *base[2]) {
>> +                    if (size[0]+size[1]+size[2] < ofs[0]+ofs[1]+ofs[2])
>> +                            goto err_info;
>> +                    bypass = true;
>> +            }
>> +            break;
>> +    case DRM_FORMAT_XRGB8888:
>> +    case DRM_FORMAT_ARGB8888:
>> +            ofs[0] = sz->hsize * sz->vsize << 2;
>> +            if (*base[0]) {
>> +                    if (size[0] < ofs[0])
>> +                            goto err_info;
>> +            }
>> +            bypass = true;
>> +            break;
>> +    default:
>> +            bypass = true;
>> +            break;
>> +    }
>> +
>> +    if (!bypass) {
>> +            *base[1] = *base[0] + ofs[0];
>> +            if (ofs[1] && ofs[2])
>> +                    *base[2] = *base[1] + ofs[1];
>> +    }
>> +
>> +    DRM_DEBUG_KMS("%s:y[0x%x],cb[0x%x],cr[0x%x]\n", __func__,
>> +            *base[0], *base[1], *base[2]);
>> +
>> +    return 0;
>> +
>> +err_info:
>> +    DRM_ERROR("invalid size for fmt[0x%x]\n", fmt);
>> +
>> +    for_each_ipp_planar(i) {
>> +            base[i] = &buf_info->base[i];
>> +            size[i] = buf_info->size[i];
>> +
>> +            DRM_ERROR("buf[%d] - base[0x%x] sz[%llu] ofs[%llu]\n",
>> +                    i, *base[i], size[i], ofs[i]);
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>>   
>>   static int fimc_src_set_fmt_order(struct fimc_context *ctx, u32 fmt)
>>   {
>> @@ -689,6 +780,7 @@ static int fimc_src_set_addr(struct device *dev,
>>      struct drm_exynos_ipp_cmd_node *c_node = ippdrv->c_node;
>>      struct drm_exynos_ipp_property *property;
>>      struct drm_exynos_ipp_config *config;
>> +    int ret;
>>   
>>      if (!c_node) {
>>              DRM_ERROR("failed to get c_node.\n");
>> @@ -709,6 +801,12 @@ static int fimc_src_set_addr(struct device *dev,
>>      switch (buf_type) {
>>      case IPP_BUF_ENQUEUE:
>>              config = &property->config[EXYNOS_DRM_OPS_SRC];
>> +            ret = fimc_set_planar_addr(buf_info, config->fmt, &config->sz);
>> +            if (ret) {
>> +                    dev_err(dev, "failed to set plane src addr.\n");
>> +                    return ret;
>> +            }
>> +
>>              fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_Y],
>>                      EXYNOS_CIIYSA0);
>>   
>> @@ -1148,6 +1246,7 @@ static int fimc_dst_set_addr(struct device *dev,
>>      struct drm_exynos_ipp_cmd_node *c_node = ippdrv->c_node;
>>      struct drm_exynos_ipp_property *property;
>>      struct drm_exynos_ipp_config *config;
>> +    int ret;
>>   
>>      if (!c_node) {
>>              DRM_ERROR("failed to get c_node.\n");
>> @@ -1168,6 +1267,11 @@ static int fimc_dst_set_addr(struct device *dev,
>>      switch (buf_type) {
>>      case IPP_BUF_ENQUEUE:
>>              config = &property->config[EXYNOS_DRM_OPS_DST];
>> +            ret = fimc_set_planar_addr(buf_info, config->fmt, &config->sz);
>> +            if (ret) {
>> +                    dev_err(dev, "failed to set plane dst addr.\n");
>> +                    return ret;
>> +            }
>>   
>>              fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_Y],
>>                      EXYNOS_CIOYSA(buf_id));
>> @@ -1562,6 +1666,8 @@ static void fimc_ippdrv_stop(struct device *dev, enum 
>> drm_exynos_ipp_cmd cmd)
>>      /* reset sequence */
>>      fimc_write(ctx, 0x0, EXYNOS_CIFCNTSEQ);
>>   
>> +    fimc_clear_addr(ctx);
>> +
>>      /* Scaler disable */
>>      fimc_clear_bits(ctx, EXYNOS_CISCCTRL, EXYNOS_CISCCTRL_SCALERSTART);
>>   
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> index 67d24236e745..408a14a9a180 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> @@ -571,6 +571,7 @@ static struct drm_exynos_ipp_mem_node
>>              /* get dma address by handle */
>>              if (qbuf->handle[i]) {
>>                      dma_addr_t *addr;
>> +                    unsigned long size;
>>   
>>                      addr = exynos_drm_gem_get_dma_addr(drm_dev,
>>                                      qbuf->handle[i], c_node->filp);
>> @@ -580,10 +581,20 @@ static struct drm_exynos_ipp_mem_node
>>                              return ERR_PTR(-EFAULT);
>>                      }
>>   
>> +                    size = exynos_drm_gem_get_size(drm_dev,
>> +                                    qbuf->handle[i], c_node->filp);
>> +                    if (!size) {
>> +                            DRM_ERROR("failed to get size.\n");
>> +                            ipp_put_mem_node(drm_dev, c_node, m_node);
>> +                            return ERR_PTR(-EFAULT);
>> +                    }
>> +
>>                      buf_info->handles[i] = qbuf->handle[i];
>>                      buf_info->base[i] = *addr;
>> -                    DRM_DEBUG_KMS("i[%d]base[0x%x]hd[0x%lx]\n", i,
>> -                                  buf_info->base[i], buf_info->handles[i]);
>> +                    buf_info->size[i] = (uint64_t)size;
>> +                    DRM_DEBUG_KMS("i[%d]base[%pad]hd[0x%lx]sz[%llx]\n", i,
>> +                                  &buf_info->base[i], buf_info->handles[i],
>> +                                  buf_info->size[i]);
>>              }
>>      }
>>   
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h 
>> b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>> index 2a61547a39d0..d4f0b588220b 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>> @@ -85,10 +85,12 @@ struct drm_exynos_ipp_cmd_node {
>>    *
>>    * @handles: Y, Cb, Cr each gem object handle.
>>    * @base: Y, Cb, Cr each planar address.
>> + * @size: Y, Cb, Cr each planar size.
>>    */
>>   struct drm_exynos_ipp_buf_info {
>>      unsigned long   handles[EXYNOS_DRM_PLANAR_MAX];
>>      dma_addr_t      base[EXYNOS_DRM_PLANAR_MAX];
>> +    uint64_t        size[EXYNOS_DRM_PLANAR_MAX];
>>   };
>>   
>>   /*
>>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Reply via email to