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

["Kamble, Sagar A" <sagar.a.kamble@intel.com>]

Thanks for the review Ville.
I will update the patch.

On 1/22/2016 10:44 PM, Ville Syrjälä wrote:
> On Wed, Jan 20, 2016 at 06:26:16PM -0800, tom.orourke@intel.com wrote:
>> 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>
>> ---
>>   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) {
>> +		ret = I915_READ(SOFT_SCRATCH(1));
>> +		ret &= 0xFF;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   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) {
>> +			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) {
> The connecotr/encoder stuff should not be here. We can enable a pipe
> without any connected connectors. Also locking is fail.
>
> The patch also fails to explain why the guc needs to know any of this
> stuff. It makes me very suspicious that the guc is going to start
> poking at some display stuff behind the driver's back. I actually
> think that doing some display stuff in the guc might be neat, but not
> when it's a black box blob.
>
>> +						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
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx