[Intel-gfx] [PATCH v3 2/3] drm/i915: Add i915_lpsp_info debugfs
Manna, Animesh
animesh.manna at intel.com
Thu Apr 2 15:29:21 UTC 2020
On 02-04-2020 18:15, Anshuman Gupta wrote:
> On 2020-04-01 at 21:23:28 +0530, Manna, Animesh wrote:
> thanks animesh for review!
>> On 31-03-2020 17:07, Anshuman Gupta wrote:
>>> New i915_pm_lpsp igt solution approach relies on connector specific
>>> debugfs attribute i915_lpsp_info, it exposes whether an output is
>>> capable of driving lpsp and exposes lpsp enablement info.
>>>
>>> v2:
>>> - CI fixup.
>>> v3:
>>> - register i915_lpsp_info only for supported connector. [Jani]
>>> - use intel_display_power_well_is_enabled() instead of looking
>>> inside power_well count. [Jani]
>>> - fixes the lpsp capable conditional logic. [Jani]
>>> - combined the lpsp capable and enable info. [Jani]
>>>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
>>> ---
>>> .../drm/i915/display/intel_display_debugfs.c | 124 ++++++++++++++++++
>>> .../drm/i915/display/intel_display_power.h | 2 +
>>> 2 files changed, 126 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>>> index 424f4e52f783..b185c4617468 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>>> @@ -9,6 +9,7 @@
>>> #include "i915_debugfs.h"
>>> #include "intel_csr.h"
>>> #include "intel_display_debugfs.h"
>>> +#include "intel_display_power.h"
>>> #include "intel_display_types.h"
>>> #include "intel_dp.h"
>>> #include "intel_fbc.h"
>>> @@ -611,6 +612,98 @@ static void intel_hdcp_info(struct seq_file *m,
>>> seq_puts(m, "\n");
>>> }
>>> +#define LPSP_CAPABLE(COND) (COND ? seq_puts(m, "LPSP capable\n") : seq_puts(m, "LPSP incapable\n"))
>>> +#define LPSP_ENABLE(COND) (COND ? seq_puts(m, "LPSP enabled\n") : seq_puts(m, "LPSP disabled\n"))
>>> +
>>> +/* LVDS also an embedded panel but we are not interested in it */
>>> +static bool intel_have_embedded_panel(struct drm_connector *connector)
>>> +{
>>> + return connector->connector_type == DRM_MODE_CONNECTOR_DSI ||
>>> + connector->connector_type == DRM_MODE_CONNECTOR_eDP;
>>> +}
>>> +
>>> +static bool intel_have_gen9_lpsp_panel(struct drm_connector *connector)
>>> +{
>>> + return intel_have_embedded_panel(connector) ||
>>> + connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort;
>>> +}
>>> +
>>> +static bool intel_have_lpsp_supported_panel(struct drm_connector *connector)
>>> +{
>>> + return intel_have_gen9_lpsp_panel(connector) ||
>>> + connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
>>> + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB;
>>> +}
>> This function will pass for every platform for almost all (EDP/MIPI/DP/HDMI) connector type even if not supported ...rt?
> Above function will only used to add i915_lpsp_info for DP/MIPI/DP/HDMI connecotr,
> It can nuke it with condition below condition.
> return connector->connector_type == DRM_MODE_CONNECTOR_DSI || connector->connector_type == DRM_MODE_CONNECTOR_eDP ||
> connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort || connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB;
> So in order to avoid such big condition i had prefred the above function, please suggest a better readable way.
> use condition as such, function name change or using a macro.
>> Apart from that I did not understand the purpose of above functions.
> My thought process was to break longer conditional if chunk in static function,
> please suggest if u are in favour of nuke these functions, or if u want to suggest
> a better name.
>> Can we have a single function to check the connector is supported lpsp or not.
>>
>> static bool is_lpsp_supported(struct drm_connector *connector)
>>
>> In function definition we can check for platform first, then check connector_type and return true/false.
> i915_lpsp_info_show is first checking the gen and then platform, after checking
> platform it needs to check DDI port and connector based upon platform lpsp block diagram
> in order to evaluate lpsp capablity, followed by power well check to evlauate lpsp
> enabled.
> if i understand you are suggesting to break above two parts,
> is_lpsp_supported()
> is_lpsp_capable()
> but by breaking above we would require to check gen and platfrom twice
> which can be avoided by combining above.
No, as I mentioned below, both looks to me same, better to have one function.
if (is_lpsp_supported())
then add debugfs-entry.
Inside is_lpsp_supported() check everything related to connector. And in i915_lpsp_info_show() function check only power-well status and print lpsp is enabled or disabled
My understanding first i-g-t will check the lpsp capability of a connector then check the lpsp status to compare with our expectation ... right?
BTW, LPSP is related to single pipe with minimum power-well usage. No direct relation with port/connector. For headless configuration we can have lpsp mode enabled. How you will handle that?
Regards,
Animesh
> please correct me if i am wrong.
> Thanks ,
> Anshuman Gupta.
>>> +
>>> +static bool
>>> +intel_lpsp_power_well_enabled(struct drm_i915_private *dev_priv,
>>> + enum i915_power_well_id power_well_id)
>>> +{
>>> + intel_wakeref_t wakeref;
>>> + bool is_enabled;
>>> +
>>> + wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
>>> + is_enabled = intel_display_power_well_is_enabled(dev_priv,
>>> + power_well_id);
>>> + intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
>>> +
>>> + return is_enabled;
>>> +}
>>> +
>>> +static void
>>> +intel_lpsp_gen12_helper(struct seq_file *m, struct drm_connector *connector)
>>> +{
>>> + struct intel_encoder *encoder =
>>> + intel_attached_encoder(to_intel_connector(connector));
>>> + struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>> + bool lpsp_capable = false;
>>> +
>>> + if (IS_TIGERLAKE(dev_priv))
>>> + lpsp_capable = encoder->port <= PORT_C;
>>> +
>>> + LPSP_CAPABLE(lpsp_capable);
>> is_supported and capable looks similar to me, what is the difference?
> is_supported is just to differentiate from legacy connector, we don't want to
> add debugfs for legacy conector.
>>> + LPSP_ENABLE(!intel_lpsp_power_well_enabled(dev_priv, ICL_DISP_PW_3));
>>> +}
>>> +
>>> +static void
>>> +intel_lpsp_gen11_helper(struct seq_file *m, struct drm_connector *connector)
>>> +{
>>> + struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>> +
>>> + LPSP_CAPABLE(intel_have_embedded_panel(connector));
>>> + LPSP_ENABLE(!intel_lpsp_power_well_enabled(dev_priv, ICL_DISP_PW_3));
>>> +}
>>> +
>>> +static void
>>> +intel_lpsp_gen9_helper(struct seq_file *m, struct drm_connector *connector)
>>> +{
>>> + struct intel_encoder *encoder =
>>> + intel_attached_encoder(to_intel_connector(connector));
>>> + struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>> +
>>> + LPSP_CAPABLE(encoder->port == PORT_A &&
>>> + intel_have_gen9_lpsp_panel(connector));
>>> + LPSP_ENABLE(!intel_lpsp_power_well_enabled(dev_priv, SKL_DISP_PW_2));
>>> +}
>>> +
>>> +static void
>>> +intel_lpsp_legacy_gen_helper(struct seq_file *m,
>>> + struct drm_connector *connector)
>>> +{
>>> + struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>> +
>>> + /*
>>> + * Apart from HASWELL/BROADWELL other legacy platform doesn't
>>> + * support lpsp.
>>> + */
>>> + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>>> + LPSP_CAPABLE(connector->connector_type == DRM_MODE_CONNECTOR_eDP);
>>> + LPSP_ENABLE(!intel_lpsp_power_well_enabled(dev_priv, HSW_DISP_PW_GLOBAL));
>>> + } else {
>>> + seq_puts(m, "LPSP not supported\n");
>>> + }
>>> +}
>>> +
>>> static void intel_dp_info(struct seq_file *m,
>>> struct intel_connector *intel_connector)
>>> {
>>> @@ -1987,6 +2080,33 @@ static int i915_hdcp_sink_capability_show(struct seq_file *m, void *data)
>>> }
>>> DEFINE_SHOW_ATTRIBUTE(i915_hdcp_sink_capability);
>>> +static int i915_lpsp_info_show(struct seq_file *m, void *data)
>>> +{
>>> + struct drm_connector *connector = m->private;
>>> + struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>> +
>>> + if (connector->status != connector_status_connected)
>>> + return -ENODEV;
>>> +
>>> + switch (INTEL_GEN(dev_priv)) {
>>> + case 12:
>>> + intel_lpsp_gen12_helper(m, connector);
>>> + break;
>>> + case 11:
>>> + intel_lpsp_gen11_helper(m, connector);
>>> + break;
>>> + case 10:
>>> + case 9:
>>> + intel_lpsp_gen9_helper(m, connector);
>>> + break;
>>> + default:
>>> + intel_lpsp_legacy_gen_helper(m, connector);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(i915_lpsp_info);
>>> +
>>> static int i915_dsc_fec_support_show(struct seq_file *m, void *data)
>>> {
>>> struct drm_connector *connector = m->private;
>>> @@ -2130,5 +2250,9 @@ int intel_connector_debugfs_add(struct drm_connector *connector)
>>> debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
>>> connector, &i915_dsc_fec_support_fops);
>>> + if (intel_have_lpsp_supported_panel(connector))
>>> + debugfs_create_file("i915_lpsp_info", 0444, root,
>>> + connector, &i915_lpsp_info_fops);
>>> +
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
>>> index 56cbae6327b7..14c5ad20287f 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
>>> @@ -266,6 +266,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain);
>>> bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
>>> enum intel_display_power_domain domain);
>>> +bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
>>> + enum i915_power_well_id power_well_id);
>>> bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
>>> enum intel_display_power_domain domain);
>>> intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
More information about the Intel-gfx
mailing list