Hi Jordan, Thanks for the review. Will incorporate the suggested changes in v2.
On 03/12/2018 08:43 PM, Jordan Crouse wrote:
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.
Its always good to review and remove older code paths :). Sure will remove this check.
- 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'.
yep makes sense, will replace it.
- }
- 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'.
ok will do just that.
Jordan