| Message ID | 20240222-kms-hdmi-connector-state-v7-35-8f4af575fce2@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/connector: Create HDMI Connector infrastructure | expand |
Dne četrtek, 22. februar 2024 ob 19:14:21 CET je Maxime Ripard napisal(a): > atomic_check and mode_valid do not check for the same things which can > lead to surprising result if the userspace commits a mode that didn't go > through mode_valid. Let's merge the two implementations into a function > called by both. > > Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev> > Signed-off-by: Maxime Ripard <mripard@kernel.org> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej > --- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 74 +++++++++++++++++++++------------- > 1 file changed, 47 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > index c276d984da6b..b7cf369b1906 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > @@ -62,18 +62,6 @@ static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi, > return 0; > } > > -static int sun4i_hdmi_atomic_check(struct drm_encoder *encoder, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state) > -{ > - struct drm_display_mode *mode = &crtc_state->mode; > - > - if (mode->flags & DRM_MODE_FLAG_DBLCLK) > - return -EINVAL; > - > - return 0; > -} > - > static void sun4i_hdmi_disable(struct drm_encoder *encoder, > struct drm_atomic_state *state) > { > @@ -166,31 +154,61 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder, > writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG); > } > > -static enum drm_mode_status sun4i_hdmi_mode_valid(struct drm_encoder *encoder, > - const struct drm_display_mode *mode) > +static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = { > + .atomic_disable = sun4i_hdmi_disable, > + .atomic_enable = sun4i_hdmi_enable, > +}; > + > +static enum drm_mode_status > +sun4i_hdmi_connector_clock_valid(const struct drm_connector *connector, > + const struct drm_display_mode *mode, > + unsigned long long clock) > { > - struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder); > - unsigned long rate = mode->clock * 1000; > - unsigned long diff = rate / 200; /* +-0.5% allowed by HDMI spec */ > + const struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector); > + unsigned long diff = clock / 200; /* +-0.5% allowed by HDMI spec */ > long rounded_rate; > > + if (mode->flags & DRM_MODE_FLAG_DBLCLK) > + return MODE_BAD; > + > /* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */ > - if (rate > 165000000) > + if (clock > 165000000) > return MODE_CLOCK_HIGH; > - rounded_rate = clk_round_rate(hdmi->tmds_clk, rate); > + > + rounded_rate = clk_round_rate(hdmi->tmds_clk, clock); > if (rounded_rate > 0 && > - max_t(unsigned long, rounded_rate, rate) - > - min_t(unsigned long, rounded_rate, rate) < diff) > + max_t(unsigned long, rounded_rate, clock) - > + min_t(unsigned long, rounded_rate, clock) < diff) > return MODE_OK; > + > return MODE_NOCLOCK; > } > > -static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = { > - .atomic_check = sun4i_hdmi_atomic_check, > - .atomic_disable = sun4i_hdmi_disable, > - .atomic_enable = sun4i_hdmi_enable, > - .mode_valid = sun4i_hdmi_mode_valid, > -}; > +static int sun4i_hdmi_connector_atomic_check(struct drm_connector *connector, > + struct drm_atomic_state *state) > +{ > + struct drm_connector_state *conn_state = > + drm_atomic_get_new_connector_state(state, connector); > + struct drm_crtc *crtc = conn_state->crtc; > + struct drm_crtc_state *crtc_state = crtc->state; > + struct drm_display_mode *mode = &crtc_state->adjusted_mode; > + enum drm_mode_status status; > + > + status = sun4i_hdmi_connector_clock_valid(connector, mode, > + mode->clock * 1000); > + if (status != MODE_OK) > + return -EINVAL; > + > + return 0; > +} > + > +static enum drm_mode_status > +sun4i_hdmi_connector_mode_valid(struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + return sun4i_hdmi_connector_clock_valid(connector, mode, > + mode->clock * 1000); > +} > > static int sun4i_hdmi_get_modes(struct drm_connector *connector) > { > @@ -236,6 +254,8 @@ static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev) > } > > static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = { > + .atomic_check = sun4i_hdmi_connector_atomic_check, > + .mode_valid = sun4i_hdmi_connector_mode_valid, > .get_modes = sun4i_hdmi_get_modes, > }; > > >
On Thu, 22 Feb 2024 19:14:21 +0100, Maxime Ripard wrote: > atomic_check and mode_valid do not check for the same things which can > lead to surprising result if the userspace commits a mode that didn't go > through mode_valid. Let's merge the two implementations into a function > called by both. > > Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index c276d984da6b..b7cf369b1906 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -62,18 +62,6 @@ static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi, return 0; } -static int sun4i_hdmi_atomic_check(struct drm_encoder *encoder, - struct drm_crtc_state *crtc_state, - struct drm_connector_state *conn_state) -{ - struct drm_display_mode *mode = &crtc_state->mode; - - if (mode->flags & DRM_MODE_FLAG_DBLCLK) - return -EINVAL; - - return 0; -} - static void sun4i_hdmi_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { @@ -166,31 +154,61 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder, writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG); } -static enum drm_mode_status sun4i_hdmi_mode_valid(struct drm_encoder *encoder, - const struct drm_display_mode *mode) +static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = { + .atomic_disable = sun4i_hdmi_disable, + .atomic_enable = sun4i_hdmi_enable, +}; + +static enum drm_mode_status +sun4i_hdmi_connector_clock_valid(const struct drm_connector *connector, + const struct drm_display_mode *mode, + unsigned long long clock) { - struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder); - unsigned long rate = mode->clock * 1000; - unsigned long diff = rate / 200; /* +-0.5% allowed by HDMI spec */ + const struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector); + unsigned long diff = clock / 200; /* +-0.5% allowed by HDMI spec */ long rounded_rate; + if (mode->flags & DRM_MODE_FLAG_DBLCLK) + return MODE_BAD; + /* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */ - if (rate > 165000000) + if (clock > 165000000) return MODE_CLOCK_HIGH; - rounded_rate = clk_round_rate(hdmi->tmds_clk, rate); + + rounded_rate = clk_round_rate(hdmi->tmds_clk, clock); if (rounded_rate > 0 && - max_t(unsigned long, rounded_rate, rate) - - min_t(unsigned long, rounded_rate, rate) < diff) + max_t(unsigned long, rounded_rate, clock) - + min_t(unsigned long, rounded_rate, clock) < diff) return MODE_OK; + return MODE_NOCLOCK; } -static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = { - .atomic_check = sun4i_hdmi_atomic_check, - .atomic_disable = sun4i_hdmi_disable, - .atomic_enable = sun4i_hdmi_enable, - .mode_valid = sun4i_hdmi_mode_valid, -}; +static int sun4i_hdmi_connector_atomic_check(struct drm_connector *connector, + struct drm_atomic_state *state) +{ + struct drm_connector_state *conn_state = + drm_atomic_get_new_connector_state(state, connector); + struct drm_crtc *crtc = conn_state->crtc; + struct drm_crtc_state *crtc_state = crtc->state; + struct drm_display_mode *mode = &crtc_state->adjusted_mode; + enum drm_mode_status status; + + status = sun4i_hdmi_connector_clock_valid(connector, mode, + mode->clock * 1000); + if (status != MODE_OK) + return -EINVAL; + + return 0; +} + +static enum drm_mode_status +sun4i_hdmi_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + return sun4i_hdmi_connector_clock_valid(connector, mode, + mode->clock * 1000); +} static int sun4i_hdmi_get_modes(struct drm_connector *connector) { @@ -236,6 +254,8 @@ static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev) } static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = { + .atomic_check = sun4i_hdmi_connector_atomic_check, + .mode_valid = sun4i_hdmi_connector_mode_valid, .get_modes = sun4i_hdmi_get_modes, };