| Message ID | 20230102062005.720964-2-chaitanya.kumar.borah@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new CDCLK step for RPL-U | expand |
On Mon, 02 Jan 2023, Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> wrote: > A new CDCLK step of 480MHz has been added on SKUs that has a > RPL-U device id. This is done to support 120Hz displays with > more efficiency. > > RPL-U device ids are currently added within the RPL-P sub > platform. It seems to be an overkill to add a separate sub > platform just to support this change. Therefore, quirks > are a good way to achieve the same. The thing is, this part is *not* a quirk. It's basic enabling for RPL-U. If you start conflating quirks and basic enabling to avoid overkill, you're eventually going to end up in all kinds of trouble with maintenance. BR, Jani. > > BSpec: 55409 > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > --- > drivers/gpu/drm/i915/display/intel_quirks.c | 14 ++++++++++++++ > drivers/gpu/drm/i915/display/intel_quirks.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c > index 6e48d3bcdfec..0a30499835b3 100644 > --- a/drivers/gpu/drm/i915/display/intel_quirks.c > +++ b/drivers/gpu/drm/i915/display/intel_quirks.c > @@ -65,6 +65,16 @@ static void quirk_no_pps_backlight_power_hook(struct drm_i915_private *i915) > drm_info(&i915->drm, "Applying no pps backlight power quirk\n"); > } > > +/* > + * A new step of 480MHz has been added on SKUs that have a RPL-U device id. > + * This particular step is to better support 120Hz panels. > + */ > +static void quirk_480mhz_cdclk_step_hook(struct drm_i915_private *i915) > +{ > + intel_set_quirk(i915, QUIRK_480MHZ_CDCLK_STEP); > + drm_info(&i915->drm, "Applying 480MHz CDCLK step quirk\n"); > +} > + > struct intel_quirk { > int device; > int subsystem_vendor; > @@ -199,6 +209,10 @@ static struct intel_quirk intel_quirks[] = { > /* ECS Liva Q2 */ > { 0x3185, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, > { 0x3184, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, > + /* RPL-U */ > + { 0xA7A1, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, > + { 0xA721, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, > + { 0xA7A9, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, > }; > > void intel_init_quirks(struct drm_i915_private *i915) > diff --git a/drivers/gpu/drm/i915/display/intel_quirks.h b/drivers/gpu/drm/i915/display/intel_quirks.h > index 10a4d163149f..71e05684f5f4 100644 > --- a/drivers/gpu/drm/i915/display/intel_quirks.h > +++ b/drivers/gpu/drm/i915/display/intel_quirks.h > @@ -17,6 +17,7 @@ enum intel_quirk_id { > QUIRK_INVERT_BRIGHTNESS, > QUIRK_LVDS_SSC_DISABLE, > QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK, > + QUIRK_480MHZ_CDCLK_STEP, > }; > > void intel_init_quirks(struct drm_i915_private *i915);
On Mon, 02 Jan 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 02 Jan 2023, Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> wrote: >> A new CDCLK step of 480MHz has been added on SKUs that has a >> RPL-U device id. This is done to support 120Hz displays with >> more efficiency. >> >> RPL-U device ids are currently added within the RPL-P sub >> platform. It seems to be an overkill to add a separate sub >> platform just to support this change. Therefore, quirks >> are a good way to achieve the same. > > The thing is, this part is *not* a quirk. It's basic enabling for RPL-U. > > If you start conflating quirks and basic enabling to avoid overkill, > you're eventually going to end up in all kinds of trouble with > maintenance. Please start off with adding RPL-U as a subplatform, and let's worry about the CDCLK after that. BR, Jani. > > > BR, > Jani. > >> >> BSpec: 55409 >> >> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_quirks.c | 14 ++++++++++++++ >> drivers/gpu/drm/i915/display/intel_quirks.h | 1 + >> 2 files changed, 15 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c >> index 6e48d3bcdfec..0a30499835b3 100644 >> --- a/drivers/gpu/drm/i915/display/intel_quirks.c >> +++ b/drivers/gpu/drm/i915/display/intel_quirks.c >> @@ -65,6 +65,16 @@ static void quirk_no_pps_backlight_power_hook(struct drm_i915_private *i915) >> drm_info(&i915->drm, "Applying no pps backlight power quirk\n"); >> } >> >> +/* >> + * A new step of 480MHz has been added on SKUs that have a RPL-U device id. >> + * This particular step is to better support 120Hz panels. >> + */ >> +static void quirk_480mhz_cdclk_step_hook(struct drm_i915_private *i915) >> +{ >> + intel_set_quirk(i915, QUIRK_480MHZ_CDCLK_STEP); >> + drm_info(&i915->drm, "Applying 480MHz CDCLK step quirk\n"); >> +} >> + >> struct intel_quirk { >> int device; >> int subsystem_vendor; >> @@ -199,6 +209,10 @@ static struct intel_quirk intel_quirks[] = { >> /* ECS Liva Q2 */ >> { 0x3185, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, >> { 0x3184, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, >> + /* RPL-U */ >> + { 0xA7A1, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, >> + { 0xA721, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, >> + { 0xA7A9, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, >> }; >> >> void intel_init_quirks(struct drm_i915_private *i915) >> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.h b/drivers/gpu/drm/i915/display/intel_quirks.h >> index 10a4d163149f..71e05684f5f4 100644 >> --- a/drivers/gpu/drm/i915/display/intel_quirks.h >> +++ b/drivers/gpu/drm/i915/display/intel_quirks.h >> @@ -17,6 +17,7 @@ enum intel_quirk_id { >> QUIRK_INVERT_BRIGHTNESS, >> QUIRK_LVDS_SSC_DISABLE, >> QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK, >> + QUIRK_480MHZ_CDCLK_STEP, >> }; >> >> void intel_init_quirks(struct drm_i915_private *i915);
Hello Jani, > -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Monday, January 2, 2023 3:34 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Kahola, Mika > <mika.kahola@intel.com>; Shankar, Uma <uma.shankar@intel.com>; > Syrjala, Ville <ville.syrjala@intel.com>; Srivatsa, Anusha > <anusha.srivatsa@intel.com>; Borah, Chaitanya Kumar > <chaitanya.kumar.borah@intel.com> > Subject: Re: [RFC 1/4] drm/i915/quirks: Add quirk for 480MHz CDCLK step > > On Mon, 02 Jan 2023, Chaitanya Kumar Borah > <chaitanya.kumar.borah@intel.com> wrote: > > A new CDCLK step of 480MHz has been added on SKUs that has a RPL-U > > device id. This is done to support 120Hz displays with more > > efficiency. > > > > RPL-U device ids are currently added within the RPL-P sub platform. It > > seems to be an overkill to add a separate sub platform just to support > > this change. Therefore, quirks are a good way to achieve the same. > > The thing is, this part is *not* a quirk. It's basic enabling for RPL-U. > > If you start conflating quirks and basic enabling to avoid overkill, you're > eventually going to end up in all kinds of trouble with maintenance. > I have floated the latest version of the patch series with addition of a sub-platform for RPL-U. This version of the patch series does not make the ES/QS differentiation. However, I have some doubts if RPL-U can be considered as a separate sub-platform as we interpret it now. For example, it does not show up as a different row in the "Stepping info" in Bspec. Also I had to duplicate the revision ids for it from RPL-P, for which there is no precedence yet. Nevertheless, it could be a better way to do it than quirks. + } else if (IS_ADLP_RPLU(i915)) { + revids = adlp_rplp_revids; + size = ARRAY_SIZE(adlp_rplp_revids); Regards Chaitanya > > BR, > Jani. > > > > > BSpec: 55409 > > > > Signed-off-by: Chaitanya Kumar Borah > <chaitanya.kumar.borah@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_quirks.c | 14 ++++++++++++++ > > drivers/gpu/drm/i915/display/intel_quirks.h | 1 + > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c > > b/drivers/gpu/drm/i915/display/intel_quirks.c > > index 6e48d3bcdfec..0a30499835b3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_quirks.c > > +++ b/drivers/gpu/drm/i915/display/intel_quirks.c > > @@ -65,6 +65,16 @@ static void > quirk_no_pps_backlight_power_hook(struct drm_i915_private *i915) > > drm_info(&i915->drm, "Applying no pps backlight power quirk\n"); } > > > > +/* > > + * A new step of 480MHz has been added on SKUs that have a RPL-U > device id. > > + * This particular step is to better support 120Hz panels. > > + */ > > +static void quirk_480mhz_cdclk_step_hook(struct drm_i915_private > > +*i915) { > > + intel_set_quirk(i915, QUIRK_480MHZ_CDCLK_STEP); > > + drm_info(&i915->drm, "Applying 480MHz CDCLK step quirk\n"); } > > + > > struct intel_quirk { > > int device; > > int subsystem_vendor; > > @@ -199,6 +209,10 @@ static struct intel_quirk intel_quirks[] = { > > /* ECS Liva Q2 */ > > { 0x3185, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, > > { 0x3184, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, > > + /* RPL-U */ > > + { 0xA7A1, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook > }, > > + { 0xA721, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook > }, > > + { 0xA7A9, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook > }, > > }; > > > > void intel_init_quirks(struct drm_i915_private *i915) diff --git > > a/drivers/gpu/drm/i915/display/intel_quirks.h > > b/drivers/gpu/drm/i915/display/intel_quirks.h > > index 10a4d163149f..71e05684f5f4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_quirks.h > > +++ b/drivers/gpu/drm/i915/display/intel_quirks.h > > @@ -17,6 +17,7 @@ enum intel_quirk_id { > > QUIRK_INVERT_BRIGHTNESS, > > QUIRK_LVDS_SSC_DISABLE, > > QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK, > > + QUIRK_480MHZ_CDCLK_STEP, > > }; > > > > void intel_init_quirks(struct drm_i915_private *i915); > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c index 6e48d3bcdfec..0a30499835b3 100644 --- a/drivers/gpu/drm/i915/display/intel_quirks.c +++ b/drivers/gpu/drm/i915/display/intel_quirks.c @@ -65,6 +65,16 @@ static void quirk_no_pps_backlight_power_hook(struct drm_i915_private *i915) drm_info(&i915->drm, "Applying no pps backlight power quirk\n"); } +/* + * A new step of 480MHz has been added on SKUs that have a RPL-U device id. + * This particular step is to better support 120Hz panels. + */ +static void quirk_480mhz_cdclk_step_hook(struct drm_i915_private *i915) +{ + intel_set_quirk(i915, QUIRK_480MHZ_CDCLK_STEP); + drm_info(&i915->drm, "Applying 480MHz CDCLK step quirk\n"); +} + struct intel_quirk { int device; int subsystem_vendor; @@ -199,6 +209,10 @@ static struct intel_quirk intel_quirks[] = { /* ECS Liva Q2 */ { 0x3185, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, { 0x3184, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time }, + /* RPL-U */ + { 0xA7A1, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, + { 0xA721, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, + { 0xA7A9, PCI_ANY_ID, PCI_ANY_ID, quirk_480mhz_cdclk_step_hook }, }; void intel_init_quirks(struct drm_i915_private *i915) diff --git a/drivers/gpu/drm/i915/display/intel_quirks.h b/drivers/gpu/drm/i915/display/intel_quirks.h index 10a4d163149f..71e05684f5f4 100644 --- a/drivers/gpu/drm/i915/display/intel_quirks.h +++ b/drivers/gpu/drm/i915/display/intel_quirks.h @@ -17,6 +17,7 @@ enum intel_quirk_id { QUIRK_INVERT_BRIGHTNESS, QUIRK_LVDS_SSC_DISABLE, QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK, + QUIRK_480MHZ_CDCLK_STEP, }; void intel_init_quirks(struct drm_i915_private *i915);
A new CDCLK step of 480MHz has been added on SKUs that has a RPL-U device id. This is done to support 120Hz displays with more efficiency. RPL-U device ids are currently added within the RPL-P sub platform. It seems to be an overkill to add a separate sub platform just to support this change. Therefore, quirks are a good way to achieve the same. BSpec: 55409 Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> --- drivers/gpu/drm/i915/display/intel_quirks.c | 14 ++++++++++++++ drivers/gpu/drm/i915/display/intel_quirks.h | 1 + 2 files changed, 15 insertions(+)