Re: [Freedreno] [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

2018-04-13 Thread abhinavk 

Hi Sean

Thanks for the review.

Reply inline.

On 2018-04-13 13:26, Sean Paul wrote:

On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:

Make sure the video mode engine is on before waiting
for the video done interrupt.

Otherwise it leads to silent timeouts increasing display
turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c

index 7a03a94..5b7b290 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -173,6 +173,7 @@ struct msm_dsi_host {

bool registered;
bool power_on;
+   bool enabled;
int irq;
 };

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int mode, 
struct msm_dsi_host *msm_host)


 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+   u32 ret = 0;
+   struct device *dev = _host->pdev->dev;
+
dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);

reinit_completion(_host->video_comp);

-   wait_for_completion_timeout(_host->video_comp,
+   ret = wait_for_completion_timeout(_host->video_comp,
msecs_to_jiffies(70));

+   if (ret <= 0)
+   dev_err(dev, "wait for video done timed out\n");
+
dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct 
msm_dsi_host *msm_host)

if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
return;

-   if (msm_host->power_on) {
+   if (msm_host->power_on && msm_host->enabled) {
dsi_wait4video_done(msm_host);
/* delay 4 ms to skip BLLP */
usleep_range(2000, 4000);
@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host 
*host)

 *  pm_runtime_put_autosuspend(_host->pdev->dev);
 * }
 */
-
+   msm_host->enabled = true;
return 0;
 }

@@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct mipi_dsi_host 
*host)

 * Reset to disable video engine so that we can send off cmd.
 */
dsi_sw_reset(msm_host);
-
+   msm_host->enabled = false;


This should go at the start of the function. Also, it's unclear from 
this patch,

but I assume this is protected by a lock?

Sean

[Abhinav] Yes, will move this to the start.
No, there is no lock here but at this point doesnt need one.
The reason is that, this variable will be written to and read by the 
same process

(suspend thread OR resume thread which sends the panel ON/OFF commands).
If we decide to expose other interfaces to send commands like debugfs or 
sysfs and

introduce more threads, we will add the locking.




return 0;
 }

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH 1/2] drm/msm/dsi: adjust dsi timing for dual dsi mode

2018-04-13 Thread Sean Paul 
On Tue, Apr 10, 2018 at 06:16:07PM -0700, Chandan Uddaraju wrote:
> For dual dsi mode, the horizontal timing needs
> to be divided by half since both the dsi controllers
> will be driving this panel. Adjust the pixel clock and
> DSI timing accordingly.
> 
> Change-Id: Iee1226b2eef9eea23d9653e3d738ee8cd2a2dd8e
> Signed-off-by: Chandan Uddaraju 
> ---
>  drivers/gpu/drm/msm/dsi/dsi.h |  1 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c| 17 +
>  drivers/gpu/drm/msm/dsi/dsi_manager.c | 15 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 70d9a9a..4131b47 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -161,6 +161,7 @@ void msm_dsi_host_cmd_xfer_commit(struct mipi_dsi_host 
> *host,
>   u32 dma_base, u32 len);
>  int msm_dsi_host_enable(struct mipi_dsi_host *host);
>  int msm_dsi_host_disable(struct mipi_dsi_host *host);
> +void msm_dsi_host_adjust_timing_config(struct mipi_dsi_host *host);
>  int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>   struct msm_dsi_phy_shared_timings *phy_shared_timings);
>  int msm_dsi_host_power_off(struct mipi_dsi_host *host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 7a03a94..66a21cb 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -2237,6 +2237,23 @@ static void msm_dsi_sfpb_config(struct msm_dsi_host 
> *msm_host, bool enable)
>   SFPB_GPREG_MASTER_PORT_EN(en));
>  }
>  
> +void msm_dsi_host_adjust_timing_config(struct mipi_dsi_host *host)
> +{
> + struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> + struct drm_display_mode *mode = NULL;
> +
> + mode = msm_host->mode;
> +
> + mutex_lock(_host->dev_mutex);
> + mode->htotal >>= 1;
> + mode->hdisplay >>= 1;
> + mode->hsync_start >>= 1;
> + mode->hsync_end >>= 1;
> +
> + mode->clock >>= 1;

I don't think you should alter the mode. Instead, apply the division when you're
writing out to hardware. Also, no need to get fancy with a bitshift, just use /2
and trust that the compiler is smart :)

Sean

> + mutex_unlock(_host->dev_mutex);
> +}
> +
>  int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>   struct msm_dsi_phy_shared_timings *phy_shared_timings)
>  {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 4cb1cb6..8ef1c3d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -627,6 +627,21 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge 
> *bridge,
>   msm_dsi_host_set_display_mode(host, adjusted_mode);
>   if (is_dual_dsi && other_dsi)
>   msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
> +
> + /*
> +  * For dual DSI mode, the current DRM mode has
> +  * the complete width of the panel. Since, the complete
> +  * panel is driven by two DSI controllers, the
> +  * horizontal timings and the pixel clock have to be
> +  * split between the two dsi controllers. Adjust the
> +  * DSI host timing structures accordingly.
> +  */
> + if (is_dual_dsi) {
> + msm_dsi_host_adjust_timing_config(host);
> + if (other_dsi)
> + msm_dsi_host_adjust_timing_config(other_dsi->host);
> + }
> +
>  }
>  
>  static const struct drm_connector_funcs dsi_mgr_connector_funcs = {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [[RFC]DPU PATCH 1/2] drm/bridge: add support for sn65dsi86 bridge driver

2018-04-13 Thread Sean Paul 
On Fri, Apr 13, 2018 at 10:53:00AM +0530, Sandeep Panda wrote:
> Add support for TI's sn65dsi86 dsi2edp bridge chip.
> The chip converts DSI transmitted signal to eDP signal,
> which is fed to the connected eDP panel.
> 
> This chip can be controlled via either i2c interface or
> dsi interface. Currently in driver all the control registers
> are being accessed through i2c interface only.
> Also as of now HPD support has not been added to bridge
> chip driver.
> 
> Signed-off-by: Sandeep Panda 

Hi Sandeep,
Thank you for your patch!

> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1019 
> +
>  1 file changed, 1019 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> new file mode 100644
> index 000..93aa1ad
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -0,0 +1,1019 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__

Instead of using pr_* for logging, please use the DRM_* variants. Since there
is unlikely to be more than one of these bridge drivers in a system, you won't
need to use the DRM_DEV_* versions.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SN_DEVICE_REV_REG 0x08
> +
> +/* Link Training specific registers */
> +#define SN_REFCLK_FREQ_REG 0x0A
> +#define SN_DSI_LANES_REG 0x10
> +#define SN_DSIA_CLK_FREQ_REG 0x12
> +#define SN_ENH_FRAME_REG 0x5A
> +#define SN_SSC_CONFIG_REG 0x93
> +#define SN_DATARATE_CONFIG_REG 0x94
> +#define SN_PLL_ENABLE_REG 0x0D
> +#define SN_SCRAMBLE_CONFIG_REG 0x95
> +#define SN_AUX_WDATA0_REG 0x64
> +#define SN_AUX_ADDR_19_16_REG 0x74
> +#define SN_AUX_ADDR_15_8_REG 0x75
> +#define SN_AUX_ADDR_7_0_REG 0x76
> +#define SN_AUX_LENGTH_REG 0x77
> +#define SN_AUX_CMD_REG 0x78
> +#define SN_ML_TX_MODE_REG 0x96
> +#define SN_PAGE_SELECT_REG 0xFF
> +#define SN_AUX_RDATA0_REG 0x79
> +
> +/* video config specific registers */
> +#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG 0x20
> +#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG 0x21
> +#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG 0x24
> +#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG 0x25
> +#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG 0x2C
> +#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG 0x2D
> +#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG 0x30
> +#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG 0x31
> +#define SN_CHA_HORIZONTAL_BACK_PORCH_REG 0x34
> +#define SN_CHA_VERTICAL_BACK_PORCH_REG 0x36
> +#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG 0x38
> +#define SN_CHA_VERTICAL_FRONT_PORCH_REG 0x3A
> +#define SN_DATA_FORMAT_REG 0x5B
> +#define SN_COLOR_BAR_CONFIG_REG 0x3C

It'd be nice if you could tab-align the values.

> +
> +struct sn65dsi86_reg_cfg {
> + u8 reg;
> + u8 val;
> + int sleep_in_ms;
> +};
> +
> +struct sn65dsi86_video_cfg {
> + u32 h_active;
> + u32 h_front_porch;
> + u32 h_pulse_width;
> + u32 h_back_porch;
> + bool h_polarity;
> + u32 v_active;
> + u32 v_front_porch;
> + u32 v_pulse_width;
> + u32 v_back_porch;
> + bool v_polarity;
> + u32 pclk_khz;
> + u32 num_of_lanes;
> +};

All of these can be derived from drm_display_mode except for num_of_lanes which
is hardcoded. So let's just use drm_display_mode directly.

> +
> +struct sn65dsi86_gpios {
> + struct gpio_desc *irq_gpio;
> + struct gpio_desc *enable_gpio;
> + struct gpio_desc *aux_i2c_sda;
> + struct gpio_desc *aux_i2c_scl;
> + struct gpio_desc *edp_bias_en;
> + struct gpio_desc *edp_bklt_en;
> + struct gpio_desc *edp_bklt_ctrl;
> +};

I see IRQ and EN in the datasheet, but not the others. It seems like the aux_*
and edp_* gpios are always equal to en. So if you actually need them, don't
specify a new struct, just add irq_gpio to the main struct and add an array of
en_gpios to handle the rest.

> +
> +struct sn65dsi86 {

I will have fits trying to type this name. Could you please use something
simple, like sn_bridge? Same comment applies to all of the function names.

> + struct device *dev;
> + struct drm_bridge bridge;
> + struct drm_connector connector;
> +
> + struct device_node *host_node;
> + struct mipi_dsi_device *dsi;
> +
> + u8 i2c_addr;

Unused

> + int irq;
> +
> + struct sn65dsi86_gpios gpios;
> +
> + unsigned int num_supplies;
> + struct regulator_bulk_data *supplies;
> +
> + struct i2c_client *i2c_client;
> +
> + enum drm_connector_status connector_status;

You never use the value of this, you just assign it. So you can remove this.

> + bool power_on;
> +
> + bool is_pluggable;

As I mentioned in the dt review, you can determine this via panel. You should
also take this into

Re: [Freedreno] [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

2018-04-13 Thread Sean Paul 
On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
> Make sure the video mode engine is on before waiting
> for the video done interrupt.
> 
> Otherwise it leads to silent timeouts increasing display
> turn ON time.
> 
> Changes in v2:
> - Replace pr_err with dev_err
> - Changed error message
> 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 7a03a94..5b7b290 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -173,6 +173,7 @@ struct msm_dsi_host {
>  
>   bool registered;
>   bool power_on;
> + bool enabled;
>   int irq;
>  };
>  
> @@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int mode, struct 
> msm_dsi_host *msm_host)
>  
>  static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
>  {
> + u32 ret = 0;
> + struct device *dev = _host->pdev->dev;
> +
>   dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
>  
>   reinit_completion(_host->video_comp);
>  
> - wait_for_completion_timeout(_host->video_comp,
> + ret = wait_for_completion_timeout(_host->video_comp,
>   msecs_to_jiffies(70));
>  
> + if (ret <= 0)
> + dev_err(dev, "wait for video done timed out\n");
> +
>   dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
>  }
>  
> @@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct msm_dsi_host 
> *msm_host)
>   if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
>   return;
>  
> - if (msm_host->power_on) {
> + if (msm_host->power_on && msm_host->enabled) {
>   dsi_wait4video_done(msm_host);
>   /* delay 4 ms to skip BLLP */
>   usleep_range(2000, 4000);
> @@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host *host)
>*  pm_runtime_put_autosuspend(_host->pdev->dev);
>* }
>*/
> -
> + msm_host->enabled = true;
>   return 0;
>  }
>  
> @@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct mipi_dsi_host *host)
>* Reset to disable video engine so that we can send off cmd.
>*/
>   dsi_sw_reset(msm_host);
> -
> + msm_host->enabled = false;

