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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
