On 9/10/2024 5:16 PM, Ville Syrjälä wrote:
On Tue, Sep 10, 2024 at 11:12:30AM +0530, Nautiyal, Ankit K wrote:
On 9/9/2024 7:10 PM, Ville Syrjälä wrote:
On Mon, Sep 09, 2024 at 11:10:16AM +0530, Nautiyal, Ankit K wrote:
On 9/6/2024 8:24 PM, Ville Syrjälä wrote:
On Fri, Sep 06, 2024 at 05:46:11PM +0300, Ville Syrjälä wrote:
On Fri, Sep 06, 2024 at 06:27:54PM +0530, Ankit Nautiyal wrote:
At the moment, the debugfs for joiner allows only to force enable/disable
pipe joiner for 2 pipes. Modify it to force join 'n' number of pipes,
where n is a valid pipe joiner configuration.
This will help in case of ultra joiner where 4 pipes are joined.
v2:
-Fix commit message to state that only valid joiner config can be
forced. (Suraj)
-Rename the identifiers to have INTEL_BIG/NONE_JOINER_PIPES. (Suraj)
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
---
.../drm/i915/display/intel_display_debugfs.c | 71 ++++++++++++++++++-
.../drm/i915/display/intel_display_types.h | 8 ++-
drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
3 files changed, 77 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 830b9eb60976..0ef573afd8a1 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -1504,6 +1504,73 @@ static int intel_crtc_pipe_show(struct seq_file *m, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe);
+static int i915_joiner_show(struct seq_file *m, void *data)
+{
+ struct intel_connector *connector = m->private;
+ struct drm_i915_private *i915 = to_i915(connector->base.dev);
+ int ret;
+
+ ret = drm_modeset_lock_single_interruptible(&i915->drm.mode_config.connection_mutex);
+ if (ret)
+ return ret;
What does that lock do for us?
+
+ seq_printf(m, "Force_joined_pipes: %d\n", connector->force_joined_pipes);
This should just be thae bare number. Adding other junk in there just
complicates matters if anyone has to parse this.
+
+ drm_modeset_unlock(&i915->drm.mode_config.connection_mutex);
+
+ return ret;
+}
+
+static ssize_t i915_joiner_write(struct file *file,
+ const char __user *ubuf,
+ size_t len, loff_t *offp)
+{
+ struct seq_file *m = file->private_data;
+ struct intel_connector *connector = m->private;
+ struct drm_i915_private *i915 = to_i915(connector->base.dev);
+ int force_join_pipes = 0;
+ int ret;
+
+ if (len == 0)
+ return 0;
+
+ drm_dbg(&i915->drm,
+ "Copied %zu bytes from user to force joiner\n", len);
Leftover debug junk.
+
+ ret = kstrtoint_from_user(ubuf, len, 0, &force_join_pipes);
+ if (ret < 0)
+ return ret;
+
+ drm_dbg(&i915->drm, "Got %d for force joining pipes\n", force_join_pipes);
More.
+
+ if (force_join_pipes < INTEL_NONE_JOINER_PIPES ||
+ force_join_pipes >= INTEL_INVALID_JOINER_PIPES) {
+ drm_dbg(&i915->drm, "Ignoring Invalid num of pipes %d for force joining\n",
+ force_join_pipes);
+ connector->force_joined_pipes = INTEL_NONE_JOINER_PIPES;
+ } else {
+ connector->force_joined_pipes = force_join_pipes;
+ }
I think just something like
switch (num_pipes) {
case 0: /* or should 1 be the default? */
I suppose both 0 and 1 should be accepted. 0==not forced, 1==forced to
exactly one pipe (ie. no joiner despite what the automagic logic
is saying).
I understand 0 as not forced. I didnt get the meaning of forcing to one
pipe.
Does this mean, disable joiner? (Perhaps do not use joiner even for the
cases where driver thinks joiner is required)
How should we handle the case in driver, where it is 1?
Whatever code that determines how many pipes will should:
1) if the override is non-zero just use it
2) otherwise determine the number by using whatever
logic is appropriate
Alright, If I get correctly the driver logic will be something like:
int intel_dp_compute_joiner_pipes(struct intel_dp *intel_dp,
struct intel_connector *connector,
int hdisplay, int clock)
{
int num_joined_pipes = 0;
This variable looks redundant. You can just directly return
the correct number from the switch statement.
Yeah I was inititally going with that, but changed later. Will remove this.
switch (connector->force_joined_pipes) {
case 1:
num_joined_pipes = connector->force_joined_pipes;
This would now return 1, which is probably a value we never
want to return from here. Either that or we want to never
return 0 (which this code would do in some of the other
cases). Not sure which way is better tbh.
Currently I have coded to not allow 0, so we would return 1, 2, or 4
from here.
But I am open to what ever makes semantics intuitive, and handling easier.
break;
case 2:
if (intel_dp_has_joiner(intel_dp))
num_joined_pipes = connector->force_joined_pipes;
Hmm. We might want to make the debugfs knob already reject the
!has_joiner case so that the user won't even be allowed to
pick a completely unsupported value.
Alright, will have this checked in the function where we parse.
Thanks,
Ankit
break;
default:
MISSING_CASE(connector->force_joined_pipes);
fallthrough;
case 0:
if (intel_dp_has_joiner(intel_dp) &&
intel_dp_needs_bigjoiner(intel_dp, connector,
hdisplay, clock))
num_joined_pipes = 2;
}
return num_joined_pipes;
}
With a value of 1 we are kind of forcing to not use joiner.
Currently for testing sent this to trybot:
https://patchwork.freedesktop.org/patch/613627/?series=138444&rev=1
Regards,
Ankit
Regards,
Ankit
case 2:
case 4:
break;
default:
bad;
}
should do for validation.
