Re: [PATCH 7/10] drm/i915: Enable eDP DRRS for CHV

[Ramalingam C <ramalingam.c@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8113 bytes --]


On Wednesday 21 January 2015 08:33 PM, Rodrigo Vivi wrote:
> On Wed, 2015-01-21 at 17:43 +0530, Ramalingam C wrote:
>> Hi
>>
>> On Friday 16 January 2015 04:41 AM, Rodrigo Vivi wrote:
>>
>>> On Fri, Jan 9, 2015 at 12:56 PM, Vandana Kannan
>>> <vandana.kannan@intel.com> wrote:
>>>> From: Durgadoss R <durgadoss.r@intel.com>
>>>>
>>>> This patch enables eDP DRRS for CHV by adding the
>>>> required IS_CHERRYVIEW() checks.
>>>> CHV uses the same register bit as VLV.
>>>>
>>>> [Vandana]: Since CHV has 2 sets of M_N registers, it will follow the same code
>>>> path as gen < 8. Added CHV check in dp_set_m_n()
>>>>
>>>> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_display.c | 4 ++--
>>>>   drivers/gpu/drm/i915/intel_dp.c      | 2 +-
>>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 25596ca..bb44fb9 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -5810,8 +5810,8 @@ void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
>>>>                   * for gen < 8) and if DRRS is supported (to make sure the
>>>>                   * registers are not unnecessarily accessed).
>>>>                   */
>>>> -               if (m2_n2 && INTEL_INFO(dev)->gen < 8 &&
>>>> -                       crtc->config.has_drrs) {
>>>> +               if (m2_n2 && (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen < 8)
>>>> +                       && crtc->config.has_drrs) {
>>> This change here doesn't seem safe. As I told on previous comment I'd
>>> prefer changing intel_dp_set_m_n instead of re-using this intel_cpu
>>> one...
>> Though I am rewriting the intel_dp_set_m_n() that will reuse
>> intel_cpu_transcoder_set_m_n() within.
>> But as a result, i am going to avoid the parallel usage of
>> intel_cpu_transcoder_set_m_n() and intel_dp_set_m_n().
>> So I am afraid this check for inclusion of cherryview for m2_n2
>> programming will be part of the newer code also.
>>
>> Appending the RFC for the newer intel_dp_set_m_n() implementation
>> below. Please review.
>>
>> RFC starts here:
>>
>>   drivers/gpu/drm/i915/intel_display.c |   19
>> ++++++++++++++++---
>>   drivers/gpu/drm/i915/intel_dp.c      |    6
>> ++----
>>   drivers/gpu/drm/i915/intel_drv.h     |    8
>> +++++++-
>>   3 files changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 061a253..59cc87f
>> 100644
>> ---
>> a/drivers/gpu/drm/i915/intel_display.c
>> +++
>> b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5829,13 +5829,26 @@ void intel_cpu_transcoder_set_m_n(struct
>> intel_crtc *crtc,
>>          }
>>   }
>>                                                                                  
>> -void intel_dp_set_m_n(struct intel_crtc
>> *crtc)
>> +void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set set
>> = M1_N1)
>>   {
>> +       struct intel_link_m_n *dp_m_n,
>> *dp_m2_n2;
>> +
>> +       if (set == M1_N1)
>> {
>> +               dp_m_n =
>> &crtc->config.dp_m_n;
>> +               dp_m2_n2 =
>> &crtc->config.dp_m2_n2;
>> +       } else if (set == M2_N2)
>> {
>> +               /* Only one register programming is supported
>> */
>> +               dp_m_n =
>> &crtc->config.dp_m_n;
>> +               dp_m2_n2 =
>> NULL;
>> +       } else
>> {
>> +               DRM_ERROR("Unsupported divider value
>> \n");
>> +
>> return;
>> +       }
>> +
>>          if
>> (crtc->config.has_pch_encoder)
>>                  intel_pch_transcoder_set_m_n(crtc,
>> &crtc->config.dp_m_n);
>>
>> else
>> -               intel_cpu_transcoder_set_m_n(crtc,
>> &crtc->config.dp_m_n,
>> -
>> &crtc->config.dp_m2_n2);
>> +               intel_cpu_transcoder_set_m_n(crtc, dp_m_n,
>> dp_m2_n2);
>>   }
>>                                                                                  
>>   static void vlv_update_pll(struct intel_crtc
>> *crtc,
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index b315292..784b8dd
>> 100644
>> ---
>> a/drivers/gpu/drm/i915/intel_dp.c
>> +++
>> b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4817,11 +4817,10 @@ static void intel_dp_set_drrs_state(struct
>> drm_device *dev, int refresh_rate)
>>          if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev))
>> {
>>                  switch(index)
>> {
>>                  case
>> DRRS_HIGH_RR:
>> -
>> intel_dp_set_m_n(intel_crtc);
>> +                       intel_dp_set_m_n(intel_crtc,
>> M1_N1);
>>
>> break;
>>                  case
>> DRRS_LOW_RR:
>> -
>> intel_cpu_transcoder_set_m_n(intel_crtc,
>> -                                       &intel_crtc->config.dp_m2_n2,
>> NULL);
>> +                       intel_dp_set_m_n(intel_crtc,
>> M2_N2);
>>
>> break;
>>                  case
>> DRRS_MAX_RR:
>>
>> default:
>> @@ -4835,7 +4834,6 @@ static void intel_dp_set_drrs_state(struct
>> drm_device *dev, int refresh_rate)
>>                                  val |=
>> PIPECONF_EDP_RR_MODE_SWITCH_VLV;
>>
>> else
>>                                  val |=
>> PIPECONF_EDP_RR_MODE_SWITCH;
>> -
>> intel_dp_set_m_n(intel_crtc);
>>                  } else
>> {
>>                          if
>> (IS_VALLEYVIEW(dev))
>>                                  val &=
>> ~PIPECONF_EDP_RR_MODE_SWITCH_VLV;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 86d31f2..910e613
>> 100644
>> ---
>> a/drivers/gpu/drm/i915/intel_drv.h
>> +++
>> b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -595,6 +595,12 @@ struct intel_hdmi
>> {
>>   struct
>> intel_dp_mst_encoder;
>>   #define DP_MAX_DOWNSTREAM_PORTS
>> 0x10
>>                                                                                  
>> +enum link_m_n_set
>> {
>> +       M1_N1 =
>> 0,
>> +
>> M2_N2,
>> +
>> DIVIDER_MAX
> do we need this divider_max here?
We dont need this. Removed it in the original patch.
>
>> +};
>> +
>>   struct intel_dp
>> {
>>          uint32_t
>> output_reg;
>>          uint32_t
>> aux_ch_ctl_reg;
>> @@ -983,7 +989,7 @@ void hsw_enable_pc8(struct drm_i915_private
>> *dev_priv);
>>   void hsw_disable_pc8(struct drm_i915_private
>> *dev_priv);
>>   void intel_dp_get_m_n(struct intel_crtc
>> *crtc,
>>                        struct intel_crtc_config
>> *pipe_config);
>> -void intel_dp_set_m_n(struct intel_crtc
>> *crtc);
>> +void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set set
>> = M1_N1);
>>   void intel_cpu_transcoder_set_m_n(struct intel_crtc
>> *crtc,
>>                                   struct intel_link_m_n
>> *m_n,
>>                                   struct intel_link_m_n
>> *m2_n2);
>> --
>> 1.7.9.5
>>>>                          I915_WRITE(PIPE_DATA_M2(transcoder),
>>>>                                          TU_SIZE(m2_n2->tu) | m2_n2->gmch_m);
>>>>                          I915_WRITE(PIPE_DATA_N2(transcoder), m2_n2->gmch_n);
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 3362d93..42195fe 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4802,7 +4802,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>>>>                  return;
>>>>          }
>>>>
>>>> -       if (INTEL_INFO(dev)->gen >= 8) {
>>>> +       if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev)) {
>>>>                  switch(index) {
>>>>                  case DRRS_HIGH_RR:
>>>>                          intel_dp_set_m_n(intel_crtc);
>>>> --
>>>> 2.0.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>> --Ram
>
> But overall it is better and much more clear than previous version.
I have submitted this change for the intel_dp_set_m_n() as V2 of patch 
set 5/10. So this patch [7/10] doesn't have any change.
> Thanks
>
--Ram

[-- Attachment #1.2: Type: text/html, Size: 12545 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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