| Message ID | 20240905051841.3012729-2-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Vrr refactoring and panel replay workaround | expand |
On Thu, 05 Sep 2024, Animesh Manna <animesh.manna@intel.com> wrote: > Connector state is not used in lobf compute config, so remove it. > > Fixes: 15438b325987 ("drm/i915/alpm: Add compute config for lobf") This is refactoring, and not a fix. We don't need to have this backported. On the actual change, it's fine. But I do find myself thinking most of the similar functions on the encoder->compute_config() path should 1) be all named _compute_config, and 2) have the same parameter set as encoder->compute_config(), needed or not. Because often you are going to need something later, and then you end up having to plumb them all the way the stack to the destination. It's just unnecessary to keep removing and adding the parameters in the _compute_config() path. (And similarly for many other encoder hooks.) BR, Jani. > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > --- > drivers/gpu/drm/i915/display/intel_alpm.c | 3 +-- > drivers/gpu/drm/i915/display/intel_alpm.h | 3 +-- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c > index 186cf4833f71..f2fea356d28a 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > @@ -266,8 +266,7 @@ bool intel_alpm_compute_params(struct intel_dp *intel_dp, > } > > void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, > - struct intel_crtc_state *crtc_state, > - struct drm_connector_state *conn_state) > + struct intel_crtc_state *crtc_state) > { > struct intel_display *display = to_intel_display(intel_dp); > struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h b/drivers/gpu/drm/i915/display/intel_alpm.h > index 8c409b10dce6..a17dfaa5248d 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.h > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h > @@ -17,8 +17,7 @@ void intel_alpm_init_dpcd(struct intel_dp *intel_dp); > bool intel_alpm_compute_params(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, > - struct intel_crtc_state *crtc_state, > - struct drm_connector_state *conn_state); > + struct intel_crtc_state *crtc_state); > void intel_alpm_configure(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > void intel_alpm_lobf_debugfs_add(struct intel_connector *connector); > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index a1fcedfd404b..86bc6d79279f 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -3107,7 +3107,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, > intel_vrr_compute_config(pipe_config, conn_state); > intel_dp_compute_as_sdp(intel_dp, pipe_config); > intel_psr_compute_config(intel_dp, pipe_config, conn_state); > - intel_alpm_lobf_compute_config(intel_dp, pipe_config, conn_state); > + intel_alpm_lobf_compute_config(intel_dp, pipe_config); > intel_dp_drrs_compute_config(connector, pipe_config, link_bpp_x16); > intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state); > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state);
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Thursday, September 12, 2024 3:37 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: ville.syrjala@linux.intel.com; Hogander, Jouni > <jouni.hogander@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>; > Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Golani, Mitulkumar Ajitkumar > <mitulkumar.ajitkumar.golani@intel.com>; Manna, Animesh > <animesh.manna@intel.com> > Subject: Re: [PATCH v10 1/4] drm/i915/lobf: No need to pass connector-state > on lobf-compute-config. > > On Thu, 05 Sep 2024, Animesh Manna <animesh.manna@intel.com> wrote: > > Connector state is not used in lobf compute config, so remove it. > > > > Fixes: 15438b325987 ("drm/i915/alpm: Add compute config for lobf") > > This is refactoring, and not a fix. We don't need to have this backported. Sure will remove the fix tag, it was added as per previous feedback. > > On the actual change, it's fine. But I do find myself thinking most of the > similar functions on the encoder->compute_config() path should 1) be all > named _compute_config, and 2) have the same parameter set as > encoder->compute_config(), needed or not. Because often you are going to > need something later, and then you end up having to plumb them all the way > the stack to the destination. It's just unnecessary to keep removing and > adding the parameters in the _compute_config() path. (And similarly for > many other encoder hooks.) Currently atomic-check() path we do feature's requirement/dependency check and then compute-config which will be applied in atomic-commit. So I also feel _check() or _compute_config() suffix will be good for all functions in atomic-check() path. Regarding passing argument to any function, I see it is used as per need. Regards, Animesh > > BR, > Jani. > > > > > > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_alpm.c | 3 +-- > > drivers/gpu/drm/i915/display/intel_alpm.h | 3 +-- > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > 3 files changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > > b/drivers/gpu/drm/i915/display/intel_alpm.c > > index 186cf4833f71..f2fea356d28a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > > @@ -266,8 +266,7 @@ bool intel_alpm_compute_params(struct intel_dp > > *intel_dp, } > > > > void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, > > - struct intel_crtc_state *crtc_state, > > - struct drm_connector_state *conn_state) > > + struct intel_crtc_state *crtc_state) > > { > > struct intel_display *display = to_intel_display(intel_dp); > > struct drm_display_mode *adjusted_mode = > > &crtc_state->hw.adjusted_mode; diff --git > > a/drivers/gpu/drm/i915/display/intel_alpm.h > > b/drivers/gpu/drm/i915/display/intel_alpm.h > > index 8c409b10dce6..a17dfaa5248d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_alpm.h > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h > > @@ -17,8 +17,7 @@ void intel_alpm_init_dpcd(struct intel_dp > > *intel_dp); bool intel_alpm_compute_params(struct intel_dp *intel_dp, > > const struct intel_crtc_state *crtc_state); void > > intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, > > - struct intel_crtc_state *crtc_state, > > - struct drm_connector_state *conn_state); > > + struct intel_crtc_state *crtc_state); > > void intel_alpm_configure(struct intel_dp *intel_dp, > > const struct intel_crtc_state *crtc_state); void > > intel_alpm_lobf_debugfs_add(struct intel_connector *connector); diff > > --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index a1fcedfd404b..86bc6d79279f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -3107,7 +3107,7 @@ intel_dp_compute_config(struct intel_encoder > *encoder, > > intel_vrr_compute_config(pipe_config, conn_state); > > intel_dp_compute_as_sdp(intel_dp, pipe_config); > > intel_psr_compute_config(intel_dp, pipe_config, conn_state); > > - intel_alpm_lobf_compute_config(intel_dp, pipe_config, conn_state); > > + intel_alpm_lobf_compute_config(intel_dp, pipe_config); > > intel_dp_drrs_compute_config(connector, pipe_config, > link_bpp_x16); > > intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state); > > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, > pipe_config, > > conn_state); > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c index 186cf4833f71..f2fea356d28a 100644 --- a/drivers/gpu/drm/i915/display/intel_alpm.c +++ b/drivers/gpu/drm/i915/display/intel_alpm.c @@ -266,8 +266,7 @@ bool intel_alpm_compute_params(struct intel_dp *intel_dp, } void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, - struct intel_crtc_state *crtc_state, - struct drm_connector_state *conn_state) + struct intel_crtc_state *crtc_state) { struct intel_display *display = to_intel_display(intel_dp); struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h b/drivers/gpu/drm/i915/display/intel_alpm.h index 8c409b10dce6..a17dfaa5248d 100644 --- a/drivers/gpu/drm/i915/display/intel_alpm.h +++ b/drivers/gpu/drm/i915/display/intel_alpm.h @@ -17,8 +17,7 @@ void intel_alpm_init_dpcd(struct intel_dp *intel_dp); bool intel_alpm_compute_params(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state); void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, - struct intel_crtc_state *crtc_state, - struct drm_connector_state *conn_state); + struct intel_crtc_state *crtc_state); void intel_alpm_configure(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state); void intel_alpm_lobf_debugfs_add(struct intel_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index a1fcedfd404b..86bc6d79279f 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -3107,7 +3107,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, intel_vrr_compute_config(pipe_config, conn_state); intel_dp_compute_as_sdp(intel_dp, pipe_config); intel_psr_compute_config(intel_dp, pipe_config, conn_state); - intel_alpm_lobf_compute_config(intel_dp, pipe_config, conn_state); + intel_alpm_lobf_compute_config(intel_dp, pipe_config); intel_dp_drrs_compute_config(connector, pipe_config, link_bpp_x16); intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state); intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state);
Connector state is not used in lobf compute config, so remove it. Fixes: 15438b325987 ("drm/i915/alpm: Add compute config for lobf") Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> Signed-off-by: Animesh Manna <animesh.manna@intel.com> --- drivers/gpu/drm/i915/display/intel_alpm.c | 3 +-- drivers/gpu/drm/i915/display/intel_alpm.h | 3 +-- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-)