| Message ID | 1453343184-160456-15-git-send-email-tom.orourke@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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@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
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
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) { + 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