| Message ID | 20240906125807.3960642-2-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ultrajoiner basic functionality series | expand |
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@intel.com> > --- > .../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? */ case 2: case 4: break; default: bad; } should do for validation. > + > + *offp += len; I don't suppose there's any kind of helper for creating a debugfs file with a standard type, with some kind of caller specified validation function? Would avoid having to hand roll all this read syscall cruft for what is a fairly common usecase... > + > + return len; > +} > + > +static int i915_joiner_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, i915_joiner_show, inode->i_private); > +} > + > +static const struct file_operations i915_joiner_fops = { > + .owner = THIS_MODULE, > + .open = i915_joiner_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > + .write = i915_joiner_write > +}; > + > /** > * intel_connector_debugfs_add - add i915 specific connector debugfs files > * @connector: pointer to a registered intel_connector > @@ -1553,8 +1620,8 @@ void intel_connector_debugfs_add(struct intel_connector *connector) > if (DISPLAY_VER(i915) >= 11 && > (connector_type == DRM_MODE_CONNECTOR_DisplayPort || > connector_type == DRM_MODE_CONNECTOR_eDP)) { > - debugfs_create_bool("i915_bigjoiner_force_enable", 0644, root, > - &connector->force_bigjoiner_enable); > + debugfs_create_file("i915_joiner_force_enable", 0644, root, > + connector, &i915_joiner_fops); > } > > if (connector_type == DRM_MODE_CONNECTOR_DSI || > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 733de5edcfdb..c213fb61ceb7 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -486,6 +486,12 @@ struct intel_hdcp { > enum transcoder stream_transcoder; > }; > > +enum intel_joiner_pipe_count { > + INTEL_NONE_JOINER_PIPES = 0, > + INTEL_BIG_JOINER_PIPES = 2, > + INTEL_INVALID_JOINER_PIPES, > +}; That just looks like obfuscation to me. Just a bare number should do. > + > struct intel_connector { > struct drm_connector base; > /* > @@ -524,7 +530,7 @@ struct intel_connector { > > struct intel_dp *mst_port; > > - bool force_bigjoiner_enable; > + enum intel_joiner_pipe_count force_joined_pipes; > > struct { > struct drm_dp_aux *dsc_decompression_aux; > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index a1fcedfd404b..862a460c32b7 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1271,7 +1271,7 @@ bool intel_dp_need_joiner(struct intel_dp *intel_dp, > return false; > > return clock > i915->display.cdclk.max_dotclk_freq || hdisplay > 5120 || > - connector->force_bigjoiner_enable; > + connector->force_joined_pipes == INTEL_BIG_JOINER_PIPES; > } > > bool intel_dp_has_dsc(const struct intel_connector *connector) > -- > 2.45.2
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@intel.com> > > --- > > .../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). > case 2: > case 4: > break; > default: > bad; > } > > should do for validation. >
Hi Ville, Thanks for the comments and suggestions. Will remove the extra things that are not required. Please my response inline: On 9/6/2024 8:16 PM, 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@intel.com> >> --- >> .../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? This might be leftover from other debugfs, I'll remove this. > >> + >> + 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. Alright, will just have the number (force joined pipes). > >> + >> + 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. Will remove this. > >> + >> + 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. Will get rid of this. > >> + >> + 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? */ > case 2: > case 4: > break; > default: > bad; > } > > should do for validation. > >> + >> + *offp += len; > I don't suppose there's any kind of helper for creating a debugfs > file with a standard type, with some kind of caller specified > validation function? Would avoid having to hand roll all this > read syscall cruft for what is a fairly common usecase... > >> + >> + return len; >> +} >> + >> +static int i915_joiner_open(struct inode *inode, struct file *file) >> +{ >> + return single_open(file, i915_joiner_show, inode->i_private); >> +} >> + >> +static const struct file_operations i915_joiner_fops = { >> + .owner = THIS_MODULE, >> + .open = i915_joiner_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = single_release, >> + .write = i915_joiner_write >> +}; >> + >> /** >> * intel_connector_debugfs_add - add i915 specific connector debugfs files >> * @connector: pointer to a registered intel_connector >> @@ -1553,8 +1620,8 @@ void intel_connector_debugfs_add(struct intel_connector *connector) >> if (DISPLAY_VER(i915) >= 11 && >> (connector_type == DRM_MODE_CONNECTOR_DisplayPort || >> connector_type == DRM_MODE_CONNECTOR_eDP)) { >> - debugfs_create_bool("i915_bigjoiner_force_enable", 0644, root, >> - &connector->force_bigjoiner_enable); >> + debugfs_create_file("i915_joiner_force_enable", 0644, root, >> + connector, &i915_joiner_fops); >> } >> >> if (connector_type == DRM_MODE_CONNECTOR_DSI || >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >> index 733de5edcfdb..c213fb61ceb7 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -486,6 +486,12 @@ struct intel_hdcp { >> enum transcoder stream_transcoder; >> }; >> >> +enum intel_joiner_pipe_count { >> + INTEL_NONE_JOINER_PIPES = 0, >> + INTEL_BIG_JOINER_PIPES = 2, >> + INTEL_INVALID_JOINER_PIPES, >> +}; > That just looks like obfuscation to me. > Just a bare number should do. I was thinking to use these in rest of the places to avoid magic numbers, but if it makes it unreadable, I can do away with this. Thanks & regards, Ankit > >> + >> struct intel_connector { >> struct drm_connector base; >> /* >> @@ -524,7 +530,7 @@ struct intel_connector { >> >> struct intel_dp *mst_port; >> >> - bool force_bigjoiner_enable; >> + enum intel_joiner_pipe_count force_joined_pipes; >> >> struct { >> struct drm_dp_aux *dsc_decompression_aux; >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index a1fcedfd404b..862a460c32b7 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -1271,7 +1271,7 @@ bool intel_dp_need_joiner(struct intel_dp *intel_dp, >> return false; >> >> return clock > i915->display.cdclk.max_dotclk_freq || hdisplay > 5120 || >> - connector->force_bigjoiner_enable; >> + connector->force_joined_pipes == INTEL_BIG_JOINER_PIPES; >> } >> >> bool intel_dp_has_dsc(const struct intel_connector *connector) >> -- >> 2.45.2
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@intel.com> >>> --- >>> .../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? Regards, Ankit > >> case 2: >> case 4: >> break; >> default: >> bad; >> } >> >> should do for validation. >>
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@intel.com> > >>> --- > >>> .../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 > > > Regards, > > Ankit > > > > >> case 2: > >> case 4: > >> break; > >> default: > >> bad; > >> } > >> > >> should do for validation. > >>
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@intel.com> >>>>> --- >>>>> .../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; switch (connector->force_joined_pipes) { case 1: num_joined_pipes = connector->force_joined_pipes; break; case 2: if (intel_dp_has_joiner(intel_dp)) num_joined_pipes = connector->force_joined_pipes; 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. >>>>
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@intel.com> > >>>>> --- > >>>>> .../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. > > 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. > 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. > 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. > >>>>
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@intel.com> >>>>>>> --- >>>>>>> .../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. >>>>>>
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@intel.com> > >>>>>>> --- > >>>>>>> .../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).
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; + + seq_printf(m, "Force_joined_pipes: %d\n", connector->force_joined_pipes); + + 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); + + 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); + + 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; + } + + *offp += len; + + return len; +} + +static int i915_joiner_open(struct inode *inode, struct file *file) +{ + return single_open(file, i915_joiner_show, inode->i_private); +} + +static const struct file_operations i915_joiner_fops = { + .owner = THIS_MODULE, + .open = i915_joiner_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, + .write = i915_joiner_write +}; + /** * intel_connector_debugfs_add - add i915 specific connector debugfs files * @connector: pointer to a registered intel_connector @@ -1553,8 +1620,8 @@ void intel_connector_debugfs_add(struct intel_connector *connector) if (DISPLAY_VER(i915) >= 11 && (connector_type == DRM_MODE_CONNECTOR_DisplayPort || connector_type == DRM_MODE_CONNECTOR_eDP)) { - debugfs_create_bool("i915_bigjoiner_force_enable", 0644, root, - &connector->force_bigjoiner_enable); + debugfs_create_file("i915_joiner_force_enable", 0644, root, + connector, &i915_joiner_fops); } if (connector_type == DRM_MODE_CONNECTOR_DSI || diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 733de5edcfdb..c213fb61ceb7 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -486,6 +486,12 @@ struct intel_hdcp { enum transcoder stream_transcoder; }; +enum intel_joiner_pipe_count { + INTEL_NONE_JOINER_PIPES = 0, + INTEL_BIG_JOINER_PIPES = 2, + INTEL_INVALID_JOINER_PIPES, +}; + struct intel_connector { struct drm_connector base; /* @@ -524,7 +530,7 @@ struct intel_connector { struct intel_dp *mst_port; - bool force_bigjoiner_enable; + enum intel_joiner_pipe_count force_joined_pipes; struct { struct drm_dp_aux *dsc_decompression_aux; diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index a1fcedfd404b..862a460c32b7 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1271,7 +1271,7 @@ bool intel_dp_need_joiner(struct intel_dp *intel_dp, return false; return clock > i915->display.cdclk.max_dotclk_freq || hdisplay > 5120 || - connector->force_bigjoiner_enable; + connector->force_joined_pipes == INTEL_BIG_JOINER_PIPES; } bool intel_dp_has_dsc(const struct intel_connector *connector)
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@intel.com> --- .../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(-)