Re: [PATCH 2/5] drm/msm/dsi: add implementation for helper functions

[Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On Mon, Mar 12, 2018 at 06:53:11PM +0530, Sibi S wrote:
> Add dsi host helper function implementation for DSI v2
> and DSI 6G 1.x controllers
> 
> Signed-off-by: Sibi S <sibis@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.h      |  15 +++
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c  |  44 +++++--
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 250 ++++++++++++++++++++++++++++++++++++-
>  3 files changed, 298 insertions(+), 11 deletions(-)

<snip>
>  static int dsi_calc_clk_rate(struct msm_dsi_host *msm_host)
>  {
>  	struct drm_display_mode *mode = msm_host->mode;
> @@ -1008,6 +1161,59 @@ static void dsi_wait4video_eng_busy(struct msm_dsi_host *msm_host)
>  	}
>  }
>  
> +int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size)
> +{
> +	struct drm_device *dev = msm_host->dev;
> +	struct msm_drm_private *priv = dev->dev_private;
> +	int ret;
> +	uint64_t iova;
> +
> +	msm_host->tx_gem_obj = msm_gem_new(dev, size, MSM_BO_UNCACHED);
> +	if (IS_ERR(msm_host->tx_gem_obj)) {
> +		ret = PTR_ERR(msm_host->tx_gem_obj);
> +		pr_err("%s: failed to allocate gem, %d\n",
> +			__func__, ret);
> +		msm_host->tx_gem_obj = NULL;
> +		return ret;
> +	}
> +
> +	ret = msm_gem_get_iova(msm_host->tx_gem_obj,
> +			priv->kms->aspace, &iova);
> +	mutex_unlock(&dev->struct_mutex);
> +	if (ret) {
> +		pr_err("%s: failed to get iova, %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	if (iova & 0x07) {
> +		pr_err("%s: buf NOT 8 bytes aligned\n", __func__);
> +		return -EINVAL;
> +	}

This is impossible - new allocations will always be page aligned.

> +	msm_host->tx_size = msm_host->tx_gem_obj->size;
> +
> +	return 0;
> +}
> +
> +int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size)
> +{
> +	struct drm_device *dev = msm_host->dev;
> +	int ret;
> +
> +	msm_host->tx_buf = dma_alloc_coherent(dev->dev, size,
> +					&msm_host->tx_buf_paddr, GFP_KERNEL);
> +	if (!msm_host->tx_buf) {
> +		ret = -ENOMEM;
> +		pr_err("%s: failed to allocate tx buf, %d\n",
> +			__func__, ret);

You don't need to print ret here, it isn't a mystery what it is.  In fact, you
probably don't need to print anything here at all because dma_alloc_coherent
should be pretty noisy when it fails.

> +		return ret;

This can just be return -ENOMEM and you can lose 'ret'.

> +	}
> +
> +	msm_host->tx_size = size;
> +
> +	return 0;
> +}
> +
>  /* dsi_cmd */
>  static int dsi_tx_buf_alloc(struct msm_dsi_host *msm_host, int size)
>  {
> @@ -1072,6 +1278,21 @@ static void dsi_tx_buf_free(struct msm_dsi_host *msm_host)
>  			msm_host->tx_buf_paddr);
>  }
>  
> +void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host)
> +{
> +	return msm_gem_get_vaddr(msm_host->tx_gem_obj);
> +}
> +
> +void *dsi_tx_buf_get_v2(struct msm_dsi_host *msm_host)
> +{
> +	return msm_host->tx_buf;
> +}
> +
> +void dsi_tx_buf_put_6g(struct msm_dsi_host *msm_host)
> +{
> +	msm_gem_put_vaddr(msm_host->tx_gem_obj);
> +}
> +
>  /*
>   * prepare cmd buffer to be txed
>   */
> @@ -1173,6 +1394,31 @@ static int dsi_long_read_resp(u8 *buf, const struct mipi_dsi_msg *msg)
>  	return msg->rx_len;
>  }
>  
> +int dsi_dma_base_get_6g(struct msm_dsi_host *msm_host, uint64_t *dma_base)
> +{
> +	struct drm_device *dev = msm_host->dev;
> +	struct msm_drm_private *priv = dev->dev_private;
> +	uint64_t **iova;
> +	int ret;
> +
> +	if (!dma_base)
> +		return -EINVAL;
> +
> +	iova = &dma_base;

This is a convoluted way of passing in the pointer and I doubt even the compiler
can see through it.

> +	ret = msm_gem_get_iova(msm_host->tx_gem_obj,
> +				priv->kms->aspace, *iova);

ret = msm_gem_get_iova(msm_host->tx_gem_obj, priv->kms->aspace, dma_base);

Easy, safe effective 

> +	return ret;

If you put a return on the front of the msm_gem_get_iova you can eliminate the
need for 'ret'.

Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno