Hi,
On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> As per TRM this controller supports pixelclocks starting from 25 MHz. The
> maximum supported pixelclocks are defined by the phy configurations we
> have. Also it can't support modes that require doubled clocks.
> If there is a phy reference clock we can additionally validate against
> VESA DMT's recommendations.
> Those checks are added to the mode_valid hook of the connector and
> encoder's mode_fixup hook.
>
> Signed-off-by: Alex Bee <knaerzche@xxxxxxxxx>
> ---
> drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index f7f0bec725f9..2f839ff31c1c 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
> struct inno_hdmi_phy_config *default_phy_config;
> };
>
> +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
> +
> struct hdmi_data_info {
> int vic;
> bool sink_has_audio;
> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> return 0;
> }
>
> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> + struct drm_display_mode *mode)
> +{
So, mode_valid is only called to filter out the modes retrieved by
get_modes, but it won't be called when userspace programs a mode. That's
atomic_check's job.
So you probably want to create a shared function between atomic_check
and mode_valid, and call it from both places (or call mode_valid from
atomic_check).
> + /* No support for double-clock modes */
> + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> + return MODE_BAD;
> +
> + unsigned int mpixelclk = mode->clock * 1000;
Variables should be declared at the top of the function.
> + if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> + return MODE_CLOCK_LOW;
You probably want to check the max TMDS clock too?
> + if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
> + return MODE_CLOCK_HIGH;
> +
> + if (hdmi->refclk) {
> + long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
> + unsigned int max_tolerance = mpixelclk / 5000;
> +
> + /* Vesa DMT standard mentions +/- 0.5% max tolerance */
> + if (abs(refclk - mpixelclk) > max_tolerance ||
> + mpixelclk - refclk > max_tolerance;
> + return MODE_NOCLOCK;
You should use abs_diff here. abs() will get confused by the unsigned vs
signed comparison.
> + }
> +
> + return MODE_OK;
> +}
> +
> static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
> struct drm_display_mode *mode,
> struct drm_display_mode *adj_mode)
> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
> const struct drm_display_mode *mode,
> struct drm_display_mode *adj_mode)
> {
> - return true;
> + struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> +
> + return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
> }
Why do you call mode_valid in mode_fixup?
Maxime
Attachment:
signature.asc
Description: PGP signature
