On Wed, 30 Oct 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Wed, Oct 30, 2024 at 01:34:38PM +0200, Jani Nikula wrote:
>> On Tue, 29 Oct 2024, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > Pull the DSI min cdclk calculation into a helper and hide
>> > it inside vlv_dsi.c in order to keep most DSI related
>> > details in one place.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++------------------
>> > drivers/gpu/drm/i915/display/vlv_dsi.c | 25 ++++++++++++++++++++++
>> > drivers/gpu/drm/i915/display/vlv_dsi.h | 8 +++++++
>> > 3 files changed, 35 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > index 89d12c521411..e10378744607 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > @@ -46,6 +46,7 @@
>> > #include "intel_vdsc.h"
>> > #include "skl_watermark.h"
>> > #include "skl_watermark_regs.h"
>> > +#include "vlv_dsi.h"
>> > #include "vlv_sideband.h"
>> >
>> > /**
>> > @@ -2849,8 +2850,6 @@ static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
>> >
>> > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>> > {
>> > - struct intel_display *display = to_intel_display(crtc_state);
>> > - struct drm_i915_private *dev_priv = to_i915(display->drm);
>> > int min_cdclk;
>> >
>> > if (!crtc_state->hw.enable)
>> > @@ -2859,25 +2858,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>> > min_cdclk = intel_pixel_rate_to_cdclk(crtc_state);
>> > min_cdclk = max(hsw_ips_min_cdclk(crtc_state), min_cdclk);
>> > min_cdclk = max(intel_audio_min_cdclk(crtc_state), min_cdclk);
>> > -
>> > - /*
>> > - * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>> > - * than 320000KHz.
>> > - */
>> > - if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
>> > - IS_VALLEYVIEW(dev_priv))
>> > - min_cdclk = max(320000, min_cdclk);
>> > -
>> > - /*
>> > - * On Geminilake once the CDCLK gets as low as 79200
>> > - * picture gets unstable, despite that values are
>> > - * correct for DSI PLL and DE PLL.
>> > - */
>> > - if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
>> > - IS_GEMINILAKE(dev_priv))
>> > - min_cdclk = max(158400, min_cdclk);
>> > -
>> > - /* Account for additional needs from the planes */
>> > + min_cdclk = max(vlv_dsi_min_cdclk(crtc_state), min_cdclk);
>> > min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
>> >
>> > if (crtc_state->dsc.compression_enable)
>> > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
>> > index 9383eedee2d4..49a895589150 100644
>> > --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
>> > +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
>> > @@ -1760,6 +1760,31 @@ static void vlv_dphy_param_init(struct intel_dsi *intel_dsi)
>> > intel_dsi_log_params(intel_dsi);
>> > }
>> >
>> > +int vlv_dsi_min_cdclk(const struct intel_crtc_state *crtc_state)
>> > +{
>> > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
>> > + int min_cdclk = 0;
>> > +
>> > + /*
>> > + * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
>> > + * than 320000KHz.
>> > + */
>> > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
>> > + IS_VALLEYVIEW(dev_priv))
>> > + min_cdclk = max(320000, min_cdclk);
>> > +
>> > + /*
>> > + * On Geminilake once the CDCLK gets as low as 79200
>> > + * picture gets unstable, despite that values are
>> > + * correct for DSI PLL and DE PLL.
>> > + */
>> > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
>> > + IS_GEMINILAKE(dev_priv))
>> > + min_cdclk = max(158400, min_cdclk);
>> > +
>> > + return min_cdclk;
>> > +}
>> > +
>> > typedef void (*vlv_dsi_dmi_quirk_func)(struct intel_dsi *intel_dsi);
>> >
>> > /*
>> > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.h b/drivers/gpu/drm/i915/display/vlv_dsi.h
>> > index cf9d7b82f288..5f99059b4c48 100644
>> > --- a/drivers/gpu/drm/i915/display/vlv_dsi.h
>> > +++ b/drivers/gpu/drm/i915/display/vlv_dsi.h
>> > @@ -8,13 +8,17 @@
>> >
>> > #include <linux/types.h>
>> >
>> > +#include <drm/drm_mipi_dsi.h>
>> > +
>>
>> Huh, why is this required? At least it's unrelated to the patch.
>
> Sorry meant to note that in the commit msg, but forgot.
>
> xe fails to build without this:
> ../drivers/gpu/drm/i915/display/vlv_dsi.h:28:42: error: return type is an incomplete type
>
> It looks like a forward declaration is not enough for
> return types of static inline functions. i915 on the other
> hand builds fine.
>
> I suppose one alternative would be to declare is as just
> 'int' for xe.
I think I've actually snuck in some of those int usages in the past,
because in the end it doesn't really matter for the inline stubs.
BR,
Jani.
>
>>
>> Other than that,
>>
>> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>
>>
>> > enum port;
>> > struct drm_i915_private;
>> > +struct intel_crtc_state;
>> > struct intel_dsi;
>> >
>> > #ifdef I915
>> > void vlv_dsi_wait_for_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>> > enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt);
>> > +int vlv_dsi_min_cdclk(const struct intel_crtc_state *crtc_state);
>> > void vlv_dsi_init(struct drm_i915_private *dev_priv);
>> > #else
>> > static inline void vlv_dsi_wait_for_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
>> > @@ -24,6 +28,10 @@ static inline enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt
>> > {
>> > return 0;
>> > }
>> > +static inline int vlv_dsi_min_cdclk(const struct intel_crtc_state *crtc_state)
>> > +{
>> > + return 0;
>> > +}
>> > static inline void vlv_dsi_init(struct drm_i915_private *dev_priv)
>> > {
>> > }
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
