Re: [Intel-gfx] [PATCH v5 4/5] drm: Connector helper function to release resources

[Pandiyan, Dhinakaran]

On Thu, 2017-03-30 at 12:36 +0200, Maarten Lankhorst wrote:
> Op 30-03-17 om 10:42 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandi...@intel.com>
> >
> > Having an ->atomic_release callback is useful to release shared resources
> > that get allocated in compute_config(). This function is expected to be
> > called in the atomic_check() phase before new resources are acquired.
> >
> > v4: Document that the function is conditionally called and before
> > other atomic_checks() (Daniel)
> > v3: Use the new 'for_each_oldnew_connector_in_state()' macro.
> > v2: Moved the caller hunk to this patch (Daniel)
> >
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Cc: Archit Taneja <arch...@codeaurora.org>
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Harry Wentland <harry.wentl...@amd.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c  | 19 +++
> >  include/drm/drm_modeset_helper_vtables.h | 16 
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index d14094d..9d07669 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -586,6 +586,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > }
> > }
> >  
> > +   for_each_oldnew_connector_in_state(state, connector, 
> > old_connector_state, new_connector_state, i) {
> > +   const struct drm_connector_helper_funcs *conn_funcs;
> > +   struct drm_crtc_state *crtc_state;
> > +
> > +   conn_funcs = connector->helper_private;
> > +   if (!conn_funcs->atomic_release)
> > +   continue;
> > +
> > +   if (!old_connector_state->crtc)
> > +   continue;
> > +
> > +   crtc_state = drm_atomic_get_existing_crtc_state(state, 
> > old_connector_state->crtc);
> > +
> > +   if (crtc_state->connectors_changed ||
> > +   crtc_state->mode_changed ||
> > +   (crtc_state->active_changed && !crtc_state->active))
> > +   conn_funcs->atomic_release(connector, 
> > new_connector_state);
> > +   }
> > +
> > return mode_fixup(state);
> >  }
> Oops, I'm just looking at patch 5 again..
> 
> atomic_release should return an int to allow error propogation. There's no 
> good reason why it shouldn't.
> This would allow handling errors in patch 5 more gracefully.

Makes sense, will change that. This made me think about how
intel_dp_mst_atomic_release() handles an invalid mst_port reference
i.e., in case the port is gone. I'll fix both and send a new version.

-DK


> >  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
> > diff --git a/include/drm/drm_modeset_helper_vtables.h 
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 091c422..84e80aa 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -836,6 +836,22 @@ struct drm_connector_helper_funcs {
> >  */
> > struct drm_encoder *(*atomic_best_encoder)(struct drm_connector 
> > *connector,
> >struct drm_connector_state 
> > *connector_state);
> > +
> > +   /**
> > +* @atomic_release:
> > +*
> > +* This function is conditionally called to release shared resources
> > +* when the attached CRTC's mode changes or it's connectors change or
> > +* becomes inactive. It is called before the corresponding
> > +* _crtc_helper_funcs.atomic_check or
> > +* _crtc_helper_funcs.mode_fixup hooks are called.
> > +*
> > +* NOTE:
> > +*
> > +* This function is called in the check phase of an atomic update.
> > +*/
> > +   void (*atomic_release)(struct drm_connector *connector,
> > +  struct drm_connector_state *connector_state);
> >  };
> >  
> >  /**
> 
> 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] Revert unrelated part of "drm: simplify the locking in the GETCRTC ioctl"

[Pandiyan, Dhinakaran]

On Thu, 2017-03-30 at 09:36 +0200, Maarten Lankhorst wrote:
> Op 28-03-17 om 09:01 schreef Daniel Vetter:
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 68cded453882..43dbad62786e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12463,6 +12463,11 @@ static int intel_atomic_check(struct drm_device 
> > *dev,
> > ret = drm_atomic_helper_check_modeset(dev, state);
> > if (ret)
> > return ret;
> > +   /* enocder->atomic_check might upgrade some crtc to a full modeset */
> > +   ret = drm_atomic_helper_check_modeset(dev, state);
> > +   if (ret)
> > +   return ret;
> > +
> >  
> > for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, 
> > i) {
> > struct intel_crtc_state *pipe_config =
> 
> I know this patch has been applied, but this hunk is completely unrelated.
> 
> Can I get a R-B on reverting it?
> 
> >8
> v2 of the commit 2c77bb29d398 ("drm: simplify the locking in the GETCRTC 
> ioctl")
> accidentally introduced a unrelated change in intel_display.c, revert the
> unrelated change.
>  
> Signed-off-by: Maarten Lankhorst 
> Fixes: 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl")
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index baa8d836c8e7..c45694abda5b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12478,11 +12478,6 @@ static int intel_atomic_check(struct drm_device *dev,
>   ret = drm_atomic_helper_check_modeset(dev, state);
>   if (ret)
>   return ret;
> - /* enocder->atomic_check might upgrade some crtc to a full modeset */
> - ret = drm_atomic_helper_check_modeset(dev, state);


Noticed this while testing my driver-private object series.
drm_atomic_helper_check_modeset->atomic_release() getting called twice
within an atomic_check() broke the vcpi slots bookkeeping I had and
ironically exposed a bug in my code.

Reported-by: Dhinakaran Pandiyan 
Reviewed-by: Dhinakaran Pandiyan 


> - if (ret)
> - return ret;
> -
>  
>   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, 
> i) {
>   struct intel_crtc_state *pipe_config =
> 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Mon, 2017-03-27 at 10:35 +0200, Maarten Lankhorst wrote:
> Op 27-03-17 om 10:31 schreef Daniel Vetter:
> > On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote:
> >> Op 27-03-17 om 08:38 schreef Daniel Vetter:
> >>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote:
> >>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandi...@intel.com>
> >>>>
> >>>> It is necessary to track states for objects other than connector, crtc
> >>>> and plane for atomic modesets. But adding objects like DP MST link
> >>>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>>> modified by the core helper functions for swapping and clearing
> >>>> it's state. So, lets add void * objects and helper functions that operate
> >>>> on void * types to keep these objects and states private to the core.
> >>>> Drivers can then implement specific functions to swap and clear states.
> >>>> The other advantage having just void * for these objects in
> >>>> drm_atomic_state is that objects of different types can be managed in the
> >>>> same state array.
> >>>>
> >>>> v4: Avoid redundant NULL checks when private_objs array is empty 
> >>>> (Maarten)
> >>>> v3: Macro alignment (Chris)
> >>>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>>
> >>>> Acked-by: Harry Wentland <harry.wentl...@amd.com>
> >>>> Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_atomic.c| 69 +++
> >>>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >>>>  include/drm/drm_atomic.h| 93 
> >>>> +
> >>>>  3 files changed, 167 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>>> index 9b892af..e590148 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> >>>> drm_atomic_state *state)
> >>>>  kfree(state->connectors);
> >>>>  kfree(state->crtcs);
> >>>>  kfree(state->planes);
> >>>> +kfree(state->private_objs);
> >>>>  }
> >>>>  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >>>>  
> >>>> @@ -184,6 +185,21 @@ void drm_atomic_state_default_clear(struct 
> >>>> drm_atomic_state *state)
> >>>>  state->planes[i].ptr = NULL;
> >>>>  state->planes[i].state = NULL;
> >>>>  }
> >>>> +
> >>>> +for (i = 0; i < state->num_private_objs; i++) {
> >>>> +void *private_obj = state->private_objs[i].obj;
> >>>> +void *obj_state = state->private_objs[i].obj_state;
> >>>> +
> >>>> +if (!private_obj)
> >>>> +continue;
> >>>> +
> >>>> +state->private_objs[i].funcs->destroy_state(obj_state);
> >>>> +state->private_objs[i].obj = NULL;
> >>>> +state->private_objs[i].obj_state = NULL;
> >>>> +state->private_objs[i].funcs = NULL;
> >>>> +}
> >>>> +state->num_private_objs = 0;
> >>> Here we set num_private_objs = 0;
> >>>
> >>>> +
> >>>>  }
> >>>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>>>  
> >>>> @@ -978,6 +994,59 @@ static void drm_atomic_plane_print_state(struct 
> >>>> drm_printer *p,
> >>>>  }
> >>>>  
> >>>>  /**
> >>>> + * drm_atomic_get_private_obj_state - get private object state
> >>>> + * @state: global atomic state
> >>>> + * @obj: private object to get the state for
> >>>> + * @funcs: pointer to the struct of function pointers that identify the 
> >>>> object
> >>>> + * type

Re: [Intel-gfx] [PATCH v4 8/8] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Wed, 2017-03-22 at 13:30 +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandi...@intel.com>
> >
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> >
> > v2:
> > Squashed atomic_release() implementation and caller (Daniel)
> > Fixed logic for connector-crtc switching case (Daniel)
> > Fixed logic for suspend-resume case.
> >
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Cc: Archit Taneja <arch...@codeaurora.org>
> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Harry Wentland <harry.wentl...@amd.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 38 
> > ++---
> >  1 file changed, 31 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index c1f62eb..a8f40fa 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct 
> > intel_encoder *encoder,
> > struct intel_dp *intel_dp = _dig_port->dp;
> > struct intel_connector *connector =
> > to_intel_connector(conn_state->connector);
> > -   struct drm_atomic_state *state;
> > +   struct drm_atomic_state *state = pipe_config->base.state;
> > int bpp;
> > -   int lane_count, slots;
> > +   int lane_count, slots = 0;
> > const struct drm_display_mode *adjusted_mode = 
> > _config->base.adjusted_mode;
> > int mst_pbn;
> >  
> > @@ -57,30 +57,53 @@ static bool intel_dp_mst_compute_config(struct 
> > intel_encoder *encoder,
> >  * seem to suggest we should do otherwise.
> >  */
> > lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> > -
> > pipe_config->lane_count = lane_count;
> >  
> > pipe_config->pipe_bpp = bpp;
> > pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
> >  
> > -   state = pipe_config->base.state;
> > -
> > if (drm_dp_mst_port_has_audio(_dp->mst_mgr, connector->port))
> > pipe_config->has_audio = true;
> > -   mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  
> > +   mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> > pipe_config->pbn = mst_pbn;
> > -   slots = drm_dp_find_vcpi_slots(_dp->mst_mgr, mst_pbn);
> >  
> > intel_link_compute_m_n(bpp, lane_count,
> >adjusted_mode->crtc_clock,
> >pipe_config->port_clock,
> >_config->dp_m_n);
> >  
> > +   if (pipe_config->base.active) {
> > +   slots = drm_dp_atomic_find_vcpi_slots(state, _dp->mst_mgr,
> > + connector->port, mst_pbn);
> > +   if (slots < 0) {
> > +   DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> > +   return false;
> > +   }
> > +   }
> > pipe_config->dp_m_n.tu = slots;
> >  
> > return true;
> > +}
> > +
> > +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> > +   struct drm_connector_state *conn_state)
> > +{
> > +   struct intel_dp_mst_encoder *intel_mst;
> > +   struct drm_dp_mst_topology_mgr *mgr;
> > +   struct drm_encoder *encoder;
> > +   struct intel_connector *intel_connector = to_intel_connector(connector);
> > +   struct drm_atomic_state *state = conn_state->state;
> > +   int slots;
> > +
> > +   encoder = connector->state->best_encoder;
> > +   intel_mst = enc_to_mst(encoder);
> > +   mgr = _mst->primary->dp.mst_mgr;
> >  
> > +   slots = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > +intel_connector->port);
> > +   if (slots < 0)
> > + 

Re: [Intel-gfx] [PATCH v4 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-03-22 at 11:00 +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 08:10 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandi...@intel.com>
> >
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> > v3: Macro alignment (Chris)
> >
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Cc: Archit Taneja <arch...@codeaurora.org>
> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Harry Wentland <harry.wentl...@amd.com>
> > Acked-by: Harry Wentland <harry.wentl...@amd.com>
> > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> 
> Mostly looks good, but too many null checks. I think it's best to get rid of 
> them all
> by freeing state->driver_private in default_clear() or setting 
> num_private_objs to 0.
> It would remove the need for all null checks I think..
> 
> ~Maarten
> 

Did you mean the NULL checks in this loop inside
drm_atomic_get_private_obj_state()

+   for (i = 0; i < state->num_private_objs; i++)
+   if (obj == state->private_objs[i].obj &&
+   state->private_objs[i].obj_state)
+   return state->private_objs[i].obj_state;

and the fact that I am not setting num_private_objs = 0 in
drm_atomic_state_default_clear() ?

-DK
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

IRC acked by Harry Wentland


" dhnkrn, the patch for driver-private atomic state object
makes sense to me.  Didn't realize that's the same one from early
February. Feel free to add my Acked-by"


-DK
On Wed, 2017-02-08 at 22:38 -0800, Dhinakaran Pandiyan wrote:
> It is necessary to track states for objects other than connector, crtc
> and plane for atomic modesets. But adding objects like DP MST link
> bandwidth to drm_atomic_state would mean that a non-core object will be
> modified by the core helper functions for swapping and clearing
> it's state. So, lets add void * objects and helper functions that operate
> on void * types to keep these objects and states private to the core.
> Drivers can then implement specific functions to swap and clear states.
> The other advantage having just void * for these objects in
> drm_atomic_state is that objects of different types can be managed in the
> same state array.
> 
> v2: Added docs and new iterator to filter private objects (Daniel)
> 
> Suggested-by: Daniel Vetter 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_atomic.c| 68 +++
>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
>  include/drm/drm_atomic.h| 91 
> +
>  3 files changed, 164 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..1a9ffe8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> drm_atomic_state *state)
>   kfree(state->connectors);
>   kfree(state->crtcs);
>   kfree(state->planes);
> + kfree(state->private_objs);
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_release);
>  
> @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> drm_atomic_state *state)
>   state->planes[i].ptr = NULL;
>   state->planes[i].state = NULL;
>   }
> +
> + for (i = 0; i < state->num_private_objs; i++) {
> + void *private_obj = state->private_objs[i].obj;
> + void *obj_state = state->private_objs[i].obj_state;
> +
> + if (!private_obj)
> + continue;
> +
> + state->private_objs[i].funcs->destroy_state(obj_state);
> + state->private_objs[i].obj = NULL;
> + state->private_objs[i].obj_state = NULL;
> + state->private_objs[i].funcs = NULL;
> + }
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the 
> object
> + * type
> + *
> + * This function returns the private object state for the given private 
> object,
> + * allocating the state if needed. It does not grab any locks as the caller 
> is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> +   const struct drm_private_state_funcs *funcs)
> +{
> + int index, num_objs, i;
> + size_t size;
> + struct __drm_private_objs_state *arr;
> +
> + for (i = 0; i < state->num_private_objs; i++)
> + if (obj == state->private_objs[i].obj &&
> + state->private_objs[i].obj_state)
> + return state->private_objs[i].obj_state;
> +
> + num_objs = state->num_private_objs + 1;
> + size = sizeof(*state->private_objs) * num_objs;
> + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> + if (!arr)
> + return ERR_PTR(-ENOMEM);
> +
> + state->private_objs = arr;
> + index = state->num_private_objs;
> + memset(>private_objs[index], 0, sizeof(*state->private_objs));
> +
> + state->private_objs[index].obj_state = funcs->duplicate_state(state, 
> obj);
> + if (!state->private_objs[index].obj_state)
> + return ERR_PTR(-ENOMEM);
> +
> + state->private_objs[index].obj = obj;
> + state->private_objs[index].funcs = funcs;
> + state->num_private_objs = num_objs;
> +
> + DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> +  state->private_objs[index].obj_state, state);
> +
> + return state->private_objs[index].obj_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> 

Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Sun, 2017-02-26 at 20:57 +0100, Daniel Vetter wrote:
> On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> > > 
> > > On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> > > > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> > > >> Comparing this func to 
> > > >> drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> > > >> doesn't seem to call drm_modeset_lock if the obj_state doesn't already 
> > > >> exist. I
> > > >> don't understand the locking stuff toowell, I just noticed this 
> > > >> difference when
> > > >> comparing this approach with what is done in the msm kms driver (where 
> > > >> we
> > > >> have subclassed drm_atomic_state to msm_kms_state).
> > > >>
> > > >> Thanks,
> > > >> Archit
> > > >>
> > > >
> > > >
> > > > The caller is expected to take care of any required locking. The
> > > > driver-private objects are opaque from core's pov, so the core is not
> > > > aware of necessary locks for that object type.
> > > 
> > > I had a look at the rest of the series, and I couldn't easily understand
> > > whether the caller code protects the MST related driver private state. Is
> > > it expected to be protect via the drm_mode_config.connection_mutex lock?
> > > 
> > > Thanks,
> > > Archit
> > > 
> > 
> > That's right, the connection_mutex takes care of the locking for the MST
> > private state. I can add that as a comment to the caller's (MST helper)
> > kernel doc with a
> > 
> > WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex)); 
> 
> Please don't add this as a comment, but as real code so it is checked at
> runtime :-) Personally I wouldn't mention locking rules in kernel-doc,
> that part tends to get outdated fast. Better to enforce with
> runtime-checks.
> -Daniel

That's what I meant but evidently didn't type it that way:) I was going
to add that the connection_mutex does the locking for MST state as a
comment and put the WARN_ON() in code.

-DK

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Pandiyan, Dhinakaran]

On Thu, 2017-02-16 at 09:09 +, Lankhorst, Maarten wrote:
> Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > <dhinakaran.pandi...@intel.com> wrote:
> > > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > > On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> > > > > > 
> > > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-
> > > > > > 0800]:
> > > > > > > 
> > > > > > > Having a ->atomic_release callback is useful to release
> > > > > > > shared
> > > > > > > resources
> > > > > > > that get allocated in compute_config(). This function is
> > > > > > > expected
> > > > > > > to
> > > > > > > be
> > > > > > > called in the atomic_check() phase before new resources are
> > > > > > > acquired.
> > > > > > > 
> > > > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > > > 
> > > > > > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int
> > > > > > > el.com
> > > > > > > > 
> > > > > > > 
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> > > > > > > +++
> > > > > > >  include/drm/drm_modeset_helper_vtables.h | 13
> > > > > > > +
> > > > > > >  2 files changed, 32 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > index 8795088..92bd741 100644
> > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > > > drm_device *dev,
> > > > > > > }
> > > > > > > }
> > > > > > > 
> > > > > > > +   for_each_connector_in_state(state, connector,
> > > > > > > connector_state, i) {
> > > > > > > +   const struct drm_connector_helper_funcs
> > > > > > > *conn_funcs;
> > > > > > > +   struct drm_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > +   conn_funcs = connector->helper_private;
> > > > > > > +   if (!conn_funcs->atomic_release)
> > > > > > > +   continue;
> > > > > > > +
> > > > > > > +   if (!connector->state->crtc)
> > > > > > > +   continue;
> > > > > > > +
> > > > > > > +   crtc_state =
> > > > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > > > > crtc);
> > > > > > > 
> > > > > > > +
> > > > > > > +   if (crtc_state->connectors_changed ||
> > > > > > > +   crtc_state->mode_changed ||
> > > > > > > +   (crtc_state->active_changed &&
> > > > > > > !crtc_state-
> > > > > > > > 
> > > > > > > > active))
> > > > > > > 
> > > > > > > +   conn_funcs-
> > > > > > > >atomic_release(connector,
> > > > > > > connector_state);
> > > > > > > +   }
> > > > > > 
> > > > > > Could we deal with the VCPI state separately in
> > > > > > intel_modeset_checks,
> > > > > > like we do with dpll?
> > > > > 
> > > > > We'd want to release the VCPI slots before they are acquired in
> > > > > ->compute_config(). intel_modeset_check

Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-02-22 at 09:59 +0530, Archit Taneja wrote:
> 
> On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> >>
> >> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> >>> On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> >>>> Hi,
> >>>>
> >>>> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> >>>>> It is necessary to track states for objects other than connector, crtc
> >>>>> and plane for atomic modesets. But adding objects like DP MST link
> >>>>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>>>> modified by the core helper functions for swapping and clearing
> >>>>> it's state. So, lets add void * objects and helper functions that 
> >>>>> operate
> >>>>> on void * types to keep these objects and states private to the core.
> >>>>> Drivers can then implement specific functions to swap and clear states.
> >>>>> The other advantage having just void * for these objects in
> >>>>> drm_atomic_state is that objects of different types can be managed in 
> >>>>> the
> >>>>> same state array.
> >>>>>
> >>>>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>>>
> >>>>> Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/drm_atomic.c| 68 +++
> >>>>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >>>>>  include/drm/drm_atomic.h| 91 
> >>>>> +
> >>>>>  3 files changed, 164 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>>>> index a567310..1a9ffe8 100644
> >>>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> >>>>> drm_atomic_state *state)
> >>>>> kfree(state->connectors);
> >>>>> kfree(state->crtcs);
> >>>>> kfree(state->planes);
> >>>>> +   kfree(state->private_objs);
> >>>>>  }
> >>>>>  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >>>>>
> >>>>> @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> >>>>> drm_atomic_state *state)
> >>>>> state->planes[i].ptr = NULL;
> >>>>> state->planes[i].state = NULL;
> >>>>> }
> >>>>> +
> >>>>> +   for (i = 0; i < state->num_private_objs; i++) {
> >>>>> +   void *private_obj = state->private_objs[i].obj;
> >>>>> +   void *obj_state = state->private_objs[i].obj_state;
> >>>>> +
> >>>>> +   if (!private_obj)
> >>>>> +   continue;
> >>>>> +
> >>>>> +   state->private_objs[i].funcs->destroy_state(obj_state);
> >>>>> +   state->private_objs[i].obj = NULL;
> >>>>> +   state->private_objs[i].obj_state = NULL;
> >>>>> +   state->private_objs[i].funcs = NULL;
> >>>>> +   }
> >>>>> +
> >>>>>  }
> >>>>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>>>>
> >>>>> @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> >>>>> drm_printer *p,
> >>>>>  }
> >>>>>
> >>>>>  /**
> >>>>> + * drm_atomic_get_private_obj_state - get private object state
> >>>>> + * @state: global atomic state
> >>>>> + * @obj: private object to get the state for
> >>>>> + * @funcs: pointer to the struct of function pointers that identify 
> >>>>> the object
> >>>>> + * type
> >>>>> + *
> >>>>> + * This functi

Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote:
> 
> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote:
> > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> >> Hi,
> >>
> >> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> >>> It is necessary to track states for objects other than connector, crtc
> >>> and plane for atomic modesets. But adding objects like DP MST link
> >>> bandwidth to drm_atomic_state would mean that a non-core object will be
> >>> modified by the core helper functions for swapping and clearing
> >>> it's state. So, lets add void * objects and helper functions that operate
> >>> on void * types to keep these objects and states private to the core.
> >>> Drivers can then implement specific functions to swap and clear states.
> >>> The other advantage having just void * for these objects in
> >>> drm_atomic_state is that objects of different types can be managed in the
> >>> same state array.
> >>>
> >>> v2: Added docs and new iterator to filter private objects (Daniel)
> >>>
> >>> Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/drm_atomic.c| 68 +++
> >>>  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >>>  include/drm/drm_atomic.h| 91 
> >>> +
> >>>  3 files changed, 164 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index a567310..1a9ffe8 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> >>> drm_atomic_state *state)
> >>>   kfree(state->connectors);
> >>>   kfree(state->crtcs);
> >>>   kfree(state->planes);
> >>> + kfree(state->private_objs);
> >>>  }
> >>>  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >>>
> >>> @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> >>> drm_atomic_state *state)
> >>>   state->planes[i].ptr = NULL;
> >>>   state->planes[i].state = NULL;
> >>>   }
> >>> +
> >>> + for (i = 0; i < state->num_private_objs; i++) {
> >>> + void *private_obj = state->private_objs[i].obj;
> >>> + void *obj_state = state->private_objs[i].obj_state;
> >>> +
> >>> + if (!private_obj)
> >>> + continue;
> >>> +
> >>> + state->private_objs[i].funcs->destroy_state(obj_state);
> >>> + state->private_objs[i].obj = NULL;
> >>> + state->private_objs[i].obj_state = NULL;
> >>> + state->private_objs[i].funcs = NULL;
> >>> + }
> >>> +
> >>>  }
> >>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >>>
> >>> @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> >>> drm_printer *p,
> >>>  }
> >>>
> >>>  /**
> >>> + * drm_atomic_get_private_obj_state - get private object state
> >>> + * @state: global atomic state
> >>> + * @obj: private object to get the state for
> >>> + * @funcs: pointer to the struct of function pointers that identify the 
> >>> object
> >>> + * type
> >>> + *
> >>> + * This function returns the private object state for the given private 
> >>> object,
> >>> + * allocating the state if needed. It does not grab any locks as the 
> >>> caller is
> >>> + * expected to care of any required locking.
> >>> + *
> >>> + * RETURNS:
> >>> + *
> >>> + * Either the allocated state or the error code encoded into a pointer.
> >>> + */
> >>> +void *
> >>> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void 
> >>> *obj,
> >>> +   const struct drm_private_state_funcs *funcs)
> >>> +{
> >>> + int index, num_objs, i;
> >>> + size_t size;
> >>> + struct __drm_private_objs_state *arr;
> >>> +
> >>> + for (i = 

Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> Hi,
> 
> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> >
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 68 +++
> >  drivers/gpu/drm/drm_atomic_helper.c |  5 ++
> >  include/drm/drm_atomic.h| 91 
> > +
> >  3 files changed, 164 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index a567310..1a9ffe8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> > drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > +   kfree(state->private_objs);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >
> > @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> > drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > +   for (i = 0; i < state->num_private_objs; i++) {
> > +   void *private_obj = state->private_objs[i].obj;
> > +   void *obj_state = state->private_objs[i].obj_state;
> > +
> > +   if (!private_obj)
> > +   continue;
> > +
> > +   state->private_objs[i].funcs->destroy_state(obj_state);
> > +   state->private_objs[i].obj = NULL;
> > +   state->private_objs[i].obj_state = NULL;
> > +   state->private_objs[i].funcs = NULL;
> > +   }
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >
> > @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct 
> > drm_printer *p,
> >  }
> >
> >  /**
> > + * drm_atomic_get_private_obj_state - get private object state
> > + * @state: global atomic state
> > + * @obj: private object to get the state for
> > + * @funcs: pointer to the struct of function pointers that identify the 
> > object
> > + * type
> > + *
> > + * This function returns the private object state for the given private 
> > object,
> > + * allocating the state if needed. It does not grab any locks as the 
> > caller is
> > + * expected to care of any required locking.
> > + *
> > + * RETURNS:
> > + *
> > + * Either the allocated state or the error code encoded into a pointer.
> > + */
> > +void *
> > +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> > + const struct drm_private_state_funcs *funcs)
> > +{
> > +   int index, num_objs, i;
> > +   size_t size;
> > +   struct __drm_private_objs_state *arr;
> > +
> > +   for (i = 0; i < state->num_private_objs; i++)
> > +   if (obj == state->private_objs[i].obj &&
> > +   state->private_objs[i].obj_state)
> > +   return state->private_objs[i].obj_state;
> 
> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, 
> it
> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. 
> I
> don't understand the locking stuff toowell, I just noticed this difference 
> when
> comparing this approach with what is done in the msm kms driver (where we
> have subclassed drm_atomic_state to msm_kms_state).
> 
> Thanks,
> Archit
> 


The caller is expected to take care of any required locking. The
driver-private objects are opaque from core's pov, so the core is not
aware of necessary locks for that object type.

-DK 

> > +
> > +   num_objs = state->num_private_objs + 1;
> > +   size = sizeof(*state->private_objs) * num_objs;
> > +   arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > +   if (!arr)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   state->private_objs = arr;
> > +   index = state->num_private_objs;
> > +   memset(>private_objs[index], 0, sizeof(*state->private_objs));
> > +
> > +   state->private_objs[index].obj_state = funcs->duplicate_state(state, 
> > obj);
> > +   if (!state->private_objs[index].obj_state)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   state->private_objs[index].obj = 

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Pandiyan, Dhinakaran]

