On Tue, Sep 10, 2024 at 07:40:04PM +0530, Nautiyal, Ankit K wrote:
>
> 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.
I guess 1,2,4 makes sense as we can just pass that into
eg. intel_mode_valid_max_plane_size() (though we need to
update all its other callers to pass a hardcoded 1 as well).
Just have to take care to not accidentally populate
crtc_state->joiner_pipes when num_pipes==1 (unless we rework
all the other places to do the right thing when there's just
one bit set in that bitmask).
--
Ville Syrjälä
Intel
