Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Tue, 05 Mar 2024, Ville Syrjälä wrote: > If the spec got updated then we should probably just do a full rename > pass over the whole codebase instead of confusing things more by > mixing up the terminology. For example: Bspec 54142 for display version 12+ has some mixed old/new terminology. Bspec 68847 for xe2lpd+ only has new terminology. > Also we should probably s/bigjoiner/joiner/ to make it clear that > all of it also applies to uncompressed joiner as well. Ack. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Tue, Mar 05, 2024 at 04:07:45PM +0200, Jani Nikula wrote: > On Tue, 05 Mar 2024, Ville Syrjälä wrote: > > On Tue, Mar 05, 2024 at 10:41:49AM +0200, Lisovskiy, Stanislav wrote: > >> I also wonder whether should we really emphasize things like > >> "master"/"slave" > >> in function names. I thought that one idea in our refactoring was to unify > >> joined pipes handling so that there are no(or at least almost no) explicit > >> code > >> paths/function names for masters/slaves. > > > > There are no master vs. slave functions. The split is going to be > > transcoder/port vs. pipe. > > Besides, for modern platforms the spec has already been changed to use > primary/secondary terminology. When renaming or refactoring stuff, > please switch to them instead of sticking with master/slave. If the spec got updated then we should probably just do a full rename pass over the whole codebase instead of confusing things more by mixing up the terminology. Also we should probably s/bigjoiner/joiner/ to make it clear that all of it also applies to uncompressed joiner as well. -- Ville Syrjälä Intel
Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Tue, 05 Mar 2024, Ville Syrjälä wrote: > On Tue, Mar 05, 2024 at 10:41:49AM +0200, Lisovskiy, Stanislav wrote: >> I also wonder whether should we really emphasize things like "master"/"slave" >> in function names. I thought that one idea in our refactoring was to unify >> joined pipes handling so that there are no(or at least almost no) explicit >> code >> paths/function names for masters/slaves. > > There are no master vs. slave functions. The split is going to be > transcoder/port vs. pipe. Besides, for modern platforms the spec has already been changed to use primary/secondary terminology. When renaming or refactoring stuff, please switch to them instead of sticking with master/slave. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Tue, Mar 05, 2024 at 11:08:56AM +0200, Lisovskiy, Stanislav wrote: > On Tue, Mar 05, 2024 at 10:50:01AM +0200, Ville Syrjälä wrote: > > On Tue, Mar 05, 2024 at 10:41:49AM +0200, Lisovskiy, Stanislav wrote: > > > On Fri, Mar 01, 2024 at 04:35:53PM +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä > > > > > > > > In preparation for doing a more sensible pipe vs. transcoder > > > > handling for bigjoiner let's rename the crtc/crtc_state in the > > > > top level crtc_enable/disable and the DDI encoder hooks to > > > > include "master" in the name. This way they won't collide with > > > > the per-pipe stuff. > > > > > > > > Note that at this point this is (at least partially) telling > > > > lies as we still run through some of these for slave pipes as > > > > well. But I wanted to get the huge rename out of the way so > > > > it won't clutter the functional patches so much. > > > > > > > > TODO: or perhaps use some other names for the per-pipe stuff instead? > > > > > > > > Signed-off-by: Ville Syrjälä > > > > > > I will then review now the patches which you could merge before the > > > bigjoiner > > > stuff could be finished. > > > > I just sent a separate series with the disable_pipes bitmask > > stuff. > > I already reviewed all the patches, including that one, if there were > no changes, I guess you can apply that r-b there as well. Sure. Thanks. > > > > > > Checked this patch I guess, you were also talking that this renaming might > > > be not the best idea. > > > I also wonder whether should we really emphasize things like > > > "master"/"slave" > > > in function names. I thought that one idea in our refactoring was to unify > > > joined pipes handling so that there are no(or at least almost no) > > > explicit code > > > paths/function names for masters/slaves. > > > > There are no master vs. slave functions. The split is going to be > > transcoder/port vs. pipe. > > In practice thats what you want to achieve, the functions which also include > encoder > programming and/or handling joined pipes you wanted to add master in the name. I wanted clarity which crtc state is for which purpose. But I think we achieve that by naming the per-pipe variables a bit differently instead (eg. pipe_crtc + pipe_crtc_state). -- Ville Syrjälä Intel
Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Tue, Mar 05, 2024 at 10:50:01AM +0200, Ville Syrjälä wrote: > On Tue, Mar 05, 2024 at 10:41:49AM +0200, Lisovskiy, Stanislav wrote: > > On Fri, Mar 01, 2024 at 04:35:53PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > > > In preparation for doing a more sensible pipe vs. transcoder > > > handling for bigjoiner let's rename the crtc/crtc_state in the > > > top level crtc_enable/disable and the DDI encoder hooks to > > > include "master" in the name. This way they won't collide with > > > the per-pipe stuff. > > > > > > Note that at this point this is (at least partially) telling > > > lies as we still run through some of these for slave pipes as > > > well. But I wanted to get the huge rename out of the way so > > > it won't clutter the functional patches so much. > > > > > > TODO: or perhaps use some other names for the per-pipe stuff instead? > > > > > > Signed-off-by: Ville Syrjälä > > > > I will then review now the patches which you could merge before the > > bigjoiner > > stuff could be finished. > > I just sent a separate series with the disable_pipes bitmask > stuff. I already reviewed all the patches, including that one, if there were no changes, I guess you can apply that r-b there as well. > > > Checked this patch I guess, you were also talking that this renaming might > > be not the best idea. > > I also wonder whether should we really emphasize things like > > "master"/"slave" > > in function names. I thought that one idea in our refactoring was to unify > > joined pipes handling so that there are no(or at least almost no) explicit > > code > > paths/function names for masters/slaves. > > There are no master vs. slave functions. The split is going to be > transcoder/port vs. pipe. In practice thats what you want to achieve, the functions which also include encoder programming and/or handling joined pipes you wanted to add master in the name. I think we should try to mention master/slave explicitly as less as possible. Stan > > -- > Ville Syrjälä > Intel
Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Tue, Mar 05, 2024 at 10:41:49AM +0200, Lisovskiy, Stanislav wrote: > On Fri, Mar 01, 2024 at 04:35:53PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > In preparation for doing a more sensible pipe vs. transcoder > > handling for bigjoiner let's rename the crtc/crtc_state in the > > top level crtc_enable/disable and the DDI encoder hooks to > > include "master" in the name. This way they won't collide with > > the per-pipe stuff. > > > > Note that at this point this is (at least partially) telling > > lies as we still run through some of these for slave pipes as > > well. But I wanted to get the huge rename out of the way so > > it won't clutter the functional patches so much. > > > > TODO: or perhaps use some other names for the per-pipe stuff instead? > > > > Signed-off-by: Ville Syrjälä > > I will then review now the patches which you could merge before the bigjoiner > stuff could be finished. I just sent a separate series with the disable_pipes bitmask stuff. > Checked this patch I guess, you were also talking that this renaming might > be not the best idea. > I also wonder whether should we really emphasize things like "master"/"slave" > in function names. I thought that one idea in our refactoring was to unify > joined pipes handling so that there are no(or at least almost no) explicit > code > paths/function names for masters/slaves. There are no master vs. slave functions. The split is going to be transcoder/port vs. pipe. -- Ville Syrjälä Intel
Re: [PATCH 1/8] drm/i915: Rename the crtc/crtc_states in the top level DDI hooks/etc
On Fri, Mar 01, 2024 at 04:35:53PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> In preparation for doing a more sensible pipe vs. transcoder
> handling for bigjoiner let's rename the crtc/crtc_state in the
> top level crtc_enable/disable and the DDI encoder hooks to
> include "master" in the name. This way they won't collide with
> the per-pipe stuff.
>
> Note that at this point this is (at least partially) telling
> lies as we still run through some of these for slave pipes as
> well. But I wanted to get the huge rename out of the way so
> it won't clutter the functional patches so much.
>
> TODO: or perhaps use some other names for the per-pipe stuff instead?
>
> Signed-off-by: Ville Syrjälä
I will then review now the patches which you could merge before the bigjoiner
stuff could be finished.
Checked this patch I guess, you were also talking that this renaming might
be not the best idea.
I also wonder whether should we really emphasize things like "master"/"slave"
in function names. I thought that one idea in our refactoring was to unify
joined pipes handling so that there are no(or at least almost no) explicit code
paths/function names for masters/slaves.
Stan
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 326 +--
> drivers/gpu/drm/i915/display/intel_display.c | 100 +++---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 91 +++---
> 3 files changed, 258 insertions(+), 259 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index c587a8efeafc..6287629f9e77 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2490,21 +2490,21 @@ static void mtl_port_buf_ctl_io_selection(struct
> intel_encoder *encoder)
>
> static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> struct intel_encoder *encoder,
> - const struct intel_crtc_state *crtc_state,
> + const struct intel_crtc_state
> *master_crtc_state,
> const struct drm_connector_state *conn_state)
> {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> - bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> + bool is_mst = intel_crtc_has_type(master_crtc_state,
> INTEL_OUTPUT_DP_MST);
>
> intel_dp_set_link_params(intel_dp,
> - crtc_state->port_clock,
> - crtc_state->lane_count);
> + master_crtc_state->port_clock,
> + master_crtc_state->lane_count);
>
> /*
>* We only configure what the register value will be here. Actual
>* enabling happens during link training farther down.
>*/
> - intel_ddi_init_dp_buf_reg(encoder, crtc_state);
> + intel_ddi_init_dp_buf_reg(encoder, master_crtc_state);
>
> /*
>* 1. Enable Power Wells
> @@ -2522,48 +2522,48 @@ static void mtl_ddi_pre_enable_dp(struct
> intel_atomic_state *state,
> intel_pps_on(intel_dp);
>
> /* 5. Enable the port PLL */
> - intel_ddi_enable_clock(encoder, crtc_state);
> + intel_ddi_enable_clock(encoder, master_crtc_state);
>
> /*
>* 6.a Configure Transcoder Clock Select to direct the Port clock to the
>* Transcoder.
>*/
> - intel_ddi_enable_transcoder_clock(encoder, crtc_state);
> + intel_ddi_enable_transcoder_clock(encoder, master_crtc_state);
>
> /*
>* 6.b If DP v2.0/128b mode - Configure TRANS_DP2_CTL register settings.
>*/
> - intel_ddi_config_transcoder_dp2(encoder, crtc_state);
> + intel_ddi_config_transcoder_dp2(encoder, master_crtc_state);
>
> /*
>* 6.c Configure TRANS_DDI_FUNC_CTL DDI Select, DDI Mode Select & MST
>* Transport Select
>*/
> - intel_ddi_config_transcoder_func(encoder, crtc_state);
> + intel_ddi_config_transcoder_func(encoder, master_crtc_state);
>
> /*
>* 6.e Program CoG/MSO configuration bits in DSS_CTL1 if selected.
>*/
> - intel_ddi_mso_configure(crtc_state);
> + intel_ddi_mso_configure(master_crtc_state);
>
> if (!is_mst)
> intel_dp_set_power(intel_dp, DP_SET_POWER_D0);
>
> - intel_dp_configure_protocol_converter(intel_dp, crtc_state);
> + intel_dp_configure_protocol_converter(intel_dp, master_crtc_state);
> if (!is_mst)
> intel_dp_sink_enable_decompression(state,
>
> to_intel_connector(conn_state->connector),
> -crtc_state);
> +master_crtc_state);
>
> /*
>* DDI FEC: "anticipates enabling FEC encoding sets the FEC_READY bit
>* in the FEC_CONFIGURATION register to 1 before