On Tue, 2017-02-14 at 20:51 +0100, Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandi...@intel.com> wrote:
> > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> >> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> >> > On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> >> > >
> >> > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> >> > > >
> >> > > > Having a ->atomic_release callback is useful to release shared
> >> > > > resources
> >> > > > that get allocated in compute_config(). This function is expected
> >> > > > to
> >> > > > be
> >> > > > called in the atomic_check() phase before new resources are
> >> > > > acquired.
> >> > > >
> >> > > > v2: Moved the caller hunk to this patch (Daniel)
> >> > > >
> >> > > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com
> >> > > > >
> >> > > > ---
> >> > > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> >> > > > +++
> >> > > >  include/drm/drm_modeset_helper_vtables.h | 13 +
> >> > > >  2 files changed, 32 insertions(+)
> >> > > >
> >> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> > > > b/drivers/gpu/drm/drm_atomic_helper.c
> >> > > > index 8795088..92bd741 100644
> >> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> >> > > > drm_device *dev,
> >> > > > }
> >> > > > }
> >> > > >
> >> > > > +   for_each_connector_in_state(state, connector,
> >> > > > connector_state, i) {
> >> > > > +   const struct drm_connector_helper_funcs
> >> > > > *conn_funcs;
> >> > > > +   struct drm_crtc_state *crtc_state;
> >> > > > +
> >> > > > +   conn_funcs = connector->helper_private;
> >> > > > +   if (!conn_funcs->atomic_release)
> >> > > > +   continue;
> >> > > > +
> >> > > > +   if (!connector->state->crtc)
> >> > > > +   continue;
> >> > > > +
> >> > > > +   crtc_state =
> >> > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> >> > > > >crtc);
> >> > > > +
> >> > > > +   if (crtc_state->connectors_changed ||
> >> > > > +   crtc_state->mode_changed ||
> >> > > > +   (crtc_state->active_changed && !crtc_state-
> >> > > > >
> >> > > > > active))
> >> > > > +   conn_funcs->atomic_release(connector,
> >> > > > connector_state);
> >> > > > +   }
> >> > >
> >> > > Could we deal with the VCPI state separately in
> >> > > intel_modeset_checks,
> >> > > like we do with dpll?
> >> >
> >> > We'd want to release the VCPI slots before they are acquired in
> >> > ->compute_config(). intel_modeset_checks() will be too late to
> >> > release
> >> > them. Are you suggesting both acquiring and releasing slots should be
> >> > done in intel_modeset_checks()?
> >>
> >> That makes things a bit more nasty. Maybe add a
> >> conn_funcs->atomic_check that always gets called, something like I did
> >> below?
> >>
> >> I'd love to use it for some atomic connector properties too.
> >
> >
> > Adding and unconditionally calling conn_funcs->atomic_check() should be
> > doable. It also follows the pattern we have for encoders and CRTCs. But
> > I'll have to move the connector->state->crtc state checks inside the
> > function.
> 
> Adding ->atomic_check that's unconditionally called sounds 

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Pandiyan, Dhinakaran]

On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> > > 
> > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > > 
> > > > Having a ->atomic_release callback is useful to release shared
> > > > resources
> > > > that get allocated in compute_config(). This function is expected
> > > > to
> > > > be
> > > > called in the atomic_check() phase before new resources are
> > > > acquired.
> > > > 
> > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > 
> > > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com
> > > > >
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> > > > +++
> > > >  include/drm/drm_modeset_helper_vtables.h | 13 +
> > > >  2 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 8795088..92bd741 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > drm_device *dev,
> > > > }
> > > > }
> > > >  
> > > > +   for_each_connector_in_state(state, connector,
> > > > connector_state, i) {
> > > > +   const struct drm_connector_helper_funcs
> > > > *conn_funcs;
> > > > +   struct drm_crtc_state *crtc_state;
> > > > +
> > > > +   conn_funcs = connector->helper_private;
> > > > +   if (!conn_funcs->atomic_release)
> > > > +   continue;
> > > > +
> > > > +   if (!connector->state->crtc)
> > > > +   continue;
> > > > +
> > > > +   crtc_state =
> > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > >crtc);
> > > > +
> > > > +   if (crtc_state->connectors_changed ||
> > > > +   crtc_state->mode_changed ||
> > > > +   (crtc_state->active_changed && !crtc_state-
> > > > > 
> > > > > active))
> > > > +   conn_funcs->atomic_release(connector,
> > > > connector_state);
> > > > +   }
> > > 
> > > Could we deal with the VCPI state separately in
> > > intel_modeset_checks,
> > > like we do with dpll?
> > 
> > We'd want to release the VCPI slots before they are acquired in
> > ->compute_config(). intel_modeset_checks() will be too late to
> > release
> > them. Are you suggesting both acquiring and releasing slots should be
> > done in intel_modeset_checks()?
> 
> That makes things a bit more nasty. Maybe add a
> conn_funcs->atomic_check that always gets called, something like I did
> below?
> 
> I'd love to use it for some atomic connector properties too.


Adding and unconditionally calling conn_funcs->atomic_check() should be
doable. It also follows the pattern we have for encoders and CRTCs. But
I'll have to move the connector->state->crtc state checks inside the
function.

-DK
> > > 
> > > 
> > > Maybe implementing the relevant VCPI state could be done as an
> > > atomic
> > > helper function too, so other atomic drivers can just plug it in.
> > > 
> > The idea was to reduce boilerplate in the drivers and use the
> > private_obj state for different object types.
> > 
> > 
> > > 
> > > Not sure how doable this is, but if it's not too hard, then it's
> > > probably cleaner :)
> > > ___
> > > Intel-gfx mailing list
> > > intel-...@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Pandiyan, Dhinakaran]

On Mon, 2017-02-13 at 21:26 +, Pandiyan, Dhinakaran wrote:
> On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> > > > 
> > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > > > 
> > > > > Having a ->atomic_release callback is useful to release shared
> > > > > resources
> > > > > that get allocated in compute_config(). This function is expected
> > > > > to
> > > > > be
> > > > > called in the atomic_check() phase before new resources are
> > > > > acquired.
> > > > > 
> > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > 
> > > > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com
> > > > > >
> > > > > ---
> > > > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> > > > > +++
> > > > >  include/drm/drm_modeset_helper_vtables.h | 13 +
> > > > >  2 files changed, 32 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index 8795088..92bd741 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > drm_device *dev,
> > > > >   }
> > > > >   }
> > > > >  
> > > > > + for_each_connector_in_state(state, connector,
> > > > > connector_state, i) {
> > > > > + const struct drm_connector_helper_funcs
> > > > > *conn_funcs;
> > > > > + struct drm_crtc_state *crtc_state;
> > > > > +
> > > > > + conn_funcs = connector->helper_private;
> > > > > + if (!conn_funcs->atomic_release)
> > > > > + continue;
> > > > > +
> > > > > + if (!connector->state->crtc)
> > > > > + continue;
> > > > > +
> > > > > + crtc_state =
> > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > >crtc);
> > > > > +
> > > > > + if (crtc_state->connectors_changed ||
> > > > > + crtc_state->mode_changed ||
> > > > > + (crtc_state->active_changed && !crtc_state-
> > > > > > 
> > > > > > active))
> > > > > + conn_funcs->atomic_release(connector,
> > > > > connector_state);
> > > > > + }
> > > > 
> > > > Could we deal with the VCPI state separately in
> > > > intel_modeset_checks,
> > > > like we do with dpll?
> > > 
> > > We'd want to release the VCPI slots before they are acquired in
> > > ->compute_config(). intel_modeset_checks() will be too late to
> > > release
> > > them. Are you suggesting both acquiring and releasing slots should be
> > > done in intel_modeset_checks()?
> > 
> > That makes things a bit more nasty. Maybe add a
> > conn_funcs->atomic_check that always gets called, something like I did
> > below?
> > 
> > I'd love to use it for some atomic connector properties too.
> 
> 
> Adding and unconditionally calling conn_funcs->atomic_check() should be
> doable. It also follows the pattern we have for encoders and CRTCs. But
> I'll have to move the connector->state->crtc state checks inside the
> function.
> 
> -DK


This is what I mean -https://pastebin.ubuntu.com/23991405/
But, I do have one concern with calling this conn_func->atomic_check().
We are not validating the new connector_state like atomic_check() seems
to do generally but only cleaning up vcpi resources for compute_config()
to later acquire. Let me know if I am wrong in my understanding what
atomic_check() is expected to do.


-DK
> > > > 
> > > > 
> > > > Maybe implementing the relevant VCPI state could be done as an
> > > > atomic
> > > > helper function too, so other atomic drivers can just plug it in.
> > > > 
> > > The idea was to reduce boilerplate in the drivers and use the
> > > private_obj state for different object types.
> > > 
> > > 
> > > > 
> > > > Not sure how doable this is, but if it's not too hard, then it's
> > > > probably cleaner :)
> > > > ___
> > > > Intel-gfx mailing list
> > > > intel-...@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > ___
> > Intel-gfx mailing list
> > intel-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Pandiyan, Dhinakaran]

On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > Having a ->atomic_release callback is useful to release shared
> > resources
> > that get allocated in compute_config(). This function is expected to
> > be
> > called in the atomic_check() phase before new resources are acquired.
> > 
> > v2: Moved the caller hunk to this patch (Daniel)
> > 
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c  | 19 +++
> >  include/drm/drm_modeset_helper_vtables.h | 13 +
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8795088..92bd741 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > drm_device *dev,
> > }
> > }
> >  
> > +   for_each_connector_in_state(state, connector,
> > connector_state, i) {
> > +   const struct drm_connector_helper_funcs *conn_funcs;
> > +   struct drm_crtc_state *crtc_state;
> > +
> > +   conn_funcs = connector->helper_private;
> > +   if (!conn_funcs->atomic_release)
> > +   continue;
> > +
> > +   if (!connector->state->crtc)
> > +   continue;
> > +
> > +   crtc_state =
> > drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
> > +
> > +   if (crtc_state->connectors_changed ||
> > +   crtc_state->mode_changed ||
> > +   (crtc_state->active_changed && !crtc_state-
> > >active))
> > +   conn_funcs->atomic_release(connector,
> > connector_state);
> > +   }
> 
> Could we deal with the VCPI state separately in intel_modeset_checks,
> like we do with dpll?

We'd want to release the VCPI slots before they are acquired in
->compute_config(). intel_modeset_checks() will be too late to release
them. Are you suggesting both acquiring and releasing slots should be
done in intel_modeset_checks()?


> 
> Maybe implementing the relevant VCPI state could be done as an atomic
> helper function too, so other atomic drivers can just plug it in.
> 
The idea was to reduce boilerplate in the drivers and use the
private_obj state for different object types.


> Not sure how doable this is, but if it's not too hard, then it's
> probably cleaner :)
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v3 4/8] drm: Add driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Thu, 2017-02-09 at 08:08 +, Chris Wilson wrote:
> On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote:
> > +#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, 
> > __funcs) \
> > +   for ((__i) = 0; \
> > +(__i) < (__state)->num_private_objs && \
> > +((obj) = (__state)->private_objs[__i].obj, \
> > +(__funcs) = (__state)->private_objs[__i].funcs,\
> > +(obj_state) = (__state)->private_objs[__i].obj_state, 1);  \
> 
> Align to ( and put the trailing 1 on its own line so it stands out.

Sure, will do that. Looks like I have to change other macros in that
file too.

-DK
> 
>(__i) < (__state)->num_private_objs && \
>((obj) = (__state)->private_objs[__i].obj, \
> (__funcs) = (__state)->private_objs[__i].funcs,   \
> (obj_state) = (__state)->private_objs[__i].obj_state, \
> 1);   \
>(__i)++)   \
> > +   for_each_if (__funcs == obj_funcs)
> > +
> > +/**
> >   * drm_atomic_crtc_needs_modeset - compute combined modeset need
> >   * @state: _crtc_state for the CRTC
> >   *
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/6] drm: scrambling support in drm layer

[Pandiyan, Dhinakaran]

On Wed, 2017-02-01 at 18:14 +0530, Shashank Sharma wrote:
> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
> than 340Mhz. This patch adds few new functions in drm layer for
> core drivers to enable/disable scrambling.
> 
> This patch adds:
> - A function to detect scrambling support parsing HF-VSDB
> - A function to check scrambling status runtime using SCDC read.
> - Two functions to enable/disable scrambling using SCDC read/write.
> - Few new bools to reflect scrambling support and status.
> 
> Signed-off-by: Shashank Sharma 
> ---
>  drivers/gpu/drm/drm_edid.c  | 131 
> +++-
>  include/drm/drm_connector.h |  24 
>  include/drm/drm_edid.h  |   6 +-
>  3 files changed, 159 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 37902e5..f0d940a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define version_greater(edid, maj, min) \
>   (((edid)->version > (maj)) || \
> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector 
> *connector,
>   }
>  }
>  
> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> +  const u8 *hf_vsdb)
> +{
> + struct drm_display_info *display = >display_info;
> + struct drm_hdmi_info *hdmi = >hdmi_info;
> +
> + /*
> +  * All HDMI 2.0 monitors must support scrambling at rates > 340M.
> +  * And as per the spec, three factors confirm this:
> +  * * Availability of a HF-VSDB block in EDID (check)
> +  * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB
> +  * * SCDC support available
> +  * Lets check it out.
> +  */
> +
> + if (hf_vsdb[5]) {
> + display->max_tmds_clock = hf_vsdb[5] * 5000;

Comment explaining or quoting where the 5000 comes from?

> + DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> + display->max_tmds_clock);
> +
> + if (hdmi->scdc_supported) {
> + hdmi->scr_info.supported = true;
> +
> + /* Few sinks support scrambling for cloks < 340M */
> + if ((hf_vsdb[6] & 0x8))
> + hdmi->scr_info.low_clocks = true;
> + }
> + }
> +}
> +
> +/**
> + * drm_check_scrambling_status - what is status of scrambling?
> + * @adapter: i2c adapter for SCDC channel
> + *
> + * Read the scrambler status over SCDC channel, and check the
> + * scrambling status.
> + *
> + * Return: True if the scrambling is enabled, false otherwise.
> + */
> +
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
> +{
> + u8 status;
> +
> + if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, ) < 0) {
> + DRM_ERROR("Failed to read scrambling status\n");
> + return false;
> + }
> +
> + status &= SCDC_SCRAMBLING_STATUS;
> + return status != 0;
> +}
> +
> +/**
> + * drm_enable_scrambling - enable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: enable scrambling, even if its already enabled
> + *
> + * Write the TMDS config over SCDC channel, and enable scrambling
> + * Return: True if scrambling is successfully enabled, false otherwise.
> + */
> +
> +bool drm_enable_scrambling(struct drm_connector *connector,
> + struct i2c_adapter *adapter, bool force)
> +{
> + u8 config;
> + struct drm_hdmi_info *hdmi_info = >display_info.hdmi_info;
> +
> + if (hdmi_info->scr_info.status && !force) {
> + DRM_DEBUG_KMS("Scrambling already enabled\n");
> + return true;
> + }
> +
> + if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, ) < 0) {
> + DRM_ERROR("Failed to read tmds config\n");
> + return false;
> + }
> +
> + config |= SCDC_SCRAMBLING_ENABLE;
> +
> + if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) {
> + DRM_ERROR("Failed to enable scrambling, write error\n");
> + return false;
> + }
> +
> + hdmi_info->scr_info.status = drm_check_scrambling_status(adapter);
> + return hdmi_info->scr_info.status;
> +}
> +
> +/**
> + * drm_disable_scrambling - disable scrambling
> + * @connector: target drm_connector
> + * @adapter: i2c adapter for SCDC channel
> + * @force: disable scrambling, even if its already disabled
> + *
> + * Write the TMDS config over SCDC channel, and disable scrambling
> + * Return: True if scrambling is successfully disabled, false otherwise.
> + */
> +bool drm_disable_scrambling(struct drm_connector *connector,
> + struct i2c_adapter *adapter, bool force)
> +{
> + u8 config;
> + struct drm_hdmi_info *hdmi_info = >display_info.hdmi_info;
> +
> + if 

Re: [Intel-gfx] [PATCH v2 7/9] drm: Connector helper function to release atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:18 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:35PM -0800, Dhinakaran Pandiyan wrote:
> > Having a ->atomic_release callback is useful to release shared resources
> > that get allocated in compute_config().
> > 
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  include/drm/drm_modeset_helper_vtables.h | 15 +++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h 
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 46f5b34..e41b18a 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -831,6 +831,21 @@ struct drm_connector_helper_funcs {
> >  */
> > struct drm_encoder *(*atomic_best_encoder)(struct drm_connector 
> > *connector,
> >struct drm_connector_state 
> > *connector_state);
> > +
> > +   /**
> > +* @atomic_release:
> > +*
> > +* This function is used to release shared resources that were
> > +* previously acquired. For example, resources acquired in
> > +* encoder->compute_config() can be released by calling this function
> 
> @compute_config is the right way to do references within the same struct.

compute_config is not in the same structure, which made me realize I
should not be referring to compute_config() at all, as it is a helper
for struct intel_encoder. 


-DK
> 
> > +* from mode_fixup()
> 
> Same here.
> 
> Patch split up is a bit strange, hence why my review of the design is in
> later patches.
> 
> Thanks, Daniel
> 
> > +*
> > +* NOTE:
> > +*
> > +* This function is called in the check phase of an atomic update.
> > +*/
> > +   void (*atomic_release)(struct drm_connector *connector,
> > +  struct drm_connector_state *connector_state);
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 2/9] drm/dp: Kill unused MST vcpi slot availability tracking

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 06:43 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:30PM -0800, Dhinakaran Pandiyan wrote:
> > The avail_slots member in the MST topology manager is never updated to
> > reflect the available vcpi slots. The check is effectively against
> > total_slots. So, let's make that check obvious. Secondly, since the total
> > vcpi time slots is always 63 and does not depend on the link BW, remove
> > total_slots from MST topology manager struct. The third change is to
> > remove total_pbn which is hardcoded to 2560. The total PBN that the
> > topology manager allocates from depends on the link rate and is not a
> > constant. So, fix this by removing the total_pbn member itself. Finally,
> > make debug messages more descriptive.
> 
> Ok, these are 3 different changes, and they need to be split up. Well, at
> least in 2 patches, to get the functional change out of there:
> - First hardcode avail_slots to 63, with the commit message explaining in
>   detail why that's the right thing. You can already remove total_pbn and
>   total_slots in that patch, since they will be unused. The commit message
>   should have a reference to the DP spec to support that "it's always 63"
>   claim.
> - Then garbage-collect ->avail_slots in a 2nd patch.
> 
> If you smash these together it's a lot less obvious that this is not just
> a pure refactoring.
> 
> Thanks, Daniel
> 

Makes sense, will do this in the next revision.

-DK
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 16 
> >  include/drm/drm_dp_mst_helper.h   | 12 
> >  2 files changed, 8 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 122a1b0..d9edd84 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2042,10 +2042,6 @@ int drm_dp_mst_topology_mgr_set_mst(struct 
> > drm_dp_mst_topology_mgr *mgr, bool ms
> > goto out_unlock;
> > }
> >  
> > -   mgr->total_pbn = 2560;
> > -   mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> > -   mgr->avail_slots = mgr->total_slots;
> > -
> > /* add initial branch device at LCT 1 */
> > mstb = drm_dp_add_mst_branch_device(1, NULL);
> > if (mstb == NULL) {
> > @@ -2475,7 +2471,8 @@ int drm_dp_find_vcpi_slots(struct 
> > drm_dp_mst_topology_mgr *mgr,
> >  
> > num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >  
> > -   if (num_slots > mgr->avail_slots)
> > +   /* max. time slots - one slot for MTP header */
> > +   if (num_slots > 63)
> > return -ENOSPC;
> > return num_slots;
> >  }
> > @@ -2489,7 +2486,8 @@ static int drm_dp_init_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr,
> >  
> > num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >  
> > -   if (num_slots > mgr->avail_slots)
> > +   /* max. time slots - one slot for MTP header */
> > +   if (num_slots > 63)
> > return -ENOSPC;
> >  
> > vcpi->pbn = pbn;
> > @@ -2528,10 +2526,12 @@ bool drm_dp_mst_allocate_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> >  
> > ret = drm_dp_init_vcpi(mgr, >vcpi, pbn);
> > if (ret) {
> > -   DRM_DEBUG_KMS("failed to init vcpi %d %d %d\n", 
> > DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->avail_slots, ret);
> > +   DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n",
> > +   DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> > goto out;
> > }
> > -   DRM_DEBUG_KMS("initing vcpi for %d %d\n", pbn, port->vcpi.num_slots);
> > +   DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
> > +   pbn, port->vcpi.num_slots);
> > *slots = port->vcpi.num_slots;
> >  
> > drm_dp_put_port(port);
> > diff --git a/include/drm/drm_dp_mst_helper.h 
> > b/include/drm/drm_dp_mst_helper.h
> > index 27f3e99..b0f4a09 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -479,18 +479,6 @@ struct drm_dp_mst_topology_mgr {
> >  * @pbn_div: PBN to slots divisor.
> >  */
> > int pbn_div;
> > -   /**
> > -* @total_slots: Total slots that can be allocated.
> > -*/
> > -   int total_slots;
> > -   /**
> > -* @avail_slots: Still available slots that can be allocated.
> > -*/
> > -   int avail_slots;
> > -   /**
> > -* @total_pbn: Total PBN count.
> > -*/
> > -   int total_pbn;
> >  
> > /**
> >  * @qlock: protects @tx_msg_downq, the tx_slots in struct
> > -- 
> > 2.7.4
> > 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 7/9] drm: Connector helper function to release atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:18 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:35PM -0800, Dhinakaran Pandiyan wrote:
> > Having a ->atomic_release callback is useful to release shared resources
> > that get allocated in compute_config().
> > 
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  include/drm/drm_modeset_helper_vtables.h | 15 +++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h 
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 46f5b34..e41b18a 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -831,6 +831,21 @@ struct drm_connector_helper_funcs {
> >  */
> > struct drm_encoder *(*atomic_best_encoder)(struct drm_connector 
> > *connector,
> >struct drm_connector_state 
> > *connector_state);
> > +
> > +   /**
> > +* @atomic_release:
> > +*
> > +* This function is used to release shared resources that were
> > +* previously acquired. For example, resources acquired in
> > +* encoder->compute_config() can be released by calling this function
> 
> @compute_config is the right way to do references within the same struct.
> 
> > +* from mode_fixup()
> 
> Same here.
> 
> Patch split up is a bit strange, hence why my review of the design is in
> later patches.
> 
> Thanks, Daniel
> 


I'll squash them appropriately, thanks for the review.

-DK
> > +*
> > +* NOTE:
> > +*
> > +* This function is called in the check phase of an atomic update.
> > +*/
> > +   void (*atomic_release)(struct drm_connector *connector,
> > +  struct drm_connector_state *connector_state);
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 9/9] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:15 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:37PM -0800, Dhinakaran Pandiyan wrote:
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c |  9 -
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 13 -
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index dd34d21..0d42173 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -385,8 +385,15 @@ mode_fixup(struct drm_atomic_state *state)
> >  
> > WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
> >  
> > -   if (!conn_state->crtc || !conn_state->best_encoder)
> > +   if (!conn_state->crtc || !conn_state->best_encoder) {
> > +   const struct drm_connector_helper_funcs *conn_funcs;
> > +
> > +   conn_funcs = connector->helper_private;
> > +   if (conn_funcs->atomic_release)
> > +   conn_funcs->atomic_release(connector,
> > +  conn_state);
> 
> I wonder whether this won't surprise drivers: The way you coded this
> release only gets called when the connector is fully disabled. But it does
> not get called when you atomically switch it from one crtc to the other
> (in which case you also might want to release resources, before allocating
> new ones). I think that case of directly switching stuff is even a bug in
> your current atomic tracking code ...
> 
> We also need to extract the release calls into a separate loop which runs
> _before_ we allocate any resources. Otherwise if you do an atomic commit
> where you disable connector2 and enable connector1 and they can't run both
> at the same time it'll fail, because we'll release the vcpi for connector2
> only after we've tried to get it for connnector1.
> 


Yeah, you are right. I thought of this problem and then forgot about it.
Is there any igt test to test the switching?


-DK
> > continue;
> > +   }
> >  
> > crtc_state = drm_atomic_get_existing_crtc_state(state,
> > 
> > conn_state->crtc);
> 
> Misplaced hunk, this needs to be in the patch that adds the
> ->atomic_release callback.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 2f57a56..ee5fedb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct 
> > intel_encoder *encoder,
> > int lane_count, slots;
> > const struct drm_display_mode *adjusted_mode = 
> > _config->base.adjusted_mode;
> > int mst_pbn;
> > +   struct drm_dp_mst_topology_state *topology_state;
> >  
> > pipe_config->has_pch_encoder = false;
> > bpp = 24;
> > @@ -65,7 +66,17 @@ static bool intel_dp_mst_compute_config(struct 
> > intel_encoder *encoder,
> > mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  
> > pipe_config->pbn = mst_pbn;
> > -   slots = drm_dp_find_vcpi_slots(_dp->mst_mgr, mst_pbn);
> > +   topology_state = drm_atomic_get_mst_topology_state(state,
> > +  _dp->mst_mgr);
> > +   if (topology_state == NULL)
> > +   return false;
> > +
> > +   slots = drm_dp_atomic_find_vcpi_slots(topology_state, connector->port,
> > + mst_pbn);
> 
> Can we merge the get_mst_topology_state and find_vcpi_slots functions
> together? Would be less code in drivers ...
> 
> > +   if (slots < 0) {
> > +   DRM_DEBUG_KMS("not enough link bw for this mode\n");
> > +   return false;
> > +   }
> 
> And then also stuff the error checking in there, and just have a return
> -EINVAL when there's not enough bw.
> 
> I think this should be together with the previous patch, to wire up the
> entire mst state tracking for i915 at the same time. Atm the previous
> patch just wires up dead code, which is a bit backwards.
> -Daniel
> 
> >  
> > intel_link_compute_m_n(bpp, lane_count,
> >adjusted_mode->crtc_clock,
> > -- 
> > 2.7.4
> > 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 8/9] drm/dp: Release DP MST shared link bandwidth

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:16 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:36PM -0800, Dhinakaran Pandiyan wrote:
> > Implement the ->atomic_release() callback to release the shared link
> > bandwidth that was originally acquired during compute_config()
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 28 
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index f51574f..2f57a56 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -78,6 +78,33 @@ static bool intel_dp_mst_compute_config(struct 
> > intel_encoder *encoder,
> >  
> >  }
> >  
> > +static void intel_dp_mst_atomic_release(struct drm_connector *connector,
> > +   struct drm_connector_state *conn_state)
> > +{
> > +   struct intel_dp_mst_encoder *intel_mst;
> > +   struct drm_dp_mst_topology_mgr *mgr;
> > +   struct drm_dp_mst_topology_state *topology_state;
> > +   struct drm_encoder *encoder;
> > +   struct intel_connector *intel_connector = to_intel_connector(connector);
> > +
> > +   encoder = connector->state->best_encoder;
> > +   if (!encoder || to_intel_encoder(encoder)->type != INTEL_OUTPUT_DP_MST)
> > +   return;
> 
> NULL encoder should imo be caught in core. Type != DP_MST is impossible,
> if you're unsure make it into a BUG_ON for testing.


I don't get why (type != INTEL_OUTPUT_DP_MST) is impossible. Is that
because we have the implementation for atomic_release() only for MST
connectors? But, that is only for now.

-DK
> > +
> > +   intel_mst = enc_to_mst(encoder);
> > +   mgr = _mst->primary->dp.mst_mgr;
> > +
> > +   topology_state = drm_atomic_get_mst_topology_state(conn_state->state,
> > +  mgr);
> > +   if (IS_ERR(topology_state)) {
> > +   DRM_DEBUG_KMS("failed to create MST topology state %ld\n",
> > +   PTR_ERR(topology_state));
> > +   return;
> > +   }
> > +
> > +   drm_dp_atomic_release_vcpi_slots(topology_state, intel_connector->port);
> 
> I think it'd be great to merge this together into 1 helper that both gets
> the state and releases the vcpi, for less code in drivers.

Agreed.

> > +}
> > +
> >  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> >  struct intel_crtc_state *old_crtc_state,
> >  struct drm_connector_state *old_conn_state)
> > @@ -401,6 +428,7 @@ static const struct drm_connector_helper_funcs 
> > intel_dp_mst_connector_helper_fun
> > .mode_valid = intel_dp_mst_mode_valid,
> > .atomic_best_encoder = intel_mst_atomic_best_encoder,
> > .best_encoder = intel_mst_best_encoder,
> > +   .atomic_release = intel_dp_mst_atomic_release,
> >  };
> >  
> >  static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
> > -- 
> > 2.7.4
> > 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 06:59 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> > 
> > Suggested-by: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> 
> I think an overview DOC: section to explain how to subclass atomic state
> and how to do private state objects would be great. But I can do that once
> your patch has landed, since it'd be much more about the overall design of
> atomic (and I promised to do that anyway).
> 
> Looks pretty good, a few nits below still. I'll also ask amd folks to
> check this out, and I think Ville could use it for his cdclk stuff.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 55 
> > +
> >  drivers/gpu/drm/drm_atomic_helper.c |  6 
> >  include/drm/drm_atomic.h| 30 
> >  3 files changed, 91 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 723392f..f3a71cc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct 
> > drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > +   kfree(state->private_objs);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >  
> > @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct 
> > drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > +   for (i = 0; i < state->num_private_objs; i++) {
> > +   void *priv_obj = state->private_objs[i].obj;
> > +   void *obj_state = state->private_objs[i].obj_state;
> > +
> > +   if (!priv_obj)
> > +   continue;
> > +
> > +   state->private_objs[i].funcs->destroy_state(obj_state);
> > +   state->private_objs[i].obj = NULL;
> > +   state->private_objs[i].obj_state = NULL;
> > +   state->private_objs[i].funcs = NULL;
> > +   }
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >  
> > @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct 
> > drm_printer *p,
> > plane->funcs->atomic_print_state(p, state);
> >  }
> >  
> > +
> 
> Needs kerneldoc here.
> 
> > +void *
> > +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj,
> 
> ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to
> one (I don't care which one).
> 
> > + const struct drm_private_state_funcs *funcs)
> > +{
> > +   int index, num_objs, i;
> > +   size_t size;
> > +   struct __drm_private_objs_state *arr;
> > +
> > +   for (i = 0; i < state->num_private_objs; i++)
> > +   if (obj == state->private_objs[i].obj &&
> > +   state->private_objs[i].obj_state)
> > +   return state->private_objs[i].obj_state;
> > +
> > +   num_objs = state->num_private_objs + 1;
> > +   size = sizeof(*state->private_objs) * num_objs;
> > +   arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > +   if (!arr)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   state->private_objs = arr;
> > +   index = state->num_private_objs;
> > +   memset(>private_objs[index], 0, sizeof(*state->private_objs));
> > +
> > +   state->private_objs[index].obj_state = funcs->duplicate_state(state, 
> > obj);
> > +   if (!state->private_objs[index].obj_state)
> > +   return ERR_PTR(-ENOMEM);
> 
> I wondered whether we need to allow other error codes than ENOMEM, e.g.
> if duplicate_state needs to acquire a ww_mutex. But we can always acquire
> the necessary locks outside of drm_atomic_get_priv_obj_state in some
> helper/driver wrapper. So no big deal, but worth explaining that
> drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it
> doesn't know about them) in the kernel-doc.
> 

Got it, will include that.

> > +
> > +   state->private_objs[index].obj = obj;
> > +   state->private_objs[index].funcs = funcs;
> > +   state->num_private_objs = num_objs;
> > +
> > +   DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> > +

Re: [Intel-gfx] [PATCH v2 3/9] drm/dp: Split drm_dp_mst_allocate_vcpi

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 10:31 +1000, Dave Airlie wrote:
> On 25 January 2017 at 09:49, Dhinakaran Pandiyan
>  wrote:
> > drm_dp_mst_allocate_vcpi() apart from setting up the vcpi structure,
> > also finds if there are enough slots available. This check is a duplicate
> > of that implemented in drm_dp_mst_find_vcpi_slots(). Let's move this check
> > out and reuse the existing drm_dp_mst_find_vcpi_slots() function to check
> > if there are enough vcpi slots before allocating them.
> >
> > This brings the check to one place. Additionally drivers that will use MST
> > state tracking for atomic modesets can use the atomic version of
> > find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()
> >
> 
> Also seem sane, at least for the core bits,
> 
> Reviewed-by: Dave Airlie 
> 

Thanks for the review.

-DK


> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c  | 20 +---
> >  drivers/gpu/drm/i915/intel_dp_mst.c|  4 ++--
> >  drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
> >  drivers/gpu/drm/radeon/radeon_dp_mst.c |  4 +++-
> >  include/drm/drm_dp_mst_helper.h|  2 +-
> >  5 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d9edd84..b871d4e 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2479,20 +2479,17 @@ int drm_dp_find_vcpi_slots(struct 
> > drm_dp_mst_topology_mgr *mgr,
> >  EXPORT_SYMBOL(drm_dp_find_vcpi_slots);
> >
> >  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > -   struct drm_dp_vcpi *vcpi, int pbn)
> > +   struct drm_dp_vcpi *vcpi, int pbn, int slots)
> >  {
> > -   int num_slots;
> > int ret;
> >
> > -   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > -
> > /* max. time slots - one slot for MTP header */
> > -   if (num_slots > 63)
> > +   if (slots > 63)
> > return -ENOSPC;
> >
> > vcpi->pbn = pbn;
> > -   vcpi->aligned_pbn = num_slots * mgr->pbn_div;
> > -   vcpi->num_slots = num_slots;
> > +   vcpi->aligned_pbn = slots * mgr->pbn_div;
> > +   vcpi->num_slots = slots;
> >
> > ret = drm_dp_mst_assign_payload_id(mgr, vcpi);
> > if (ret < 0)
> > @@ -2507,7 +2504,7 @@ static int drm_dp_init_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr,
> >   * @pbn: payload bandwidth number to request
> >   * @slots: returned number of slots for this PBN.
> >   */
> > -bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> > drm_dp_mst_port *port, int pbn, int *slots)
> > +bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> > drm_dp_mst_port *port, int pbn, int slots)
> >  {
> > int ret;
> >
> > @@ -2515,16 +2512,18 @@ bool drm_dp_mst_allocate_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > if (!port)
> > return false;
> >
> > +   if (slots < 0)
> > +   return false;
> > +
> > if (port->vcpi.vcpi > 0) {
> > DRM_DEBUG_KMS("payload: vcpi %d already allocated for pbn 
> > %d - requested pbn %d\n", port->vcpi.vcpi, port->vcpi.pbn, pbn);
> > if (pbn == port->vcpi.pbn) {
> > -   *slots = port->vcpi.num_slots;
> > drm_dp_put_port(port);
> > return true;
> > }
> > }
> >
> > -   ret = drm_dp_init_vcpi(mgr, >vcpi, pbn);
> > +   ret = drm_dp_init_vcpi(mgr, >vcpi, pbn, slots);
> > if (ret) {
> > DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 
> > ret=%d\n",
> > DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> > @@ -2532,7 +2531,6 @@ bool drm_dp_mst_allocate_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > }
> > DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
> > pbn, port->vcpi.num_slots);
> > -   *slots = port->vcpi.num_slots;
> >
> > drm_dp_put_port(port);
> > return true;
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 38e3ca2..f51574f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -147,7 +147,6 @@ static void intel_mst_pre_enable_dp(struct 
> > intel_encoder *encoder,
> > to_intel_connector(conn_state->connector);
> > int ret;
> > uint32_t temp;
> > -   int slots;
> >
> > /* MST encoders are bound to a crtc, not to a connector,
> >  * force the mapping here for get_hw_state.
> > @@ -177,7 +176,8 @@ static void intel_mst_pre_enable_dp(struct 
> > intel_encoder *encoder,
> >
> > ret = 

[Intel-gfx] [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Pandiyan, Dhinakaran]

On Wed, 2017-01-04 at 19:20 +, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> > On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > > Link bandwidth is shared between multiple display streams in DP MST
> > > configurations. The DP MST topology manager structure maintains the shared
> > > link bandwidth for a primary link directly connected to the GPU. For 
> > > atomic
> > > modesetting drivers, checking if there is sufficient link bandwidth for a
> > > mode needs to be done during the atomic_check phase to avoid failed
> > > modesets. Let's encsapsulate the available link bw information in a state
> > > structure so that bw can be allocated and released atomically for each of
> > > the ports sharing the primary link.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan 
> > 
> > Overall issue with the patch is that dp helpers now have 2 places where
> > available_slots is stored: One for atomic drivers in ->state, and the
> > legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> > drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> > mgr->avail_slots entirely.
> 
> PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
> path, so the check turns out to be against mgr->total_slots. So, I did
> just that, albeit explicitly. 
> 
> -DK
> 
> > 
> > Another design concern below, but in principle this looks like what we
> > need.
> > -Daniel
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c  | 66 
> > > +++
> > >  drivers/gpu/drm/drm_atomic_helper.c   | 10 ++
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> > >  include/drm/drm_atomic.h  | 13 +++
> > >  include/drm/drm_dp_mst_helper.h   | 13 +++
> > >  5 files changed, 112 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 681d5f9..b8e2cea 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -31,6 +31,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  
> > >  #include "drm_crtc_internal.h"
> > > @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct 
> > > drm_atomic_state *state)
> > >   kfree(state->connectors);
> > >   kfree(state->crtcs);
> > >   kfree(state->planes);
> > > + kfree(state->dp_mst_topologies);
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> > >  
> > > @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct 
> > > drm_atomic_state *state)
> > >   state->planes[i].ptr = NULL;
> > >   state->planes[i].state = NULL;
> > >   }
> > > +
> > > + for (i = 0; i < state->num_mst_topologies; i++) {
> > > + struct drm_dp_mst_topology_mgr *mgr = 
> > > state->dp_mst_topologies[i].ptr;
> > > +
> > > + if (!mgr)
> > > + continue;
> > > +
> > > + kfree(state->dp_mst_topologies[i].state);
> > > + state->dp_mst_topologies[i].ptr = NULL;
> > > + state->dp_mst_topologies[i].state = NULL;
> > > + }
> > > +
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> > >  
> > > @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct 
> > > drm_printer *p,
> > >   plane->funcs->atomic_print_state(p, state);
> > >  }
> > >  
> > > +struct drm_dp_mst_topology_state 
> > > *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > + struct drm_dp_mst_topology_mgr *mgr)
> > > +{
> > > +
> > > + int ret, i;
> > > + size_t new_size;
> > > + struct __drm_dp_mst_topology_state *new_arr;
> > > + struct drm_dp_mst_topology_state *new_mst_state;
> > > + int num_topologies;
> > > + struct drm_mode_config *config = >dev->mode_config;
> > > +
> > > + WARN_ON(!state->acquire_ctx);
> > > +
> > > + ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + for (i = 0; i &l

[Intel-gfx] [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Pandiyan, Dhinakaran]

On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > Link bandwidth is shared between multiple display streams in DP MST
> > configurations. The DP MST topology manager structure maintains the shared
> > link bandwidth for a primary link directly connected to the GPU. For atomic
> > modesetting drivers, checking if there is sufficient link bandwidth for a
> > mode needs to be done during the atomic_check phase to avoid failed
> > modesets. Let's encsapsulate the available link bw information in a state
> > structure so that bw can be allocated and released atomically for each of
> > the ports sharing the primary link.
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> 
> Overall issue with the patch is that dp helpers now have 2 places where
> available_slots is stored: One for atomic drivers in ->state, and the
> legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> mgr->avail_slots entirely.

PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
path, so the check turns out to be against mgr->total_slots. So, I did
just that, albeit explicitly. 

-DK

> 
> Another design concern below, but in principle this looks like what we
> need.
> -Daniel
> 
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c  | 66 
> > +++
> >  drivers/gpu/drm/drm_atomic_helper.c   | 10 ++
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> >  include/drm/drm_atomic.h  | 13 +++
> >  include/drm/drm_dp_mst_helper.h   | 13 +++
> >  5 files changed, 112 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 681d5f9..b8e2cea 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -31,6 +31,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct 
> > drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > +   kfree(state->dp_mst_topologies);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_release);
> >  
> > @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct 
> > drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > +   for (i = 0; i < state->num_mst_topologies; i++) {
> > +   struct drm_dp_mst_topology_mgr *mgr = 
> > state->dp_mst_topologies[i].ptr;
> > +
> > +   if (!mgr)
> > +   continue;
> > +
> > +   kfree(state->dp_mst_topologies[i].state);
> > +   state->dp_mst_topologies[i].ptr = NULL;
> > +   state->dp_mst_topologies[i].state = NULL;
> > +   }
> > +
> >  }
> >  EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >  
> > @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct 
> > drm_printer *p,
> > plane->funcs->atomic_print_state(p, state);
> >  }
> >  
> > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct 
> > drm_atomic_state *state,
> > +   struct drm_dp_mst_topology_mgr *mgr)
> > +{
> > +
> > +   int ret, i;
> > +   size_t new_size;
> > +   struct __drm_dp_mst_topology_state *new_arr;
> > +   struct drm_dp_mst_topology_state *new_mst_state;
> > +   int num_topologies;
> > +   struct drm_mode_config *config = >dev->mode_config;
> > +
> > +   WARN_ON(!state->acquire_ctx);
> > +
> > +   ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
> > +   if (ret)
> > +   return ERR_PTR(ret);
> > +
> > +   for (i = 0; i < state->num_mst_topologies; i++) {
> > +   if (mgr == state->dp_mst_topologies[i].ptr &&
> > +   state->dp_mst_topologies[i].state) {
> > +   return state->dp_mst_topologies[i].state;
> > +   }
> > +   }
> > +
> > +   num_topologies = state->num_mst_topologies + 1;
> > +   new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
> > +   new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
> > +   if (!new_arr)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   state->dp_mst_topologies = new_arr;
> > +   memset(>dp_mst_topologies[state->num_mst_topologies], 0,
> > +   sizeof(*state->dp_mst_topologies));
> > +
> > +   new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
> > +   if (!new_mst_state)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   new_mst_state->avail_slots = mgr->state->avail_slots;
> > +   state->dp_mst_topologies[state->num_mst_topologies].state = 
> > new_mst_state;
> > +   state->dp_mst_topologies[state->num_mst_topologies].ptr = mgr;
> > +   state->num_mst_topologies = num_topologies;
> > +   new_mst_state->mgr = mgr;
> > +   mgr->state->state = state;
> > +
> > +  

[Intel-gfx] [PATCH v2 1/2] drm: Wrap the check for atomic_commit implementation

[Pandiyan, Dhinakaran]

Done, hope I got it right this time. 

-DK

-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, December 22, 2016 12:52 AM
To: Ander Conselvan De Oliveira 
Cc: Pandiyan, Dhinakaran ; intel-gfx at 
lists.freedesktop.org; Daniel Vetter ; dri-devel at 
lists.freedesktop.org; Ben Skeggs ; nouveau at 
lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 1/2] drm: Wrap the check for atomic_commit 
implementation

On Thu, Dec 22, 2016 at 10:36:01AM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2016-12-21 at 12:12 -0800, Dhinakaran Pandiyan wrote:
> > This check is useful for drivers that do not have DRIVER_ATOMIC set 
> > but have atomic modesetting internally implemented. Wrap the check 
> > into a function since this is used in many places and as a bonus, 
> > the function name helps to document what the check is for.
> > 
> > v2:
> > Change return type to bool (Ville)
> > Move the function drm_atomic.h (Daniel)
> > 
> > Suggested-by: Daniel Vetter 
> > Cc: Daniel Vetter 
> > Cc: Ben Skeggs 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c             |  6 +++---
> >  drivers/gpu/drm/nouveau/nouveau_connector.c |  5 +++--
> >  drivers/gpu/drm/nouveau/nouveau_display.c   |  6 +++---
> >  drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  3 ++-
> >  include/drm/drm_atomic.h                    | 11 
> > +++
> >  5 files changed, 22 insertions(+), 9 deletions(-)
> > 
> 
> ...
> 
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h 
> > index 8cc7ca2..43db162 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -419,5 +419,16 @@ drm_atomic_crtc_needs_modeset(const struct 
> > drm_crtc_state
> > *state)
> >           state->connectors_changed;
> >  }
> >  
> > +/* drm_drv_uses_atomic_modeset - check if the driver implements
> 
> Shouldn't this be
> 
> /**
>  * drm_drv_uses_atomic_modeset - ...
> 
> so it is included in the generated documentation?

Yup. I'm blind this week it seems. DK, please run

$ make DOCBOOKS="" htmldocs

and make sure your new documentation does get rendered correctly and shows up 
in the output.
-Daniel

> 
> Ander
> 
> 
> > + * atomic_commit()
> > + * @dev: DRM device
> > + *
> > + * This check is useful if drivers do not have DRIVER_ATOMIC set 
> > +but
> > + * have atomic modesetting internally implemented.
> > + */
> > +static inline bool drm_drv_uses_atomic_modeset(struct drm_device 
> > +*dev) {
> > +   return dev->mode_config.funcs->atomic_commit ? true : false; }
> >  
> >  #endif /* DRM_ATOMIC_H_ */

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 0/2] drm: link status property and DP link training failure handling

[Pandiyan, Dhinakaran]

On Sun, 2016-12-18 at 14:43 +0100, Daniel Vetter wrote:
> On Sat, Dec 17, 2016 at 05:47:56AM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2016-12-16 at 16:47 +0200, Jani Nikula wrote:
> > > On Fri, 16 Dec 2016, Daniel Vetter  wrote:
> > > > On Fri, Dec 16, 2016 at 12:29:05PM +0200, Jani Nikula wrote:
> > > >> The two remaining patches from [1], rebased.
> > > >> 
> > > >> BR,
> > > >> Jani.
> > > >> 
> > > >> 
> > > >> [1] 
> > > >> http://mid.mail-archive.com/1480984058-552-1-git-send-email-manasi.d.navare
> > > >>  at intel.com
> > > >
> > > > Just for the record, I think the only thing missing here is the Xorg
> > > > review on the -modesetting patch. As soon as we have that I can vacuum
> > > > this up (probably best through drm-misc, but not sure).
> > > 
> > > Yeah I rebased this (and provided a debug hack privately) so Martin can
> > > test the modesetting changes.
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > 
> > I tested the -modesetting patch, which Martin had provided to Manasi,
> > with a compliance testing device (DPR-120) that can simulate link
> > training failure. The link rate correctly lowered after the link_status
> > property was set to BAD by the kernel and the userspace responded with a
> > modeset. 
> > 
> > One thing that was not straight forward to figure out was I had to boot
> > with i915.nuclear_pageflip=1. Is it documented somewhere that the
> > property needs DRIVER_ATOMIC to be set, or is it implicit?
> 
> It should work without DRIVER_ATOMIC. At least the property should be
> exposed ... If this does only work with DRIVER_ATOMIC set then we have a
> bug somewhere. Can you pls try to figure out why it doesn't work?
> 

The property is exposed even without DRIVER_ATOMIC set, but the value is
always GOOD (0).
We set connector->state->link_status to BAD when link training fails but
the getconnector() ioctl ends up reading obj->properties->values[i] if
DRIVER_ATOMIC is NOT set. But with DRIVER_ATOMIC set, getconnector()
calls into drm_atomic_connector_get_property() and retrieves the value
stored in connector->state->link_status.


> > The other thing I had trouble with -modesetting was, there was no
> > modeset following a long pulse from the sink at the begging of the test.
> > I had to force a modeset by changing the resolution so that the link
> > training path is executed. However, the link training failure induced a
> > modeset without any intervention.
> 
> Sounds roughly like how it's supposed to work. For real mode configuration
> changes the desktop environment is supposed to set the mode it wants, by
> listening to the xrandr hotplug event. That's not the same as the udev
> hotplug event. You can listen for the xrandr hotplug event using
> 
> $ xev -event randr
> 

Got it, -modesetting does indeed send out the hotplug events upon
hotplug.

-DK


> Cheers, Daniel
> 
> > 
> > -DK
> > 
> > 
> > > > -Daniel
> > > >
> > > >> 
> > > >> 
> > > >> Manasi Navare (2):
> > > >>   drm: Add a new connector atomic property for link status
> > > >>   drm/i915: Implement Link Rate fallback on Link training failure
> > > >> 
> > > >>  drivers/gpu/drm/drm_atomic.c  | 16 +
> > > >>  drivers/gpu/drm/drm_atomic_helper.c   | 15 
> > > >>  drivers/gpu/drm/drm_connector.c   | 52 
> > > >> +++
> > > >>  drivers/gpu/drm/i915/intel_dp.c   | 27 ++
> > > >>  drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++--
> > > >>  drivers/gpu/drm/i915/intel_drv.h  |  3 ++
> > > >>  include/drm/drm_connector.h   | 19 ++
> > > >>  include/drm/drm_mode_config.h |  5 +++
> > > >>  include/uapi/drm/drm_mode.h   |  4 +++
> > > >>  9 files changed, 161 insertions(+), 2 deletions(-)
> > > >> 
> > > >> -- 
> > > >> 2.1.4
> > > >> 
> > > >> ___
> > > >> dri-devel mailing list
> > > >> dri-devel at lists.freedesktop.org
> > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > 
> 

[Intel-gfx] [PATCH 0/2] drm: link status property and DP link training failure handling

[Pandiyan, Dhinakaran]

On Fri, 2016-12-16 at 16:47 +0200, Jani Nikula wrote:
> On Fri, 16 Dec 2016, Daniel Vetter  wrote:
> > On Fri, Dec 16, 2016 at 12:29:05PM +0200, Jani Nikula wrote:
> >> The two remaining patches from [1], rebased.
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> [1] 
> >> http://mid.mail-archive.com/1480984058-552-1-git-send-email-manasi.d.navare
> >>  at intel.com
> >
> > Just for the record, I think the only thing missing here is the Xorg
> > review on the -modesetting patch. As soon as we have that I can vacuum
> > this up (probably best through drm-misc, but not sure).
> 
> Yeah I rebased this (and provided a debug hack privately) so Martin can
> test the modesetting changes.
> 
> BR,
> Jani.
> 
> 

I tested the -modesetting patch, which Martin had provided to Manasi,
with a compliance testing device (DPR-120) that can simulate link
training failure. The link rate correctly lowered after the link_status
property was set to BAD by the kernel and the userspace responded with a
modeset. 

One thing that was not straight forward to figure out was I had to boot
with i915.nuclear_pageflip=1. Is it documented somewhere that the
property needs DRIVER_ATOMIC to be set, or is it implicit?

The other thing I had trouble with -modesetting was, there was no
modeset following a long pulse from the sink at the begging of the test.
I had to force a modeset by changing the resolution so that the link
training path is executed. However, the link training failure induced a
modeset without any intervention.

-DK


> > -Daniel
> >
> >> 
> >> 
> >> Manasi Navare (2):
> >>   drm: Add a new connector atomic property for link status
> >>   drm/i915: Implement Link Rate fallback on Link training failure
> >> 
> >>  drivers/gpu/drm/drm_atomic.c  | 16 +
> >>  drivers/gpu/drm/drm_atomic_helper.c   | 15 
> >>  drivers/gpu/drm/drm_connector.c   | 52 
> >> +++
> >>  drivers/gpu/drm/i915/intel_dp.c   | 27 ++
> >>  drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++--
> >>  drivers/gpu/drm/i915/intel_drv.h  |  3 ++
> >>  include/drm/drm_connector.h   | 19 ++
> >>  include/drm/drm_mode_config.h |  5 +++
> >>  include/uapi/drm/drm_mode.h   |  4 +++
> >>  9 files changed, 161 insertions(+), 2 deletions(-)
> >> 
> >> -- 
> >> 2.1.4
> >> 
> >> ___
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

[Intel-gfx] [PATCH 2/3] drm/dp/mst: Calculate total link bandwidth instead of hardcoding it

[Pandiyan, Dhinakaran]

On Tue, 2016-11-29 at 22:58 +0200, Ville Syrjälä wrote:
> On Thu, Nov 17, 2016 at 06:03:47PM -0800, Dhinakaran Pandiyan wrote:
> > The total or the nominal link bandwidth, which we save in terms of PBN, is
> > a factor of link rate and lane count. But, currently we hardcode it to
> > 2560 PBN. This results in incorrect computation of total slots.
> > 
> > E.g, 2 lane HBR2 configuration and 4k at 60Hz, 24bpp mode
> >   nominal link bw = 1080 MBps = 1280PBN = 64 slots
> >   required bw 533.25 MHz*3 = 1599.75 MBps or 1896 PBN
> >  with +0.6% margin = 1907.376 PBN = 96 slots
> >   This is greater than the max. possible value of 64 slots. But, we
> >   incorrectly compute available slots as 2560 PBN = 128 slots and don't
> >   return error.
> > 
> > So, let's fix this by calculating the total link bandwidth as
> > link bw (PBN) = BW per time slot(PBN) * max. time slots , where max. time
> > slots is 64
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 04e4571..26dfd99 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2038,9 +2038,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct 
> > drm_dp_mst_topology_mgr *mgr, bool ms
> > ret = -EINVAL;
> > goto out_unlock;
> > }
> > -
> > -   mgr->total_pbn = 2560;
> > -   mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> > +   mgr->total_pbn = 64 * mgr->pbn_div;
> 
> Do we actually have a use in mind for total_pbn? It seems unused now.

Not really, I will remove it and submit this patch separately.

> 
> > +   mgr->total_slots = 64;
> 
> Same for total_slots.
> 
> > mgr->avail_slots = mgr->total_slots;
> 
> So avail_slots is all that's used. And shouldn't it actually start
> out at 63 instead of 64 on account of the MTP header always taking
> up one slot?
> 

Yeah, I had a check for < avail_slots in the patch that followed.


-DK
> >  
> > /* add initial branch device at LCT 1 */
> > -- 
> > 2.7.4
> 

[Intel-gfx] [PATCH 2/3] drm/dp/mst: Calculate total link bandwidth instead of hardcoding it

[Pandiyan, Dhinakaran]

This patch along with https://patchwork.freedesktop.org/series/15305/
will fix https://bugs.freedesktop.org/show_bug.cgi?id=98141. I'd like
this to be reviewed independently since the other two patches in this
series require rework for atomic support.

-DK

On Thu, 2016-11-17 at 18:03 -0800, Dhinakaran Pandiyan wrote:
> The total or the nominal link bandwidth, which we save in terms of PBN, is
> a factor of link rate and lane count. But, currently we hardcode it to
> 2560 PBN. This results in incorrect computation of total slots.
> 
> E.g, 2 lane HBR2 configuration and 4k at 60Hz, 24bpp mode
>   nominal link bw = 1080 MBps = 1280PBN = 64 slots
>   required bw 533.25 MHz*3 = 1599.75 MBps or 1896 PBN
>  with +0.6% margin = 1907.376 PBN = 96 slots
>   This is greater than the max. possible value of 64 slots. But, we
>   incorrectly compute available slots as 2560 PBN = 128 slots and don't
>   return error.
> 
> So, let's fix this by calculating the total link bandwidth as
> link bw (PBN) = BW per time slot(PBN) * max. time slots , where max. time
> slots is 64
> 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e4571..26dfd99 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2038,9 +2038,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct 
> drm_dp_mst_topology_mgr *mgr, bool ms
>   ret = -EINVAL;
>   goto out_unlock;
>   }
> -
> - mgr->total_pbn = 2560;
> - mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> + mgr->total_pbn = 64 * mgr->pbn_div;
> + mgr->total_slots = 64;
>   mgr->avail_slots = mgr->total_slots;
>  
>   /* add initial branch device at LCT 1 */

[Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

[Pandiyan, Dhinakaran]

On Fri, 2016-11-18 at 06:57 +, Chris Wilson wrote:
> On Thu, Nov 17, 2016 at 06:03:48PM -0800, Dhinakaran Pandiyan wrote:
> >  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > struct drm_dp_vcpi *vcpi, int pbn)
> >  {
> > -   int num_slots;
> > +   int req_slots;
> > int ret;
> >  
> > -   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > +   req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >  
> > -   if (num_slots > mgr->avail_slots)
> > -   return -ENOSPC;
> > +   mutex_lock(>lock);
> > +   if (req_slots > mgr->avail_slots) {
> > +   ret = -ENOSPC;
> > +   goto out;
> > +   }
> 
> You are not atomically reducing the mgr->avail_slots, leading to
> possible overallocation of multiple streams?
> -Chris
> 

Yeah, I got it wrong. I should have reduced mgr->avail_slots here before
releasing the mutex. Thanks for pointing it out.

[Intel-gfx] [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Pandiyan, Dhinakaran]

On Fri, 2016-11-18 at 09:43 +0100, Daniel Vetter wrote:
> On Thu, Nov 17, 2016 at 06:03:46PM -0800, Dhinakaran Pandiyan wrote:
> > drm_dp_find_vcpi_slots() returns an error when there is not enough
> > available bandwidth on a link to support a mode. This error should make
> > compute_config() to fail. Not returning false could end up in a modeset
> > which will not work.
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index e21cf08..4280a83 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct 
> > intel_encoder *encoder,
> >  
> > pipe_config->pbn = mst_pbn;
> > slots = drm_dp_find_vcpi_slots(_dp->mst_mgr, mst_pbn);
> > +   if (slots < 0) {
> > +   DRM_ERROR("not enough available time slots for pbn=%d", 
> > mst_pbn);
> 
> No DRM_ERROR for cases that are expected to fail, i.e. DRM_DEBUG_KMS is
> the right level.
> 
It'd be nice to document the usage somewhere. There seems to be several
not very obvious reasons to choose one over the other. I can volunteer
to write it but I am not getting it right as it's evident here.


> And I don't think this works correctly either. Assume you do an atomic
> modeset where you enable 2 mst sinks at the same time, and the following
> happens:
> - mst connector 1 can be allocated, and passes
>   intel_dp_mst_compute_config.
> - mst connector 2 can be allocated, but not together with connector 1.
>   But drm_dp_find_vcpi_slots only checks what's available, it doesn't do a
>   temporary reservation.
> 
> And we can just reserve the slot in drm_dp_find_vcpi_slots, because then
> in the above case (when connector 2 doesn't have enough slots anymore)
> we'd leak the vcpi allocation for connector 1.
> 
> Even worse, when we do a TEST_ONLY atomic commit (i.e. only run
> atomic_check/compute_config code, but not commit anything) this can even
> happen for a successful commit.
> 
> Long story short: To fix this properly we need to rewrite the dp helpers
> to be compliant with atomic design. I think for that we roughly need:
> 
> - Exract vcpi allocations into a free-standing state structure. I'd call
>   it drm_dp_mst_state or similar. Provide duplicate(get_state)/release
>   functions like we already have for plane, connector and crtc states in
>   the core, and e.g. dpll configuration in i915/intel_dpll_mgr.c. Ander
>   Conselvan can help you with this. I'm also planning to write better
>   documentation for how to do this exactly (since you also need a ww_mutex
>   to protect this state), and I'll prioritize that work.
> 
> - Wire up that dp mst state at the right places globally (we need one slot
>   per drm_dp_mst_topology_mgr, i.e. per port), and duplicate that state in
>   intel_dp_mst_compute_config. We can't wire this up anywhere in the core
>   since the dp mst library is a helper library, so all the integration
>   points need to be done explicitly in drivers.
> 
> - Do the same for nouveau - nouveau is now also atomic, and it also is
>   atomic with mst support.
> 
> - While doing all that make sure that the existing (non-atomic-compliant)
>   functions in the dp mst helpers keep working, we need those for amggpu.
> 
> - Create a new drm_dp_state_allocate_vcpi_slots, which only touches the
>   new drm_dp_mst_state structure and allocats the vcpi slots there. Also
>   add some function to find the allocation for each sink again (we need
>   that in the modeset commit functions).
> 

Let me rephrase so that I am sure I understand this.
With the way that I am doing this (assuming the mutex bug in Patch 3/3
is fixed), we'll still end up passing compute_config() but fail modeset
in the scenario you pointed out. This is because the slots are not
reserved in drm_dp_find_vcpi_slots().

However, if we do reserve the slots for connector1 in
drm_dp_find_vcpi_slots() and fail compute_config() for connector2, then
the vcpi slots that were assigned to connector1 are not released.

But, by having drm_dp_mst_state, we can simulate vcpi slot allocation in
the atomic_check()/compute_config() phase and fail without committing,
while also releasing the slots. 


-DK


> - Rework our compute_config and modeset code to use this new function, and
>   not the old legacy find/allocate functions.
> 
> To make this happen we need buy-in from Ben Skeggs (nouveau maintainer)
> and preferrably also from the AMD display team (since they support mst
> already, and also plan to eventually support atomic).
> 
> Fixing this correctly is unfortunately a _lot_ more work than your simple
> patch here :(
> -Daniel

[Intel-gfx] [PATCH v2] drm/dp: Make space for null terminator in the DP device ID char array

[Pandiyan, Dhinakaran]

Adding Cc's.


On Mon, 2016-11-07 at 15:22 -0800, Dhinakaran Pandiyan wrote:
> The DP device identification string read from the DPCD registers is 6
> characters long at max. and we store it in a char array of the same length
> without space for the NULL terminator. Fix this by increasing the array
> size to 7 and initialize it to an empty string.
> 
> v2: Use %*pE format specifier (Jani)
> 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e6fe82..2d42760 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -544,7 +544,7 @@ void drm_dp_downstream_debug(struct seq_file *m,
>DP_DETAILED_CAP_INFO_AVAILABLE;
>   int clk;
>   int bpc;
> - char id[6];
> + char id[6] = "";
>   int len;
>   uint8_t rev[2];
>   int type = port_cap[0] & DP_DS_PORT_TYPE_MASK;
> @@ -584,7 +584,8 @@ void drm_dp_downstream_debug(struct seq_file *m,
>   }
>  
>   drm_dp_downstream_id(aux, id);
> - seq_printf(m, "\t\tID: %s\n", id);
> + len = strnlen(id, 6);
> + seq_printf(m, "\t\tID: %*pE\n", len, id);
>  
>   len = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, [0], 1);
>   if (len > 0)

[Intel-gfx] [PATCH] drm/dp: Make space for null terminator in the DP device ID char array

[Pandiyan, Dhinakaran]

Mika,

Can you take a look at this?

-DK

On Fri, 2016-11-04 at 14:06 -0700, Dhinakaran Pandiyan wrote:
> The DP device identification string read from the DPCD registers is 6
> characters long at max. and we store it in a char array of the same length
> without space for the NULL terminator. Fix this by increasing the array
> size to 7 and initialize it to an empty string.
> 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e6fe82..3a39312 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -544,7 +544,7 @@ void drm_dp_downstream_debug(struct seq_file *m,
>DP_DETAILED_CAP_INFO_AVAILABLE;
>   int clk;
>   int bpc;
> - char id[6];
> + char id[7] = "";
>   int len;
>   uint8_t rev[2];
>   int type = port_cap[0] & DP_DS_PORT_TYPE_MASK;

[Intel-gfx] [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry

[Pandiyan, Dhinakaran]

On Mon, 2016-10-24 at 23:28 -0700, Manasi Navare wrote:
> On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote:
> > On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote:
> > > This work struct will be used to schedule a uevent on a separate
> > > thread. This will be scheduled after a link train failure during modeset
> > > to indicate a modeset retry request. It will get executed after the
> > > current modeset is complete and all locks are released. This was
> > > required to avoid deadlock.
> > > 
> > > Cc: dri-devel at lists.freedesktop.org
> > > Cc: Jani Nikula 
> > > Cc: Daniel Vetter 
> > > Cc: Ville Syrjala 
> > > 
> > > Signed-off-by: Manasi Navare 
> > > ---
> > >  include/drm/drm_connector.h | 5 +
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index ac9d7d8..fcf6b97 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -682,6 +682,11 @@ struct drm_connector {
> > >   uint8_t num_h_tile, num_v_tile;
> > >   uint8_t tile_h_loc, tile_v_loc;
> > >   uint16_t tile_h_size, tile_v_size;
> > > +
> > > + /* Work struct to schedule a uevent on link train failure for
> > > +  * DisplayPort.
> > > +  */
> > > + struct work_struct i915_modeset_retry_work;
> > 
> > You cannot put i915 stuff into core structs.
> > -Daniel
> > 
> > >  };
> 
> I will rename it to use modeset_retry_work instead.
> 
> Manasi


Why not move it struct intel_connector? The work function is defined in
intel_dp.c afaict

-DK
> > >  
> > >  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> > > -- 
> > > 1.9.1
> > > 
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH RFC 2/8] drm: Define a work struct for scheduling a uevent for modeset retry

[Pandiyan, Dhinakaran]

On Wed, 2016-10-19 at 14:46 -0700, Manasi Navare wrote:
> This work struct will be used to schedule a uevent on a separate
> thread. This will be scheduled after a link train failure during modeset
> to indicate a modeset retry request. It will get executed after the
> current modeset is complete and all locks are released. This was
> required to avoid deadlock.
> 
> Cc: dri-devel at lists.freedesktop.org
> Cc: Jani Nikula 
> Cc: Daniel Vetter 
> Cc: Ville Syrjala 
> 
> Signed-off-by: Manasi Navare 
> ---
>  include/drm/drm_connector.h | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index d499466..9218a24 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -687,6 +687,11 @@ struct drm_connector {
>* in case of link train failure during current modeset
>*/
>   bool link_train_retry;
> +
> + /* Work struct to schedule a uevent on link train failure for
> +  * DisplayPort.
> +  */
> + struct work_struct i915_modeset_retry_work;

Should this be in struct intel_connector instead ?

>  };
>  
>  #define obj_to_connector(x) container_of(x, struct drm_connector, base)

[2/9] drm/doc: Polish kerneldoc for encoders

[Pandiyan, Dhinakaran]

I guess it's too late for review now. But, I want to send this anyway.

On Mon, 2016-08-29 at 10:27 +0200, Daniel Vetter wrote:
> - Move missing bits into struct drm_encoder docs.
> - Explain that encoders are 95% internal and only 5% uapi, and that in
>   general the uapi part is broken.
> - Remove verbose comments for functions not exposed to drivers.
> 
> v2: Review from Archit:
> - Appease checkpatch in the moved code.
> - Make it clearer that bridges are not exposed to userspace.
> 
> Reviewed-by: Archit Taneja 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/gpu/drm-kms.rst | 46 
>  drivers/gpu/drm/drm_encoder.c | 48 ++---
>  include/drm/drm_encoder.h | 70 
> +++
>  3 files changed, 101 insertions(+), 63 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 7f788caebea3..47c2835b7c2d 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -128,6 +128,12 @@ Connector Functions Reference
>  Encoder Abstraction
>  ===
>  
> +.. kernel-doc:: drivers/gpu/drm/drm_encoder.c
> +   :doc: overview
> +
> +Encoder Functions Reference
> +---
> +
>  .. kernel-doc:: include/drm/drm_encoder.h
> :internal:
>  
> @@ -207,46 +213,6 @@ future); drivers that do not wish to provide special 
> handling for
>  primary planes may make use of the helper functions described in ? to
>  create and register a primary plane with standard capabilities.
>  
> -Encoders (:c:type:`struct drm_encoder `)
> --
> -
> -An encoder takes pixel data from a CRTC and converts it to a format
> -suitable for any attached connectors. On some devices, it may be
> -possible to have a CRTC send data to more than one encoder. In that
> -case, both encoders would receive data from the same scanout buffer,
> -resulting in a "cloned" display configuration across the connectors
> -attached to each encoder.
> -
> -Encoder Initialization
> -~~
> -
> -As for CRTCs, a KMS driver must create, initialize and register at least
> -one :c:type:`struct drm_encoder ` instance. The
> -instance is allocated and zeroed by the driver, possibly as part of a
> -larger structure.
> -
> -Drivers must initialize the :c:type:`struct drm_encoder
> -` possible_crtcs and possible_clones fields before
> -registering the encoder. Both fields are bitmasks of respectively the
> -CRTCs that the encoder can be connected to, and sibling encoders
> -candidate for cloning.
> -
> -After being initialized, the encoder must be registered with a call to
> -:c:func:`drm_encoder_init()`. The function takes a pointer to the
> -encoder functions and an encoder type. Supported types are
> -
> --  DRM_MODE_ENCODER_DAC for VGA and analog on DVI-I/DVI-A
> --  DRM_MODE_ENCODER_TMDS for DVI, HDMI and (embedded) DisplayPort
> --  DRM_MODE_ENCODER_LVDS for display panels
> --  DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video,
> -   Component, SCART)
> --  DRM_MODE_ENCODER_VIRTUAL for virtual machine displays
> -
> -Encoders must be attached to a CRTC to be used. DRM drivers leave
> -encoders unattached at initialization time. Applications (or the fbdev
> -compatibility layer when implemented) are responsible for attaching the
> -encoders they want to use to a CRTC.
> -
>  Cleanup
>  ---
>  
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index bce781b7bb5f..998a6743a586 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -26,6 +26,30 @@
>  
>  #include "drm_crtc_internal.h"
>  
> +/**
> + * DOC: overview
> + *
> + * Encoders represent the connecting element between the CRTC (as the overall
> + * pixel pipeline, represented by struct _crtc) and the connectors (as 
> the
> + * generic sink entity, represented by struct _connector). Encoders are

Doesn't really say what encoders actually do apart being in between crtc
and connector. This line could have been useful here - "An encoder takes
pixel data from a CRTC and converts it to a format suitable for any
attached connectors." 

> + * objects exposed to userspace, originally to allow userspace to infer 
> cloning
> + * and connector/CRTC restrictions. Unfortunately almost all drivers get this
> + * wrong, making the uabi pretty much useless. On top of that the exposed
> + * restrictions are too simple for todays hardware, and the recommend way to
> + * infer restrictions is by using the DRM_MODE_ATOMIC_TEST_ONLY flag for the
> + * atomic IOCTL.
> + *
> + * Otherwise encoders aren't used in the uapi at all (any modeset request 
> from
> + * userspace directly connects a connector with a CRTC), drivers are 
> therefore
> + * free to use them however they wish. Modeset helper libraries make strong 
> use
> + * of encoders to facilitate code sharing. But for more complex settings it 
> is

[Intel-gfx] [PATCH 1/2] A Helper function that returns available link bandwidth

[Pandiyan, Dhinakaran]

On Thu, 2016-08-11 at 16:41 -0700, Anusha Srivatsa wrote:
> drm/dp/mst
> 
> Signed-off-by: Anusha Srivatsa 
> 
> Add a function that returns the available link bandwidth for
> MST port so that we can accurately determine whether a new
> mode is valid for the link or not.
> 

The Signed-off line should follow the explanation body.

> Cc: dri-devel at lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 12 
>  include/drm/drm_dp_mst_helper.h   |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e4571..7a239f6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -43,6 +43,8 @@ static bool dump_dp_payload_table(struct 
> drm_dp_mst_topology_mgr *mgr,
> char *buf);
>  static int test_calc_pbn_mode(void);
>  
> +int drm_dp_mst_get_avail_pbn(struct drm_dp_mst_topology_mgr *mgr, struct 
> drm_dp_mst_port *port);
> +
>  static void drm_dp_put_port(struct drm_dp_mst_port *port);
>  
>  static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
> @@ -2730,6 +2732,16 @@ static int test_calc_pbn_mode(void)
>   return 0;
>  }
>  
> +int drm_dp_mst_get_avail_pbn(struct drm_dp_mst_topology_mgr *mgr, struct 
> drm_dp_mst_port *port)
> +{
> +port = drm_dp_get_validated_port_ref(mgr,port);
> +if (port)
> +return port->available_pbn;
> +
> +return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_dp_mst_get_avail_pbn);
> +
>  /* we want to kick the TX after we've ack the up/down IRQs. */
>  static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
>  {
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 0032076..74dc4ab 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -576,6 +576,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector 
> *connector, struct drm_dp_
>  
>  int drm_dp_calc_pbn_mode(int clock, int bpp);
>  
> +int drm_dp_mst_get_avail_pbn(struct drm_dp_mst_topology_mgr *mgr, struct 
> drm_dp_mst_port *port);
>  
>  bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> drm_dp_mst_port *port, int pbn, int *slots);
>  

[Intel-gfx] [PATCH 1/4] drm/i915/dp: Add debug messages to print DP link training pattern

[Pandiyan, Dhinakaran]

On Thu, 2016-08-04 at 04:07 +0100, Chris Wilson wrote:
> On Wed, Aug 03, 2016 at 08:07:38PM -0700, Dhinakaran Pandiyan wrote:
> > @@ -2588,7 +2592,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
> > *DP |= DP_LINK_TRAIN_PAT_2_CPT;
> > break;
> > case DP_TRAINING_PATTERN_3:
> > -   DRM_ERROR("DP training pattern 3 not supported\n");
> > +   DRM_ERROR("TPS3 not supported, using TPS2 instead\n");
> > *DP |= DP_LINK_TRAIN_PAT_2_CPT;
> > break;
> > }
> > @@ -2613,7 +2617,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
> > if (IS_CHERRYVIEW(dev)) {
> > *DP |= DP_LINK_TRAIN_PAT_3_CHV;
> > } else {
> > -   DRM_ERROR("DP training pattern 3 not 
> > supported\n");
> > +   DRM_ERROR("TPS3 not supported, using TPS2 
> > instead\n");
> > *DP |= DP_LINK_TRAIN_PAT_2;
> 
> Given that you have a fallback plan and if the fallback plan fails you
> alert the user with an error already, these aren't errors but debug.
> -Chris
> 
I will make that change. Thanks for the review.

[Intel-gfx] [PATCH 3/4] drm/dp: Clarify clock recovery and channel equalization failures

[Pandiyan, Dhinakaran]

On Thu, 2016-08-04 at 04:12 +0100, Chris Wilson wrote:
> On Wed, Aug 03, 2016 at 08:07:40PM -0700, Dhinakaran Pandiyan wrote:
> > The causes of clock recovery and channel equalization failures are not
> > explicitly printed in debug messages. Help debugging link training
> > failures by printing why it failed.
> > 
> > Doing this in the driver would mean re-implementing some of the drm static
> > functions that decode link status. Let's avoid that with these debug
> > messages in drm.
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 12 +---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c 
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 091053e..d763b57 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -64,12 +64,16 @@ bool drm_dp_channel_eq_ok(const u8 
> > link_status[DP_LINK_STATUS_SIZE],
> >  
> > lane_align = dp_link_status(link_status,
> > DP_LANE_ALIGN_STATUS_UPDATED);
> > -   if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0)
> > +   if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0) {
> > +   DRM_DEBUG_KMS("Inter-lane alignment not done\n");
> > return false;
> > +   }
> > for (lane = 0; lane < lane_count; lane++) {
> > lane_status = dp_get_lane_status(link_status, lane);
> > -   if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS)
> > +   if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS) {
> > +   DRM_DEBUG_KMS("Channel equalization not done for lane 
> > %d\n", lane);
> > return false;
> > +   }
> > }
> > return true;
> >  }
> > @@ -83,8 +87,10 @@ bool drm_dp_clock_recovery_ok(const u8 
> > link_status[DP_LINK_STATUS_SIZE],
> >  
> > for (lane = 0; lane < lane_count; lane++) {
> > lane_status = dp_get_lane_status(link_status, lane);
> > -   if ((lane_status & DP_LANE_CR_DONE) == 0)
> > +   if ((lane_status & DP_LANE_CR_DONE) == 0) {
> > +   DRM_DEBUG_KMS("Clock recovery not done for lane %d\n", 
> > lane);
> 
> Please petition, with patch, for DRM_DEBUG_DP.
> -Chris
> 
Is that because DRM_DEBUG_KMS in not appropriate or that we have plenty
of DP related debug messages that it warrants it's own debug macro?

[Intel-gfx] [PATCH 4/4] drm/i915/dp: Dump DP link status when link training stages fails

[Pandiyan, Dhinakaran]

On Thu, 2016-08-04 at 10:46 +0300, Jani Nikula wrote:
> On Thu, 04 Aug 2016, Dhinakaran Pandiyan  
> wrote:
> > A full dump of link status can be handy in debugging link training
> > failures. Let's add that to the debug messages when link training fails.
> >
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 11 +++
> >  drivers/gpu/drm/i915/intel_drv.h  |  6 --
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index c0a858d..ab7d1a6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -24,6 +24,15 @@
> >  #include "intel_drv.h"
> >  
> >  static void
> > +intel_dp_dump_link_status(const uint8_t link_status[DP_LINK_STATUS_SIZE])
> > +{
> > +
> > +   DRM_DEBUG_KMS("ln0_1:0x%x ln2_3:0x%x align:0x%x sink:0x%x 
> > adj_req0_1:0x%x adj_req2_3:0x%x",
> > + link_status[0], link_status[1], link_status[2],
> > + link_status[3], link_status[4], link_status[5]);
> > +}
> > +
> > +static void
> >  intel_get_adjust_train(struct intel_dp *intel_dp,
> >const uint8_t link_status[DP_LINK_STATUS_SIZE])
> >  {
> > @@ -170,6 +179,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp 
> > *intel_dp)
> > ++loop_tries;
> > if (loop_tries == 5) {
> > DRM_ERROR("too many full retries, give up\n");
> > +   intel_dp_dump_link_status(link_status);
> > break;
> > }
> > intel_dp_reset_link_train(intel_dp,
> > @@ -257,6 +267,7 @@ intel_dp_link_training_channel_equalization(struct 
> > intel_dp *intel_dp)
> >  
> > if (cr_tries > 5) {
> > DRM_ERROR("failed to train DP, aborting\n");
> > +   intel_dp_dump_link_status(link_status);
> > break;
> > }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 87069ba..549a8fd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1356,8 +1356,6 @@ bool intel_dp_init_connector(struct 
> > intel_digital_port *intel_dig_port,
> >  struct intel_connector *intel_connector);
> >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >   const struct intel_crtc_state *pipe_config);
> > -void intel_dp_start_link_train(struct intel_dp *intel_dp);
> > -void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> >  void intel_dp_encoder_reset(struct drm_encoder *encoder);
> >  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
> > @@ -1409,6 +1407,10 @@ static inline unsigned int 
> > intel_dp_unused_lane_mask(int lane_count)
> > return ~((1 << lane_count) - 1) & 0xf;
> >  }
> >  
> > +/* intel_dp_link_training.c */
> > +void intel_dp_start_link_train(struct intel_dp *intel_dp);
> > +void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> 
> Unrelated cleanup change. Should be a (standalone) separate patch, and
> if it were, it could have been merged already.
> 
> Pro-tip: In general, you'll want to organize your series in a way that
> allows the early patches to be merged even when the review rounds are
> still in progress on the later patches. Crucial fixes first (so they can
> be backported without conflicts), trivial and non-controversial things
> next, and so on. You'll have a feeling of making progress, you'll have
> fewer rebases and conflicts later on, and it'll be easier for the
> reviewers too as the number of patches in later versions shrinks.
> 
> BR,
> Jani.
> 

Thanks for the tip Jani, will keep that in mind. I will split this patch
when I send the next version of the series.
> > +
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector 
> > *intel_connector);
>