[Intel-gfx] [RFC 14/22] drm/i915/slpc: Notification of Display mode change
Kamble, Sagar A
sagar.a.kamble at intel.com
Thu Jan 28 01:43:27 PST 2016
Thanks for the review Paulo.
Will incorporate the suggestions.
Thanks
Sagar
On 1/21/2016 6:54 PM, Zanoni, Paulo R wrote:
> Em Qua, 2016-01-20 às 18:26 -0800, tom.orourke at intel.com escreveu:
>> From: Sagar Arun Kamble <sagar.a.kamble at 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 at intel.com>
>> Acked-by: Tom O'Rourke <Tom.O'Rourke at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list