Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support

[jilaiw@codeaurora.org]

Thanks Emil. Please check the comments embedded and for the rest I will
update the code.

> Hi Jilai,
>
> Just a few questions, not really a review as I'm not that familiar
> with the code.
>
>
>> +++ b/drivers/gpu/drm/msm/Kconfig
>> @@ -27,6 +27,16 @@ config DRM_MSM_FBDEV
>>           support. Note that this support also provide the linux console
>>           support on top of the MSM modesetting driver.
>>
>> +config DRM_MSM_WB
>> +       bool "Enable writeback support for MSM modesetting driver"
>> +       depends on DRM_MSM
>> +       depends on VIDEO_V4L2
>> +       select VIDEOBUF2_CORE
>> +       default y
>> +       help
>> +         Choose this option if you have a need to support writeback
>> +         connector.
>> +
> Is it worth mentioning which devices/SoCs have such connector ?
If the devices have WB connector, it will be added in the device tree.

>
>
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -1,4 +1,5 @@
>> -ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/msm
>> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/msm
>> -Idrivers/gpu/drm/msm/mdp_wb
>> +ccflags-$(CONFIG_DRM_MSM_WB) += -Idrivers/gpu/drm/msm/mdp/mdp_wb
>>
> I think you only want the second line here.
>
>
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_wb_encoder.c
>
>> +static struct msm_bus_paths mdp_bus_usecases[] = { {
>> +               .num_paths = 1,
>> +               .vectors = &mdp_bus_vectors[0],
>> +}, {
>> +               .num_paths = 1,
>> +               .vectors = &mdp_bus_vectors[1],
>> +} };
> The following formatting seems more common:
>
> static struct foo foo1[] = {
>     {
>         bar
>      }
> };
>
>
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb.c
>
>> +int msm_wb_modeset_init(struct msm_wb *wb,
>> +       struct drm_device *dev, struct drm_encoder *encoder)
>> +{
>> +       struct msm_drm_private *priv = dev->dev_private;
>> +       int ret;
>> +
>> +       wb->dev = dev;
>> +       wb->encoder = encoder;
>> +
>> +       wb->connector = msm_wb_connector_init(wb);
>> +       if (IS_ERR(wb->connector)) {
>> +               ret = PTR_ERR(wb->connector);
>> +               dev_err(dev->dev, "failed to create WB connector: %d\n",
>> ret);
>> +               wb->connector = NULL;
>> +               goto fail;
>> +       }
>> +
>> +       priv->connectors[priv->num_connectors++] = wb->connector;
>> +
>> +       return 0;
>> +
>> +fail:
>> +       if (wb->connector) {
>> +               wb->connector->funcs->destroy(wb->connector);
>> +               wb->connector = NULL;
>> +       }
>> +
> Drop the unused if block ?
>
>
>> +static struct msm_wb *msm_wb_init(struct platform_device *pdev)
>> +{
>> +       struct msm_wb *wb = NULL;
>> +
>> +       wb = devm_kzalloc(&pdev->dev, sizeof(*wb), GFP_KERNEL);
>> +       if (!wb)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       wb->pdev = pdev;
>> +       wb->priv_data = devm_kzalloc(&pdev->dev, sizeof(*wb->priv_data),
>> +               GFP_KERNEL);
>> +       if (!wb->priv_data)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       if (msm_wb_v4l2_init(wb)) {
>> +               pr_err("%s: wb v4l2 init failed\n", __func__);
>> +               return ERR_PTR(-ENODEV);
>> +       }
>> +
> Seems like we're leaking wb and/or wb->priv_data. Add a label and
> consolidate error handling in there ?

Since the devm_kzalloc function is used here, the system should take care
freeing the memory.
>
>
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb.h
>
>> +#ifndef __WB_CONNECTOR_H__
>> +#define __WB_CONNECTOR_H__
>> +
> The file is called mdp_wb.h, so one might want to change this to
> __MDP_WB_H__
>
>
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
>> @@ -0,0 +1,522 @@
>
>> +static const struct msm_wb_fmt *get_format(u32 fourcc)
>> +{
>> +       const struct msm_wb_fmt *fmt;
>> +       unsigned int k;
>> +
>> +       for (k = 0; k < ARRAY_SIZE(formats); k++) {
>> +               fmt = &formats[k];
>> +               if (fmt->fourcc == fourcc)
>> +                       break;
>> +       }
>> +
>> +       if (k == ARRAY_SIZE(formats))
>> +               return NULL;
>> +
>> +       return &formats[k];
> You could move the return within the loop, and drop the follow up
> conditional.
>
>
>> +static void *msm_wb_vb2_attach_dmabuf(void *alloc_ctx, struct dma_buf
>> *dbuf,
>> +       unsigned long size, int write)
>> +{
>> +       struct msm_wb_v4l2_dev *dev = alloc_ctx;
>> +       struct drm_device *drm_dev = dev->wb->dev;
>> +       struct drm_gem_object *obj;
>> +
>> +       mutex_lock(&drm_dev->object_name_lock);
>> +       obj = drm_dev->driver->gem_prime_import(drm_dev, dbuf);
>> +       if (WARN_ON(!obj)) {
>> +               mutex_unlock(&drm_dev->object_name_lock);
>> +               v4l2_err(&dev->v4l2_dev, "Can't convert dmabuf to gem
>> obj.\n");
>> +               return NULL;
> Shouldn't one return ERR_PTR here ? Consolidating the error paths to a
> single label will be cleaner imho.
>
>
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>
>> @@ -224,18 +229,28 @@ struct drm_framebuffer
>> *msm_framebuffer_create(struct drm_device *dev,
>>
>>  struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
>>
>> -struct hdmi;
>>  int hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,
>>                 struct drm_encoder *encoder);
>>  void __init hdmi_register(void);
>>  void __exit hdmi_unregister(void);
>>
>> -struct msm_edp;
> Unrelated cosmetic changes ?
>
>
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -534,6 +534,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>>                 if (msm_obj->pages)
>>                         drm_free_large(msm_obj->pages);
>>
>> +               drm_prime_gem_destroy(obj, msm_obj->sgt);
> Should this fix be a separate patch ?
>
>
> Cheers,
> Emil
>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel