[Freedreno] [DPU PATCH 1/2] dt-bindings: msm/disp: Remove hw block offset DT entries for SDM845

2018-03-13 Thread Sravanthi Kollukuduru 
Remove DT entries of hw block offsets and other target specific catalog
information for SDM845.

Signed-off-by: Sravanthi Kollukuduru 
---
 .../devicetree/bindings/display/msm/dpu.txt| 530 -
 1 file changed, 530 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt 
b/Documentation/devicetree/bindings/display/msm/dpu.txt
index 136f0d3..90cd3e0 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
@@ -19,61 +19,6 @@ Required properties
 - interrupt-controller: Mark the device node as an interrupt controller.
 - #interrupt-cells: Should be one. The first cell is interrupt number.
 - iommus: Specifies the SID's used by this context bank.
-- qcom,dpu-sspp-type:  Array of strings for DPU source surface pipes 
type information.
-   A source pipe can be "vig", "rgb", "dma" or 
"cursor" type.
-   Number of xin ids defined should match the 
number of offsets
-   defined in property: qcom,dpu-sspp-off.
-- qcom,dpu-sspp-off:   Array of offset for DPU source surface pipes. 
The offsets
-   are calculated from register "mdp_phys" defined 
in
-   reg property + "dpu-off". The number of offsets 
defined here should
-   reflect the amount of pipes that can be active 
in DPU for
-   this configuration.
-- qcom,dpu-sspp-xin-id:Array of VBIF clients ids (xins) 
corresponding
-   to the respective source pipes. Number of xin 
ids
-   defined should match the number of offsets
-   defined in property: qcom,dpu-sspp-off.
-- qcom,dpu-ctl-off:Array of offset addresses for the available ctl
-   hw blocks within DPU, these offsets are
-   calculated from register "mdp_phys" defined in
-   reg property.  The number of ctl offsets defined
-   here should reflect the number of control paths
-   that can be configured concurrently on DPU for
-   this configuration.
-- qcom,dpu-wb-off: Array of offset addresses for the programmable
-   writeback blocks within DPU.
-- qcom,dpu-wb-xin-id:  Array of VBIF clients ids (xins) corresponding
-   to the respective writeback. Number of xin ids
-   defined should match the number of offsets
-   defined in property: qcom,dpu-wb-off.
-- qcom,dpu-mixer-off:  Array of offset addresses for the available
-   mixer blocks that can drive data to panel
-   interfaces. These offsets are be calculated from
-   register "mdp_phys" defined in reg property.
-   The number of offsets defined should reflect the
-   amount of mixers that can drive data to a panel
-   interface.
-- qcom,dpu-dspp-top-off:   Offset address for the dspp top block.
-   The offset is calculated from register 
"mdp_phys"
-   defined in reg property.
-- qcom,dpu-dspp-off:   Array of offset addresses for the available dspp
-   blocks. These offsets are calculated from
-   register "mdp_phys" defined in reg property.
-- qcom,dpu-pp-off: Array of offset addresses for the available
-   pingpong blocks. These offsets are calculated
-   from register "mdp_phys" defined in reg 
property.
-- qcom,dpu-pp-slave:   Array of flags indicating whether each ping pong
-   block may be configured as a pp slave.
-- qcom,dpu-intf-off:   Array of offset addresses for the available DPU
-   interface blocks that can drive data to a
-   panel controller. The offsets are calculated
-   from "mdp_phys" defined in reg property. The 
number
-   of offsets defined should reflect the number of
-   programmable interface blocks available in 
hardware.
-- qcom,dpu-mixer-blend-op-off  Array of offset addresses for the available
-   blending stages. The offsets are relative to
-   qcom,dpu-mixer-off.
-- qcom,dpu-mixer-pair-mask Array of mixer numbers that can be paired with
-

[Freedreno] [DPU PATCH 0/2] Add hardware catalog information in driver source for SDM845

2018-03-13 Thread Sravanthi Kollukuduru 
This patch series aims at adding the target specific hardware
catalog information in driver source.
As a result, the current logic of dt based parsing is removed.

The DT clean up patch corresponding to this driver change will
be posted separately.

Sravanthi Kollukuduru (2):
  dt-bindings: msm/disp: Remove hw block offset DT entries for SDM845
  drm/msm: Add hardware catalog file for SDM845

 .../devicetree/bindings/display/msm/dpu.txt|  530 
 drivers/gpu/drm/msm/Makefile   |1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3071 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |   17 +-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog_sdm845.c  |  744 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c|2 +-
 6 files changed, 767 insertions(+), 3598 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog_sdm845.c

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

Re: [Freedreno] [DPU PATCH 02/11] drm/msm: Don't duplicate modeset_enables atomic helper

2018-03-13 Thread Jeykumar Sankaran 

On 2018-03-12 13:21, Sean Paul wrote:

On Thu, Mar 08, 2018 at 04:56:01PM -0800, Jeykumar Sankaran wrote:

On 2018-02-28 11:18, Sean Paul wrote:
> Instead, shuffle things around so we kickoff crtc after enabling

encoder

> during modesets. Also moves the vblank wait to after the frame.
>
> Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3
> Signed-off-by: Sean Paul 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|   9 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |   5 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  31 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |   2 +
>  drivers/gpu/drm/msm/msm_atomic.c| 132

+---

>  5 files changed, 48 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index a3ab6ed2bf1d..94fab2dcca5b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc
> *crtc,
>DPU_EVT32_VERBOSE(DRMID(crtc));
>dpu_crtc = to_dpu_crtc(crtc);
>
> +  if (msm_is_mode_seamless(>state->adjusted_mode) ||
> +  msm_is_mode_seamless_vrr(>state->adjusted_mode)) {
> +  DPU_DEBUG("Skipping crtc enable, seamless mode\n");
> +  return;
> +  }
> +
>pm_runtime_get_sync(crtc->dev->dev);
>
>drm_for_each_encoder(encoder, crtc->dev) {
> @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc

*crtc,

>DPU_POWER_EVENT_POST_ENABLE | DPU_POWER_EVENT_POST_DISABLE
> |
>DPU_POWER_EVENT_PRE_DISABLE,
>dpu_crtc_handle_power_event, crtc, dpu_crtc->name);
> +
> +  if (msm_needs_vblank_pre_modeset(>state->adjusted_mode))
> +  drm_crtc_wait_one_vblank(crtc);
>  }
>
>  struct plane_state {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 28ceb589ee40..4d1e3652dbf4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -3693,8 +3693,11 @@ static void

dpu_encoder_frame_done_timeout(struct

> timer_list *t)
>  static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs

=

> {
>.mode_set = dpu_encoder_virt_mode_set,
>.disable = dpu_encoder_virt_disable,
> -  .enable = dpu_encoder_virt_enable,
> +  .enable = dpu_kms_encoder_enable,
>.atomic_check = dpu_encoder_virt_atomic_check,
> +
> +  /* This is called by dpu_kms_encoder_enable */
> +  .commit = dpu_encoder_virt_enable,
>  };
>
>  static const struct drm_encoder_funcs dpu_encoder_funcs = {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 81fd3a429e9f..3d83037e8305 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct

msm_kms

> *kms,
>dpu_encoder_prepare_commit(encoder);
>  }
>
> -static void dpu_kms_commit(struct msm_kms *kms,
> -  struct drm_atomic_state *old_state)
> +/*
> + * Override the encoder enable since we need to setup the inline
> rotator
> and do
> + * some crtc magic before enabling any bridge that might be present.
> + */
> +void dpu_kms_encoder_enable(struct drm_encoder *encoder)
> +{
> +  const struct drm_encoder_helper_funcs *funcs =
> encoder->helper_private;
> +  struct drm_crtc *crtc = encoder->crtc;
> +
> +  /* Forward this enable call to the commit hook */
> +  if (funcs && funcs->commit)
> +  funcs->commit(encoder);

The purpose of this function is not clear. Where are we setting up the
inline rotator?
Why do we need a kickoff here?


The reason the code is shuffled is to avoid duplicating the entire 
atomic

helper
function. By moving calls into the ->enable hooks, we can avoid having 
to

hand
roll the helpers.

The kickoff is preserved from the helper copy when you call
kms->funcs->commit
in between the encoder enable and bridge enable. If this can be 
removed,

that'd
be even better. I was simply trying to preserve the call order of
everything.

Sean
I am with you on cleaning up the atomic helper copy. But using 
enc->commit helper

to call into encoder_enable doesn't look valid to me.

Also, I verified removing the kms->funcs->commit call between encoder 
enable and
bridge enable. It works fine with your newly placed kms->funcs->commit 
after
drm_atomic_helper_commit_modeset_enables. So can you revisit this 
function?


Jeykumar S



> +
> +  if (crtc && crtc->state->active) {
> +  DPU_EVT32(DRMID(crtc));
> +  dpu_crtc_commit_kickoff(crtc);
> +  }
> +}
> +
> +static void dpu_kms_commit(struct msm_kms *kms, struct

drm_atomic_state

> *state)
>  {
>struct drm_crtc *crtc;
> -  struct drm_crtc_state *old_crtc_state;
> +  struct drm_crtc_state *crtc_state;
> +  struct dpu_crtc_state *cstate;
>int i;
>
> -

Re: [Freedreno] [DPU PATCH] msm/hdcp: Remove redundant stubs/CONFIG

2018-03-13 Thread abhinavk 

On 2018-03-12 13:12, Sean Paul wrote:

On Tue, Feb 27, 2018 at 11:24:31AM -0500, Sean Paul wrote:
On Mon, Feb 26, 2018 at 03:01:14PM -0800, abhin...@codeaurora.org 
wrote:

> The change itself is okay.


So, Reviewed-by?

Sean


> However I am planning to do a bigger cleanup here
> ( removing the entire hdmi_hdcp.c ).
>
> We dont use this file as we have our equivalent sde_hdcp_1x.c.

Yes, we definitely need to rationalize the 2 versions.

There's going to be a fair amount of work to get the sde/dpu version 
working
with the mainline property, backwards compatible with the hdmi version 
that
exists, as well as figuring out what key injection is going look like. 
So let's

not disable the current thing before the next thing is ready :-)

>
> Was planning this cleanup as part of the HDCP 1x requirements.
>
> If we want to push this as as separate change, I am okay with it but would
> prefer to wait ...

git revert is cheap, we can always overturn it, no need to wait.

Sean



>
> Abhinav
> On 2018-02-26 12:48, Sean Paul wrote:
> > We already have CONFIG_DRM_MSM_HDMI_HDCP, with accompanying stubs in
> > hdmi/hdmi.h.
> >
> > Signed-off-by: Sean Paul 

Reviewed-by: Abhinav Kumar 

> > ---
> >  drivers/gpu/drm/msm/hdmi/hdmi_hdcp.c | 24 
> >  1 file changed, 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hdcp.c
> > b/drivers/gpu/drm/msm/hdmi/hdmi_hdcp.c
> > index d24527468284..87e3acb3a259 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_hdcp.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hdcp.c
> > @@ -14,7 +14,6 @@
> >  #include "hdmi.h"
> >  #include 
> >
> > -#ifdef CONFIG_DRM_MSM_HDCP
> >  #define HDCP_REG_ENABLE 0x01
> >  #define HDCP_REG_DISABLE 0x00
> >  #define HDCP_PORT_ADDR 0x74
> > @@ -1436,26 +1435,3 @@ void msm_hdmi_hdcp_destroy(struct hdmi *hdmi)
> >   hdmi->hdcp_ctrl = NULL;
> >   }
> >  }
> > -
> > -#else
> > -struct hdmi_hdcp_ctrl *msm_hdmi_hdcp_init(struct hdmi *hdmi)
> > -{
> > - return NULL;
> > -}
> > -
> > -void msm_hdmi_hdcp_destroy(struct hdmi *hdmi)
> > -{
> > -}
> > -
> > -void msm_hdmi_hdcp_on(struct hdmi_hdcp_ctrl *hdcp_ctrl)
> > -{
> > -}
> > -
> > -void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl)
> > -{
> > -}
> > -
> > -void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl)
> > -{
> > -}
> > -#endif

--
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [Freedreno] [PATCH 2/2] arm64: dts: sdm845: Support GPU/GMU

2018-03-13 Thread Stephen Boyd 
On Sun, Mar 11, 2018 at 10:52 PM, Viresh Kumar  wrote:
> On 09-03-18, 09:03, Jordan Crouse wrote:
>> I don't think we are understanding each other. The GMU is a separate
>> microcontroller. It is given a magic number (actually a combination of magic
>> numbers) that it then uses to directly interact with the other hardware to 
>> make
>> the vote. The only responsibility that the CPU has is to construct that magic
>> number (once, at init) and send it across when asked.
>>
>> Looking at the sdhc code from the testing tree it makes perfect sense
>> that you have a device that needs to eventually do a RPMh vote from the CPU 
>> and
>> so the 'required-opp' and performance state come together to do the right 
>> thing.
>> This is good code.
>>
>> None of this is true for the GPU. The CPU never votes for the GPU so there
>> isn't any need to connect it to the power domain drivers. Even if you did
>> there isn't any current mechanism for the rpmpd driver to pass the right 
>> magic
>> number to the GPU driver which is what it really needs.
>>
>> I suppose that instead of using 'qcom,arc-level' we could use 'qcom-corner' 
>> but
>> then thats just a naming dispute. We still need a way for the GPU to query 
>> the
>> magic values.
>
> Okay, I get it. You can't (shouldn't) use genpd stuff here. I would still like
> you guys (You/Rajendra/Stephen) to conclude if qcom-corner and qcom,arc-level
> are eventually same values and we should use same property for them.
>

It sounds like it's qcom,{corner,level} from Jordan's description. In
my mind 'level' and 'corner' are the same but they got a name change
with the new RPM interface. Then another number space was introduced
with the new RPM interface, 'arc-level', which represents the numbers
that the hardware deals with. It may be that DT doesn't ever care to
express the 'arc-level', because cmd db hides the numberspace by
requiring software to translate the software 'level' to the hardware
'arc-level'. So the whole thing may be a moot point and we can decide
to use qcom,level everywhere because it's the future.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH 01/11] drm/msm: Skip seamless disables in crtc/encoder

2018-03-13 Thread Jeykumar Sankaran 

On 2018-03-12 13:14, Sean Paul wrote:

On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsa...@codeaurora.org wrote:

On 2018-02-28 11:18, Sean Paul wrote:
> Instead of duplicating whole swaths of atomic helper functions (which
> are already out-of-date), just skip the encoder/crtc disables in the
> .disable hooks.
>
> Change-Id: I7bd9183ae60624204fb1de9550656b776efc7202
> Signed-off-by: Sean Paul 


Reviewed-by: Jeykumar Sankaran 



Can you consider getting rid of these checks?


Do you mean the Change-Id? Yeah, I forgot to strip them out before
sending, I'll
make sure I clean it up before I apply.

Actually, I meant removing the seamless check flags that you are moving 
to

encode/crtc. But you can ignore that, I am planning to submit a seperate
change to remove the support from the whole pipeline.


> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|   8 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |   8 +
>  drivers/gpu/drm/msm/msm_atomic.c| 185

+---

>  3 files changed, 17 insertions(+), 184 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 3cdf1e3d9d96..a3ab6ed2bf1d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -3393,6 +3393,7 @@ static void dpu_crtc_disable(struct drm_crtc
> *crtc)
>  {
>struct dpu_crtc *dpu_crtc;
>struct dpu_crtc_state *cstate;
> +  struct drm_display_mode *mode;
>struct drm_encoder *encoder;
>struct msm_drm_private *priv;
>unsigned long flags;
> @@ -3407,8 +3408,15 @@ static void dpu_crtc_disable(struct drm_crtc
> *crtc)
>}
>dpu_crtc = to_dpu_crtc(crtc);
>cstate = to_dpu_crtc_state(crtc->state);
> +  mode = >base.adjusted_mode;
>priv = crtc->dev->dev_private;
>
> +  if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> ||
> +  msm_is_mode_seamless_dms(mode)) {
> +  DPU_DEBUG("Seamless mode is being applied, skip
> disable\n");
> +  return;
> +  }
> +
Another topic of discussion which should be brought up with dri-devel.

May not be common in PC world, but there are a handful of mobile OEM's
using panels which supports more than one resolution. Primary use 
cases

involve "seamless" switching to optimized display resolution when
streaming content changes resolutions or rendering lossless data.


Yeah, I think we can do this under the covers if the hardware supports 
it

such
as this patch. We could probably do a better job of making this useful 
for

other
drivers, but I was really just trying to get the seamless stuff out of 
the

way
so we don't need to roll our own atomic commit.

Sean



Jeykumar S.

>DPU_DEBUG("crtc%d\n", crtc->base.id);
>
>if (dpu_kms_is_suspend_state(crtc->dev))
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3d168fa09f3f..28ceb589ee40 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2183,6 +2183,7 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>struct dpu_encoder_virt *dpu_enc = NULL;
>struct msm_drm_private *priv;
>struct dpu_kms *dpu_kms;
> +  struct drm_display_mode *mode;
>int i = 0;
>
>if (!drm_enc) {
> @@ -2196,6 +2197,13 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>return;
>}
>
> +  mode = _enc->crtc->state->adjusted_mode;
> +  if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> ||
> +  msm_is_mode_seamless_dms(mode)) {
> +  DPU_DEBUG("Seamless mode is being applied, skip
> disable\n");
> +  return;
> +  }
> +
>dpu_enc = to_dpu_encoder_virt(drm_enc);
>DPU_DEBUG_ENC(dpu_enc, "\n");
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> index 46536edb72ee..5cfb80345052 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -84,189 +84,6 @@ static void msm_atomic_wait_for_commit_done(
>}
>  }
>
> -static void
> -msm_disable_outputs(struct drm_device *dev, struct drm_atomic_state
> *old_state)
> -{
> -  struct drm_connector *connector;
> -  struct drm_connector_state *old_conn_state, *new_conn_state;
> -  struct drm_crtc *crtc;
> -  struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> -  int i;
> -
> -  for_each_oldnew_connector_in_state(old_state, connector,
> old_conn_state, new_conn_state, i) {
> -  const struct drm_encoder_helper_funcs *funcs;
> -  struct drm_encoder *encoder;
> -  struct drm_crtc_state *old_crtc_state;
> -  unsigned int crtc_idx;
> -
> -  /*
> -   * Shut down everything that's in the changeset and
> currently
> -   * still on. So need to check the old, saved state.
> -   */
> -  if (!old_conn_state->crtc)
> -

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

2018-03-13 Thread Sibi S 

Hi Archit,
Thanks for the review.

On 03/13/2018 10:49 AM, Archit Taneja wrote:



On Monday 12 March 2018 06:53 PM, Sibi S wrote:

Add dsi host helper function implementation for DSI v2
and DSI 6G 1.x controllers

Signed-off-by: Sibi S 
---
  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(-)

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

index 80be83e..dfa049d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -183,6 +183,21 @@ int msm_dsi_host_modeset_init(struct 
mipi_dsi_host *host,

  int msm_dsi_host_init(struct msm_dsi *msm_dsi);
  int msm_dsi_runtime_suspend(struct device *dev);
  int msm_dsi_runtime_resume(struct device *dev);
+int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
+int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
+void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
+void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
+int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
+int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
+void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
+void *dsi_tx_buf_get_v2(struct msm_dsi_host *msm_host);
+void dsi_tx_buf_put_6g(struct msm_dsi_host *msm_host);
+int dsi_dma_base_get_6g(struct msm_dsi_host *msm_host, uint64_t *iova);
+int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t *iova);
+int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
+int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
+int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
+int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
  /* dsi phy */
  struct msm_dsi_phy;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c 
b/drivers/gpu/drm/msm/dsi/dsi_cfg.c

index 0327bb5..dc51aaa 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -136,20 +136,46 @@
  .num_dsi = 2,
  };
+const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
+    .link_clk_enable = dsi_link_clk_enable_v2,
+    .link_clk_disable = dsi_link_clk_disable_v2,
+    .clk_init_ver = dsi_clk_init_v2,
+    .tx_buf_alloc = dsi_tx_buf_alloc_v2,
+    .tx_buf_get = dsi_tx_buf_get_v2,
+    .tx_buf_put = NULL,
+    .dma_base_get = dsi_dma_base_get_v2,
+    .calc_clk_rate = dsi_calc_clk_rate_v2,
+};
+
+const static struct msm_dsi_host_cfg_ops msm_dsi_6g_host_ops = {
+    .link_clk_enable = dsi_link_clk_enable_6g,
+    .link_clk_disable = dsi_link_clk_disable_6g,
+    .clk_init_ver = NULL,
+    .tx_buf_alloc = dsi_tx_buf_alloc_6g,
+    .tx_buf_get = dsi_tx_buf_get_6g,
+    .tx_buf_put = dsi_tx_buf_put_6g,
+    .dma_base_get = dsi_dma_base_get_6g,
+    .calc_clk_rate = dsi_calc_clk_rate_6g,
+};