This should go at the start of the function. Also, it's unclear from this patch,
but I assume this is protected by a lock?

Sean


>   return 0;
>  }
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH 2/2] drm/msm/dsi: Use one connector for dual DSI mode

2018-04-13 Thread Sean Paul 
On Tue, Apr 10, 2018 at 06:16:08PM -0700, Chandan Uddaraju wrote:
> Current DSI driver uses two connectors for dual DSI case even
> though we only have one panel. Fix this by implementing one
> connector/bridge for dual DSI use case. Use master DSI
> controllers to register one connector/bridge.
> 
> Change-Id: I067b39f3b32eb3aa92d4155d4ca703ca7690645b

Please strip Change-Id when sending patches upstream.

> Signed-off-by: Chandan Uddaraju 
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c |   3 +
>  drivers/gpu/drm/msm/dsi/dsi.h |   1 +
>  drivers/gpu/drm/msm/dsi/dsi_manager.c | 110 
> --
>  3 files changed, 29 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index b744bcc..ff8164c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -208,6 +208,9 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct 
> drm_device *dev,
>   goto fail;
>   }
>  
> + if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
> + goto fail;
> +
>   msm_dsi->encoder = encoder;
>  
>   msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 4131b47..d487d94 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -100,6 +100,7 @@ struct msm_dsi {
>  void msm_dsi_manager_attach_dsi_device(int id, u32 device_flags);
>  int msm_dsi_manager_register(struct msm_dsi *msm_dsi);
>  void msm_dsi_manager_unregister(struct msm_dsi *msm_dsi);
> +bool msm_dsi_manager_validate_current_config(u8 id);
>  
>  /* msm dsi */
>  static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 8ef1c3d..5817f59 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -305,67 +305,6 @@ static void dsi_mgr_connector_destroy(struct 
> drm_connector *connector)
>   kfree(dsi_connector);
>  }
>  
> -static void dsi_dual_connector_fix_modes(struct drm_connector *connector)
> -{
> - struct drm_display_mode *mode, *m;
> -
> - /* Only support left-right mode */
> - list_for_each_entry_safe(mode, m, >probed_modes, head) {
> - mode->clock >>= 1;
> - mode->hdisplay >>= 1;
> - mode->hsync_start >>= 1;
> - mode->hsync_end >>= 1;
> - mode->htotal >>= 1;
> - drm_mode_set_name(mode);
> - }
> -}
> -
> -static int dsi_dual_connector_tile_init(
> - struct drm_connector *connector, int id)
> -{
> - struct drm_display_mode *mode;
> - /* Fake topology id */
> - char topo_id[8] = {'M', 'S', 'M', 'D', 'U', 'D', 'S', 'I'};
> -
> - if (connector->tile_group) {
> - DBG("Tile property has been initialized");
> - return 0;
> - }
> -
> - /* Use the first mode only for now */
> - mode = list_first_entry(>probed_modes,
> - struct drm_display_mode,
> - head);
> - if (!mode)
> - return -EINVAL;
> -
> - connector->tile_group = drm_mode_get_tile_group(
> - connector->dev, topo_id);
> - if (!connector->tile_group)
> - connector->tile_group = drm_mode_create_tile_group(
> - connector->dev, topo_id);
> - if (!connector->tile_group) {
> - pr_err("%s: failed to create tile group\n", __func__);
> - return -ENOMEM;
> - }
> -
> - connector->has_tile = true;
> - connector->tile_is_single_monitor = true;
> -
> - /* mode has been fixed */
> - connector->tile_h_size = mode->hdisplay;
> - connector->tile_v_size = mode->vdisplay;
> -
> - /* Only support left-right mode */
> - connector->num_h_tile = 2;
> - connector->num_v_tile = 1;
> -
> - connector->tile_v_loc = 0;
> - connector->tile_h_loc = (id == DSI_RIGHT) ? 1 : 0;
> -
> - return 0;
> -}
> -
>  static int dsi_mgr_connector_get_modes(struct drm_connector *connector)
>  {
>   int id = dsi_mgr_connector_get_id(connector);
> @@ -376,31 +315,15 @@ static int dsi_mgr_connector_get_modes(struct 
> drm_connector *connector)
>   if (!panel)
>   return 0;
>  
> - /* Since we have 2 connectors, but only 1 drm_panel in dual DSI mode,
> -  * panel should not attach to any connector.
> -  * Only temporarily attach panel to the current connector here,
> -  * to let panel set mode to this connector.
> + /*
> +  * In dual DSI mode, we have one connector that can be
> +  * attached to the drm_panel.
>*/
>   drm_panel_attach(panel, connector);
>   num = drm_panel_get_modes(panel);
> - drm_panel_detach(panel);
>   if

Re: [Freedreno] [DPU PATCH 2/2] drm/panel: add backlight control support for truly panel

2018-04-13 Thread Sean Paul 
On Sat, Apr 07, 2018 at 12:06:53AM -0700, Abhinav Kumar wrote:
> Register truly panel as a backlight led device and
> provide methods to control its backlight operation.
> 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/panel/panel-truly-dual-dsi.c | 96 
> +++-
>  1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c 
> b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> index 47891ee..5d0ef90 100644
> --- a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

Includes should be alphabetical.

>  
>  #include 
>  #include 
> @@ -23,6 +24,9 @@
>  #include 
>  #include 
>  
> +#define BL_NODE_NAME_SIZE 32
> +#define PRIM_DISPLAY_NODE 0
> +
>  struct truly_wqxga {
>   struct device *dev;
>   struct drm_panel panel;
> @@ -33,6 +37,8 @@ struct truly_wqxga {
>   struct gpio_desc *mode_gpio;
>  
>   struct backlight_device *backlight;
> + /* WLED params */
> + struct led_trigger *wled;
>   struct videomode vm;
>  
>   struct mipi_dsi_device *dsi[2];
> @@ -447,6 +453,83 @@ static void truly_wqxga_panel_del(struct truly_wqxga 
> *ctx)
>   put_device(>dsi[1]->dev);
>  }
>  
> +static int truly_backlight_device_update_status(struct backlight_device *bd)
> +{
> + int brightness;
> + int max_brightness;
> + int rc = 0;
> +
extra line

> + struct truly_wqxga *ctx = dev_get_drvdata(>dev);
> +
> + brightness = bd->props.brightness;
> + max_brightness = bd->props.max_brightness;
> +
> + if ((bd->props.power != FB_BLANK_UNBLANK) ||
> + (bd->props.state & BL_CORE_FBBLANK) ||
> +   (bd->props.state & BL_CORE_SUSPENDED))
> + brightness = 0;
> +
> + if (brightness > max_brightness)
> + brightness = max_brightness;
> +
> + /* Need to check WLED driver capability upstream */
> + if (ctx && ctx->wled)

ctx can't be NULL, so no need to check for that. And if ctx->wled is null, it
doesn't seem like this function will do anything. So how about just not
registering the backlight if wled == NULL (if that's possible).

> + led_trigger_event(ctx->wled, brightness);
> +
> + return rc;
> +}
> +
> +static int truly_backlight_device_get_brightness(struct backlight_device *bd)
> +{
> + return bd->props.brightness;
> +}
> +
> +static const struct backlight_ops truly_backlight_device_ops = {
> + .update_status = truly_backlight_device_update_status,
> + .get_brightness = truly_backlight_device_get_brightness,
> +};
> +
> +static int truly_backlight_setup(struct truly_wqxga *ctx)
> +{
> + struct backlight_properties props;
> + char bl_node_name[BL_NODE_NAME_SIZE];
> +
> + if (!ctx) {
> + dev_err(ctx->dev, "invalid context\n");
> + return -EINVAL;
> + }

This can't happen.

> +
> + if (!ctx->backlight) {
> + memset(, 0, sizeof(props));
> + props.type = BACKLIGHT_RAW;
> + props.power = FB_BLANK_UNBLANK;
> + props.max_brightness = 4096;
> +
> + snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
> +  PRIM_DISPLAY_NODE);

Given that PRIM_DISPLAY_NODE is always 0, this seems like overkill for a pretty
generic name "panel0-backlight". So let's just call it "truly_backlight" in the
register call.

> +
> + ctx->backlight =  backlight_device_register(bl_node_name,
> + ctx->dev, ctx,
> + _backlight_device_ops, );
> +
> + if (IS_ERR_OR_NULL(ctx->backlight)) {
> + pr_err("Failed to register backlight\n");
> + ctx->backlight = NULL;
> + return -ENODEV;
> + }
> +
> + /* Register with the LED driver interface */
> + led_trigger_register_simple("bkl-trigger", >wled);
> +
> + if (!ctx->wled) {
> + pr_err("backlight led registration failed\n");
> + return -ENODEV;
> + }
> + }
> +
> + return 0;
> +}
> +
>  static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  {
>   struct device *dev = >dev;
> @@ -466,10 +549,11 @@ static int truly_wqxga_probe(struct mipi_dsi_device 
> *dsi)
>   secondary = of_find_mipi_dsi_device_by_node(np);
>   of_node_put(np);
>  
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +

Why move this?

>   if (!secondary)
>   return -EPROBE_DEFER;
>  
> - ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>   if (!ctx) {
>   put_device(>dev);
>   return -ENOMEM;
> @@ -485,6 +569,12 @@ static int truly_wqxga_probe(struct

Re: [Freedreno] [DPU PATCH v2 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

2018-04-13 Thread abhinavk 

On 2018-04-13 13:29, Sean Paul wrote:

On Tue, Apr 10, 2018 at 06:54:07PM -0700, Abhinav Kumar wrote:

Currently the DSI PHY timings are hard-coded for a specific panel
for the 10nm PHY.

Replace this with the auto PHY timing calculator which can calculate
the PHY timings for any panel.


Any chance you could document what you're doing so anyone without 
documentation

has a clue what's going on?

Sean

I am afraid it will hard to document more about this function other than 
whats mentioned here.
Basically, we have an excel sheet which does this math to calculate the 
DSI timings.

This patch implements that excel sheet for SDM845 which uses 10nm PHY.
We will not be able to explain the math in more detail.


Changes in v2:
- None

Reviewed-by: Archit Taneja 
Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c  | 111 
+

 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h  |   2 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c |  28 
 3 files changed, 113 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c

index 8e9d5c2..5b42885 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -265,6 +265,117 @@ int msm_dsi_dphy_timing_calc_v2(struct 
msm_dsi_dphy_timing *timing,

return 0;
 }

+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+  struct msm_dsi_phy_clk_request *clk_req)
+{
+   const unsigned long bit_rate = clk_req->bitclk_rate;
+   const unsigned long esc_rate = clk_req->escclk_rate;
+   s32 ui, ui_x8, lpx;
+   s32 tmax, tmin;
+   s32 pcnt0 = 50;
+   s32 pcnt1 = 50;
+   s32 pcnt2 = 10;
+   s32 pcnt3 = 30;
+   s32 pcnt4 = 10;
+   s32 pcnt5 = 2;
+   s32 coeff = 1000; /* Precision, should avoid overflow */
+   s32 hb_en, hb_en_ckln;
+   s32 temp;
+
+   if (!bit_rate || !esc_rate)
+   return -EINVAL;
+
+   timing->hs_halfbyte_en = 0;
+   hb_en = 0;
+   timing->hs_halfbyte_en_ckln = 0;
+   hb_en_ckln = 0;
+
+   ui = mult_frac(NSEC_PER_MSEC, coeff, bit_rate / 1000);
+   ui_x8 = ui << 3;
+   lpx = mult_frac(NSEC_PER_MSEC, coeff, esc_rate / 1000);
+
+   temp = S_DIV_ROUND_UP(38 * coeff, ui_x8);
+   tmin = max_t(s32, temp, 0);
+   temp = (95 * coeff) / ui_x8;
+   tmax = max_t(s32, temp, 0);
+   timing->clk_prepare = linear_inter(tmax, tmin, pcnt0, 0, false);
+
+
+   temp = 300 * coeff - (timing->clk_prepare << 3) * ui;
+   tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+   tmax = (tmin > 255) ? 511 : 255;
+   timing->clk_zero = linear_inter(tmax, tmin, pcnt5, 0, false);
+
+   tmin = DIV_ROUND_UP(60 * coeff + 3 * ui, ui_x8);
+   temp = 105 * coeff + 12 * ui - 20 * coeff;
+   tmax = (temp + 3 * ui) / ui_x8;
+   timing->clk_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+   temp = S_DIV_ROUND_UP(40 * coeff + 4 * ui, ui_x8);
+   tmin = max_t(s32, temp, 0);
+   temp = (85 * coeff + 6 * ui) / ui_x8;
+   tmax = max_t(s32, temp, 0);
+   timing->hs_prepare = linear_inter(tmax, tmin, pcnt1, 0, false);
+
+   temp = 145 * coeff + 10 * ui - (timing->hs_prepare << 3) * ui;
+   tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+   tmax = 255;
+   timing->hs_zero = linear_inter(tmax, tmin, pcnt4, 0, false);
+
+   tmin = DIV_ROUND_UP(60 * coeff + 4 * ui, ui_x8) - 1;
+   temp = 105 * coeff + 12 * ui - 20 * coeff;
+   tmax = (temp / ui_x8) - 1;
+   timing->hs_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+   temp = 50 * coeff + ((hb_en << 2) - 8) * ui;
+   timing->hs_rqst = S_DIV_ROUND_UP(temp, ui_x8);
+
+   tmin = DIV_ROUND_UP(100 * coeff, ui_x8) - 1;
+   tmax = 255;
+   timing->hs_exit = linear_inter(tmax, tmin, pcnt2, 0, false);
+
+   temp = 50 * coeff + ((hb_en_ckln << 2) - 8) * ui;
+   timing->hs_rqst_ckln = S_DIV_ROUND_UP(temp, ui_x8);
+
+   temp = 60 * coeff + 52 * ui - 43 * ui;
+   tmin = DIV_ROUND_UP(temp, ui_x8) - 1;
+   tmax = 63;
+   timing->shared_timings.clk_post =
+   linear_inter(tmax, tmin, pcnt2, 0, false);
+
+   temp = 8 * ui + (timing->clk_prepare << 3) * ui;
+   temp += (((timing->clk_zero + 3) << 3) + 11) * ui;
+   temp += hb_en_ckln ? (((timing->hs_rqst_ckln << 3) + 4) * ui) :
+   (((timing->hs_rqst_ckln << 3) + 8) * ui);
+   tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+   tmax = 63;
+   if (tmin > tmax) {
+   temp = linear_inter(tmax << 1, tmin, pcnt2, 0, false);
+   timing->shared_timings.clk_pre = temp >> 1;
+   timing->shared_timings.clk_pre_inc_by_2 = 1;
+   } else {
+   timing->shared_timings.clk_pre =
+   linear_inter(tmax, tmin,

Re: [Freedreno] [[RFC]DPU PATCH 2/2] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

2018-04-13 Thread Sean Paul 
On Fri, Apr 13, 2018 at 1:23 AM Sandeep Panda  wrote:

> Document the bindings used for the sn65dsi86 DSI to eDP bridge.

> Signed-off-by: Sandeep Panda 
> ---
>   .../bindings/display/bridge/ti,sn65dsi86.txt   | 75
++
>   1 file changed, 75 insertions(+)
>   create mode 100644
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt

> diff --git
a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> new file mode 100644
> index 000..cbd2f0e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> @@ -0,0 +1,75 @@
> +SN65DSI86 DSI to eDP bridge chip
> +
> +
> +This is the binding for Texas Instruments SN65DSI86 bridge.
> +
> +Required properties:
> +- compatible: Must be "ti,sn65dsi86"
> +- reg: i2c address of the chip, 0x2d as per datasheet
> +- enable-gpios: OF device-tree gpio specification for EDP_BRIJ_EN pin
> +- aux-sda-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SDA
pin of AUX channel
> +- aux-scl-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SCL
pin of AUX channel
> +- bias-en-gpios: OF device-tree gpio specification for EN_PP3300_DX_EDP
pin to


PP3300?


> +   enable 3.3V supply to eDP connector
> +- bklt-en-gpios: OF device-tree gpio specification for AP_EDP_BKLTEN pin


I don't see any of these pins listed in the SN65DSI86 datasheet. Can you
please use the actual pin names?


> +
> +- vccio-supply: A 1.8V supply that powers up the PHY.
> +- vcca-supply: A 1.2V supply that powers up the Controller.
> +- vccs-supply: A 3.3V supply that power the eDP connector
> +
> +Optional properties:
> +
> +- irq-gpios: OF device-tree gpio specification for interrupt pin
> +- bklt-ctrl-gpios: OF device-tree gpio specification for EDP_BKLTCTL pin


Same comment here, this isn't listed in the datasheet


> +
> +- sn,is-pluggable: boolean property to specify if HPD supported or not


Could you infer this from whether there's a panel present in the dts?


> +- sn,custom-modes: OF device-tree specifiction to add support for custom
modes


Please drop custom-modes, it doesn't belong on the bridge.

> +
> +Required nodes:
> +
> +This device has two video ports. Their connections are modelled using
the OF
> +graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> +
> +- Video port 0 for DSI input
> +- Video port 1 for eDP output
> +
> +Example
> +---
> +
> +edp-bridge@2d {
> +   compatible = "ti,sn65dsi86";
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   reg = <0x2d>;
> +
> +   enable-gpios = < 33 GPIO_ACTIVE_HIGH>;
> +   aux-sda-gpios = < 34 GPIO_ACTIVE_HIGH>;
> +   aux-scl-gpios = < 35 GPIO_ACTIVE_HIGH>;
> +   bias-en-gpios = < 36 GPIO_ACTIVE_HIGH>;
> +   bklt-en-gpios = < 37 GPIO_ACTIVE_HIGH>;
> +
> +   vccio-supply = <_l17>;
> +   vcca-supply = <_l6>;
> +   vccs-supply = <_l7>;
> +
> +   ports {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +
> +   port@0 {
> +   reg = <0>;
> +
> +   edp_bridge_in: endpoint {
> +   remote-endpoint = <_out>;
> +   };
> +   };
> +
> +   port@1 {
> +   reg = <1>;
> +
> +   edp_bridge_out: endpoint {
> +   remote-endpoint = <_panel_in>;
> +   };
> +   };
> +   };
> +}
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno