Re: [Intel-gfx] [PATCH] drm/i915/display: A check on mode->clock, if it exceeds max_dot_clock

[Jani Nikula <jani.nikula@linux.intel.com>]

Although the change isn't needed as Ville points out, I'll take the
opportunity to comment on other things here, with hopes it'll improve
future contributions.

On Wed, 23 Nov 2022, Nischal Varide <nischal.varide@intel.com> wrote:
> Making sure that the  mode-clock is not greater than the
> max_dot_clock freq.This patch will prevent attempts from
> userspace to modeset with mode->clock greater than
> max_dot_clock freq.

Both in the patch subject and commit message, please use the imperative
mood, e.g. "add", "make sure", "prevent".

Please don't refer to "this patch". When it's been committed, it's no
longer a patch. But don't refer to "this commit" either. Just say what
the patch does, "Prevent ...".

>
> Signed-off-by: Nischal Varide <nischal.varide@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c   | 10 ++++++++++
>  drivers/gpu/drm/i915/display/intel_hdmi.c |  3 +++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7400d6b4c587..e603b7c01d27 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2045,6 +2045,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	if (intel_dp_hdisplay_bad(dev_priv, adjusted_mode->crtc_hdisplay))
>  		return -EINVAL;
>  
> +	if (!intel_dp_can_bigjoiner(intel_dp) &&
> +			(adjusted_mode->clock > dev_priv->max_dotclk_freq))
> +		return -EINVAL;
> +
> +	if (intel_dp_can_bigjoiner(intel_dp) &&
> +		(adjusted_mode->clock >  (2 * dev_priv->max_dotclk_freq)))
> +		return -EINVAL;
> +
> +
> +

The alignment is wrong, there are unnecessary parenthesis and
superfluous newlines. We care about this stuff, and we would not apply
the patch without fixing, requiring an extra version.

All this would be caught by checkpatch. Please make it a habit to run
checkpatch.pl --strict locally on your patches before submitting. (Or
dim checkpatch.)

BR,
Jani.


>  	/*
>  	 * Try to respect downstream TMDS clock limits first, if
>  	 * that fails assume the user might know something we don't.
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 02f8374ea51f..50603806a964 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2247,6 +2247,9 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
>  		return -EINVAL;
>  
> +	if (adjusted_mode->clock > dev_priv->max_dotclk_freq)
> +		return -EINVAL;
> +
>  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
>  	pipe_config->has_hdmi_sink = intel_has_hdmi_sink(intel_hdmi,
>  							 conn_state);

-- 
Jani Nikula, Intel Open Source Graphics Center