Re: [RFC 14/22] drm/i915/slpc: Notification of Display mode change

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qua, 2016-01-20 às 18:26 -0800, tom.orourke@intel.com escreveu:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> GuC SLPC need to be sent data related to Active pipes, refresh rates,
> widi pipes, fullscreen pipes related via host to GuC display mode
> change event.
> This patch defines the event and implements trigger of the event.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Acked-by: Tom O'Rourke <Tom.O'Rourke@intel.com>

(this is just a comment from a quick look at the file, not a full
review)

> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 +
>  drivers/gpu/drm/i915/intel_slpc.c    | 92
> ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_slpc.h    |  1 +
>  3 files changed, 95 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 06ab6df..7c3d902 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13638,6 +13638,8 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	 */
>  	intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
>  
> +	intel_slpc_update_display_mode_info(dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_slpc.c
> b/drivers/gpu/drm/i915/intel_slpc.c
> index f155d88..f5f7cad 100644
> --- a/drivers/gpu/drm/i915/intel_slpc.c
> +++ b/drivers/gpu/drm/i915/intel_slpc.c
> @@ -74,6 +74,27 @@ static int host2guc_slpc_shutdown(struct
> drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> +static int host2guc_slpc_display_mode_change(struct drm_i915_private
> *dev_priv)
> +{
> +        u32 data[7];
> +	int ret, i;
> +
> +        data[0] = HOST2GUC_ACTION_SLPC_REQUEST;
> +        data[1] = SLPC_EVENT(SLPC_EVENT_DISPLAY_MODE_CHANGE,
> MAX_NUM_OF_PIPE + 1);
> +	data[2] = dev_priv-
> >guc.slpc.display_mode_params.global_data;
> +	for(i = 0; i < MAX_NUM_OF_PIPE; ++i)
> +		data[3+i] = dev_priv-
> >guc.slpc.display_mode_params.per_pipe_info[i].data;
> +
> +        ret = host2guc_action(&dev_priv->guc, data, 7);
> +
> +	if (0 == ret) {

https://en.wikipedia.org/wiki/Yoda_conditions are not part of our usual
coding style.

> +		ret = I915_READ(SOFT_SCRATCH(1));
> +		ret &= 0xFF;
> +	}
> +
> +	return ret;
> +}

There are some tab/space issues in the function above.

Just a suggestion, not a request: for both functions you introduced,
since the only callers do not bother to check the return values, you
could just make the functions return void. Having unchecked return
values may give a false sense that errors would be caught by the upper
layer, when in fact they aren't. In a lot of cases it's better to just
print some error message instead of returning some value that is going
to be ignored.

> +
>  static u8 slpc_get_platform_sku(struct drm_i915_gem_object *obj)
>  {
>  	struct drm_device *dev = obj->base.dev;
> @@ -225,3 +246,74 @@ int intel_slpc_reset(struct drm_device *dev)
>  
>  	return ret;
>  }
> +
> +int intel_slpc_update_display_mode_info(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_connector *connector;
> +	struct intel_connector *intel_connector;
> +	struct drm_encoder *encoder;
> +	struct intel_encoder *intel_encoder;
> +	struct intel_display_pipe_basic_info *per_pipe_info;
> +	struct intel_slpc_display_mode_event_params *cur_params,
> old_params;
> +	bool notify = false;
> +
> +	if (!HAS_SLPC(dev))
> +		return -EINVAL;
> +
> +	cur_params = &dev_priv->guc.slpc.display_mode_params;
> +
> +	/* Copy display mode parameters for comparison */
> +	old_params.global_data  = cur_params->global_data;
> +	cur_params->global_data = 0;
> +
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		per_pipe_info = &cur_params-
> >per_pipe_info[intel_crtc->pipe];
> +		old_params.per_pipe_info[intel_crtc->pipe].data =
> per_pipe_info->data;
> +		per_pipe_info->data = 0;
> +	}
> +
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		struct intel_crtc_state *pipe_config;
> +
> +		per_pipe_info = &cur_params-
> >per_pipe_info[intel_crtc->pipe];
> +		crtc = &intel_crtc->base;
> +		pipe_config = to_intel_crtc_state(crtc->state);
> +
> +		if (pipe_config->base.active) {

As far as I understand from the new atomic locking model, since this
code is called from intel_atomic_commit, it can't just iterate through
every crtc/encoder/connector checking state structures. During an
atomic commit we can only look at the objects affected by the commit,
so we have to use things such as for_each_crtc_in_state().

It seems to me that the model here could be:
- At driver load, during or after HW state readout, initialize some
state structure to make it match the hardware.
- Whenever there's an atomic commit, just look+update the status of the
affected crtc/encoder/connector, leaving everything else the same

> +			for_each_encoder_on_crtc(dev, crtc,
> intel_encoder) {
> +				encoder = &intel_encoder->base;
> +				for_each_connector_on_encoder(dev,
> encoder, intel_connector) {
> +					connector =
> &intel_connector->base;
> +					if (connector->status ==
> connector_status_connected) {
> +						struct
> drm_display_mode *mode = &crtc->mode;
> +						/* FIXME: Update
> is_widi based on encoder */
> +						per_pipe_info-
> >is_widi = 0;
> +						per_pipe_info-
> >refresh_rate = mode->vrefresh;
> +						per_pipe_info-
> >vsync_ft_usec = 1000000 / mode->vrefresh;
> +						cur_params-
> >active_pipes_bitmask |= (1 << intel_crtc->pipe);
> +						cur_params-
> >vbi_sync_on_pipes |= (1 << intel_crtc->pipe);
> +						cur_params-
> >num_active_pipes++;
> +					}
> +				}
> +			}
> +		}
> +	}
> +
> +	/* Compare old display mode with current mode. Notify SLPC
> if it is changed. */
> +	if (cur_params->global_data != old_params.global_data)
> +		notify = true;
> +
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		per_pipe_info = &cur_params-
> >per_pipe_info[intel_crtc->pipe];
> +		if (old_params.per_pipe_info[intel_crtc->pipe].data
> != per_pipe_info->data)
> +			notify = true;
> +	}
> +
> +	if (notify)
> +		host2guc_slpc_display_mode_change(dev_priv);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_slpc.h
> b/drivers/gpu/drm/i915/intel_slpc.h
> index 9673a63..18e551b 100644
> --- a/drivers/gpu/drm/i915/intel_slpc.h
> +++ b/drivers/gpu/drm/i915/intel_slpc.h
> @@ -152,5 +152,6 @@ int intel_slpc_suspend(struct drm_device *dev);
>  int intel_slpc_disable(struct drm_device *dev);
>  int intel_slpc_enable(struct drm_device *dev);
>  int intel_slpc_reset(struct drm_device *dev);
> +int intel_slpc_update_display_mode_info(struct drm_device *dev);
>  
>  #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx