| Message ID | 1427132524-27829-3-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
since drrs struct is used primarily in intel_dp.c file alone can it be moved to intel_dp ? On 3/23/2015 11:12 PM, Ramalingam C wrote: > In case of multiple eDP panels, only one can have > the DRRS enabled on it. > > In future eDP DRRS will be extended for multiple panels. > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 0b26df9..ec40d19 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5311,6 +5311,11 @@ intel_dp_drrs_init(struct intel_connector *intel_connector, > return NULL; > } > > + if (dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) { > + DRM_DEBUG_KMS("DRRS already enabled on previous connector\n"); > + return NULL; > + } > + > INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_downclock_work); > > mutex_init(&dev_priv->drrs.mutex);
Hi Siva, Since the same structure will be used to implement DRRS on DSI also we have placed it at i915_dev_priv. I am working on the RFC for the DRRS on DSI. On Tuesday 24 March 2015 07:33 AM, Sivakumar Thulasimani wrote: > since drrs struct is used primarily in intel_dp.c file alone can it be > moved to intel_dp ? > > On 3/23/2015 11:12 PM, Ramalingam C wrote: >> In case of multiple eDP panels, only one can have >> the DRRS enabled on it. >> >> In future eDP DRRS will be extended for multiple panels. >> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c >> index 0b26df9..ec40d19 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -5311,6 +5311,11 @@ intel_dp_drrs_init(struct intel_connector >> *intel_connector, >> return NULL; >> } >> + if (dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) { >> + DRM_DEBUG_KMS("DRRS already enabled on previous connector\n"); >> + return NULL; >> + } >> + >> INIT_DELAYED_WORK(&dev_priv->drrs.work, >> intel_edp_drrs_downclock_work); >> mutex_init(&dev_priv->drrs.mutex); >
On Tue, Mar 24, 2015 at 11:15:00AM +0530, Ramalingam C wrote: > Hi Siva, > > Since the same structure will be used to implement DRRS on DSI also we have > placed it at i915_dev_priv. > I am working on the RFC for the DRRS on DSI. I think a better approach would be to make an array of dev_priv->drrs->dp pointers, so that we can support one drrs instance per pipe. Well maybe we need to change it from dp to intel_encoder so that it also works for dsi ports. Aside since I just again looked at the code: The locking for drrs is still a bit broken: - A few places check for ->drrs.dp != NULL without holding drrs.mutex. - Imo we should add a WARN_ON(!mutex_locked(drrs.mutex)) to drrs_set_state function. Ramalingam can you please take care of that? Thanks, Daniel > > On Tuesday 24 March 2015 07:33 AM, Sivakumar Thulasimani wrote: > >since drrs struct is used primarily in intel_dp.c file alone can it be > >moved to intel_dp ? > > > >On 3/23/2015 11:12 PM, Ramalingam C wrote: > >>In case of multiple eDP panels, only one can have > >>the DRRS enabled on it. > >> > >>In future eDP DRRS will be extended for multiple panels. > >> > >>Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > >>--- > >> drivers/gpu/drm/i915/intel_dp.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_dp.c > >>b/drivers/gpu/drm/i915/intel_dp.c > >>index 0b26df9..ec40d19 100644 > >>--- a/drivers/gpu/drm/i915/intel_dp.c > >>+++ b/drivers/gpu/drm/i915/intel_dp.c > >>@@ -5311,6 +5311,11 @@ intel_dp_drrs_init(struct intel_connector > >>*intel_connector, > >> return NULL; > >> } > >> + if (dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) { > >>+ DRM_DEBUG_KMS("DRRS already enabled on previous connector\n"); > >>+ return NULL; > >>+ } > >>+ > >> INIT_DELAYED_WORK(&dev_priv->drrs.work, > >>intel_edp_drrs_downclock_work); > >> mutex_init(&dev_priv->drrs.mutex); > > > > -- > Ram >
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6031
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -2 275/275 273/275
ILK -1 303/303 302/303
SNB 304/304 304/304
IVB 339/339 339/339
BYT 287/287 287/287
HSW 361/361 361/361
BDW 310/310 310/310
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt@gem_userptr_blits@minor-sync-interruptible PASS(3) DMESG_WARN(1)PASS(1)
*PNV igt@gem_userptr_blits@minor-unsync-normal PASS(2) DMESG_WARN(2)
*ILK igt@gem_unfence_active_buffers PASS(2) DMESG_WARN(2)
Note: You need to pay more attention to line start with '*'
On Tuesday 24 March 2015 03:03 PM, Daniel Vetter wrote: > On Tue, Mar 24, 2015 at 11:15:00AM +0530, Ramalingam C wrote: >> Hi Siva, >> >> Since the same structure will be used to implement DRRS on DSI also we have >> placed it at i915_dev_priv. >> I am working on the RFC for the DRRS on DSI. > I think a better approach would be to make an array of dev_priv->drrs->dp > pointers, so that we can support one drrs instance per pipe. Well maybe we > need to change it from dp to intel_encoder so that it also works for dsi > ports. > > Aside since I just again looked at the code: The locking for drrs is still > a bit broken: > - A few places check for ->drrs.dp != NULL without holding drrs.mutex. > - Imo we should add a WARN_ON(!mutex_locked(drrs.mutex)) to drrs_set_state > function. > > Ramalingam can you please take care of that? Sure I will look into it Daniel. > > Thanks, Daniel > >> On Tuesday 24 March 2015 07:33 AM, Sivakumar Thulasimani wrote: >>> since drrs struct is used primarily in intel_dp.c file alone can it be >>> moved to intel_dp ? >>> >>> On 3/23/2015 11:12 PM, Ramalingam C wrote: >>>> In case of multiple eDP panels, only one can have >>>> the DRRS enabled on it. >>>> >>>> In future eDP DRRS will be extended for multiple panels. >>>> >>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_dp.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>>> b/drivers/gpu/drm/i915/intel_dp.c >>>> index 0b26df9..ec40d19 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -5311,6 +5311,11 @@ intel_dp_drrs_init(struct intel_connector >>>> *intel_connector, >>>> return NULL; >>>> } >>>> + if (dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) { >>>> + DRM_DEBUG_KMS("DRRS already enabled on previous connector\n"); >>>> + return NULL; >>>> + } >>>> + >>>> INIT_DELAYED_WORK(&dev_priv->drrs.work, >>>> intel_edp_drrs_downclock_work); >>>> mutex_init(&dev_priv->drrs.mutex); >> -- >> Ram >>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 0b26df9..ec40d19 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5311,6 +5311,11 @@ intel_dp_drrs_init(struct intel_connector *intel_connector, return NULL; } + if (dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) { + DRM_DEBUG_KMS("DRRS already enabled on previous connector\n"); + return NULL; + } + INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_downclock_work); mutex_init(&dev_priv->drrs.mutex);
In case of multiple eDP panels, only one can have the DRRS enabled on it. In future eDP DRRS will be extended for multiple panels. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 5 +++++ 1 file changed, 5 insertions(+)