diff mbox series

[1/6] drm/display: hdmi: add generic mode_valid helper

Message ID 20241018-hdmi-mode-valid-v1-1-6e49ae4801f7@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/display: hdmi: add drm_hdmi_connector_mode_valid() | expand

Commit Message

Dmitry Baryshkov Oct. 18, 2024, 8:34 p.m. UTC 
Add drm_hdmi_connector_mode_valid(), generic helper for HDMI connectors.
It can be either used directly or as a part of the .mode_valid callback.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 25 +++++++++++++++++++++++++
 include/drm/display/drm_hdmi_helper.h     |  4 ++++
 2 files changed, 29 insertions(+)

Comments

Maxime Ripard Oct. 21, 2024, 9:32 a.m. UTC | #1 
On Fri, Oct 18, 2024 at 11:34:19PM +0300, Dmitry Baryshkov wrote:
> Add drm_hdmi_connector_mode_valid(), generic helper for HDMI connectors.
> It can be either used directly or as a part of the .mode_valid callback.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/display/drm_hdmi_helper.c | 25 +++++++++++++++++++++++++
>  include/drm/display/drm_hdmi_helper.h     |  4 ++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 74dd4d01dd9b..0ac5cb000ee2 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -256,3 +256,28 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
>  	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
>  }
>  EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> +
> +/**
> + * drm_hdmi_connector_mode_valid() - Check if mode is valid for HDMI connector
> + * @connector: DRM connector to validate the mode
> + * @mode: Display mode to validate
> + *
> + * Generic .mode_valid implementation for HDMI connectors.
> + */
> +enum drm_mode_status
> +drm_hdmi_connector_mode_valid(const struct drm_connector *connector,
> +			      const struct drm_display_mode *mode)
> +{
> +	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
> +	unsigned long long rate;
> +
> +	rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
> +	if (!rate)
> +		return MODE_ERROR;
> +
> +	if (!funcs || !funcs->tmds_char_rate_valid)
> +		return MODE_OK;
> +
> +	return funcs->tmds_char_rate_valid(connector, mode, rate);
> +}
> +EXPORT_SYMBOL(drm_hdmi_connector_mode_valid);

As discussed in the discussion that sparked that change, I believe that
we should use hdmi_clock_valid.

AFAIU, your concern was that max_tmds_clock might get stale, but then it
would not only prevent mode_valid from running but also the commit
entirely.

We don't have any evidence from that, so I'd rather try to keep
consistency between the two. And we can always try to address whatever
issue we might have if it turned out to be a bad idea :)

Maxime
Dmitry Baryshkov Oct. 21, 2024, 1:44 p.m. UTC | #2 
On Mon, Oct 21, 2024 at 11:32:03AM +0200, Maxime Ripard wrote:
> On Fri, Oct 18, 2024 at 11:34:19PM +0300, Dmitry Baryshkov wrote:
> > Add drm_hdmi_connector_mode_valid(), generic helper for HDMI connectors.
> > It can be either used directly or as a part of the .mode_valid callback.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/display/drm_hdmi_helper.c | 25 +++++++++++++++++++++++++
> >  include/drm/display/drm_hdmi_helper.h     |  4 ++++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index 74dd4d01dd9b..0ac5cb000ee2 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -256,3 +256,28 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> >  	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> >  }
> >  EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> > +
> > +/**
> > + * drm_hdmi_connector_mode_valid() - Check if mode is valid for HDMI connector
> > + * @connector: DRM connector to validate the mode
> > + * @mode: Display mode to validate
> > + *
> > + * Generic .mode_valid implementation for HDMI connectors.
> > + */
> > +enum drm_mode_status
> > +drm_hdmi_connector_mode_valid(const struct drm_connector *connector,
> > +			      const struct drm_display_mode *mode)
> > +{
> > +	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
> > +	unsigned long long rate;
> > +
> > +	rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
> > +	if (!rate)
> > +		return MODE_ERROR;
> > +
> > +	if (!funcs || !funcs->tmds_char_rate_valid)
> > +		return MODE_OK;
> > +
> > +	return funcs->tmds_char_rate_valid(connector, mode, rate);
> > +}
> > +EXPORT_SYMBOL(drm_hdmi_connector_mode_valid);
> 
> As discussed in the discussion that sparked that change, I believe that
> we should use hdmi_clock_valid.

Ack, I will modify the code accordingly.

> 
> AFAIU, your concern was that max_tmds_clock might get stale, but then it
> would not only prevent mode_valid from running but also the commit
> entirely.

It might be stale when parsing / validating the modes. But let's try
landing it the way you had in mind and fix the drivers which misbehave
(if any).

> 
> We don't have any evidence from that, so I'd rather try to keep
> consistency between the two. And we can always try to address whatever
> issue we might have if it turned out to be a bad idea :)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index 74dd4d01dd9b..0ac5cb000ee2 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -256,3 +256,28 @@  drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
 	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
 }
 EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
+
+/**
+ * drm_hdmi_connector_mode_valid() - Check if mode is valid for HDMI connector
+ * @connector: DRM connector to validate the mode
+ * @mode: Display mode to validate
+ *
+ * Generic .mode_valid implementation for HDMI connectors.
+ */
+enum drm_mode_status
+drm_hdmi_connector_mode_valid(const struct drm_connector *connector,
+			      const struct drm_display_mode *mode)
+{
+	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
+	unsigned long long rate;
+
+	rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
+	if (!rate)
+		return MODE_ERROR;
+
+	if (!funcs || !funcs->tmds_char_rate_valid)
+		return MODE_OK;
+
+	return funcs->tmds_char_rate_valid(connector, mode, rate);
+}
+EXPORT_SYMBOL(drm_hdmi_connector_mode_valid);
diff --git a/include/drm/display/drm_hdmi_helper.h b/include/drm/display/drm_hdmi_helper.h
index 57e3b18c15ec..e38b62df59f3 100644
--- a/include/drm/display/drm_hdmi_helper.h
+++ b/include/drm/display/drm_hdmi_helper.h
@@ -28,4 +28,8 @@  unsigned long long
 drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
 			    unsigned int bpc, enum hdmi_colorspace fmt);
 
+enum drm_mode_status
+drm_hdmi_connector_mode_valid(const struct drm_connector *connector,
+			      const struct drm_display_mode *mode);
+
 #endif