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 ?
> +++ 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 ?
> +++ 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