Could you introduce the host ops for SDM845 (i.e,
msm_dsi_6g_v2_host_ops) in this patch itself? It would be nice to
keep the DSI command broadcast code as a separate patch since it
probably needs to go through more iterations.

The ops approach looks good otherwise.

Thanks,
Archit



Sure I'll re-order them and probably should separately post
separate patch series for command broadcast on sdm845.


  static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
-    {MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064, _dsi_cfg},
+    {MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064,
+    _dsi_cfg, _dsi_v2_host_ops},
  {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_0,
-    _apq8084_dsi_cfg},
+    _apq8084_dsi_cfg, _dsi_6g_host_ops},
  {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_1,
-    _apq8084_dsi_cfg},
+    _apq8084_dsi_cfg, _dsi_6g_host_ops},
  {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_1_1,
-    _apq8084_dsi_cfg},
+    _apq8084_dsi_cfg, _dsi_6g_host_ops},
  {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_2,
-    _apq8084_dsi_cfg},
-    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_3, _dsi_cfg},
-    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_3_1, 
_dsi_cfg},
-    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_4_1, 
_dsi_cfg},
-    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1, 
_dsi_cfg},

+    _apq8084_dsi_cfg, _dsi_6g_host_ops},
+    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_3,
+    _dsi_cfg, _dsi_6g_host_ops},
+    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_3_1,
+    _dsi_cfg, _dsi_6g_host_ops},
+    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_4_1,
+    _dsi_cfg, _dsi_6g_host_ops},
+    {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
+    _dsi_cfg, _dsi_6g_host_ops},
  };
  const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c

index 7a03a94..f7a066d 100644
---

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

2018-03-13 Thread Sibi S 

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 
---
  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(-)




  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, );
+   mutex_unlock(>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,
+   _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 = _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



--
Qualcomm Innovation Center, Inc.
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