Here are a few cleanups to the ingenic-drm driver. - some error paths were missing and have been added; - the mode validation has been moved to the .mode_valid helper callback.
Cheers, -Paul
Paul Cercueil (2): drm/ingenic: Handle errors of drm_atomic_get_plane_state drm/ingenic: Validate mode in a .mode_valid callback
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 +++++++++++++++-------- 1 file changed, 27 insertions(+), 14 deletions(-)
drm_atomic_get_plane_state() can return errors, so we need to handle these.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index ada990a7f911..64eabab3ef69 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -215,10 +215,17 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
if (priv->soc_info->has_osd) { f1_state = drm_atomic_get_plane_state(state->state, &priv->f1); + if (IS_ERR(f1_state)) + return PTR_ERR(f1_state); + f0_state = drm_atomic_get_plane_state(state->state, &priv->f0); + if (IS_ERR(f0_state)) + return PTR_ERR(f0_state);
if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU) && priv->ipu_plane) { ipu_state = drm_atomic_get_plane_state(state->state, priv->ipu_plane); + if (IS_ERR(ipu_state)) + return PTR_ERR(ipu_state);
/* IPU and F1 planes cannot be enabled at the same time. */ if (f1_state->fb && ipu_state->fb) {
Validate modes in the drm_crtc_helper_funcs.mode_valid() callback, which is designed for this purpose, instead of doing it in drm_crtc_helper_funcs.atomic_check().
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 34 +++++++++++++---------- 1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 64eabab3ef69..5dab9c3d0a52 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -199,21 +199,8 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc, { struct ingenic_drm *priv = drm_crtc_get_priv(crtc); struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL; - long rate; - - if (!drm_atomic_crtc_needs_modeset(state)) - return 0; - - if (state->mode.hdisplay > priv->soc_info->max_width || - state->mode.vdisplay > priv->soc_info->max_height) - return -EINVAL;
- rate = clk_round_rate(priv->pix_clk, - state->adjusted_mode.clock * 1000); - if (rate < 0) - return rate; - - if (priv->soc_info->has_osd) { + if (drm_atomic_crtc_needs_modeset(state) && priv->soc_info->has_osd) { f1_state = drm_atomic_get_plane_state(state->state, &priv->f1); if (IS_ERR(f1_state)) return PTR_ERR(f1_state); @@ -242,6 +229,24 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc, return 0; }
+static enum drm_mode_status +ingenic_drm_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) +{ + struct ingenic_drm *priv = drm_crtc_get_priv(crtc); + long rate; + + if (mode->hdisplay > priv->soc_info->max_width) + return MODE_BAD_HVALUE; + if (mode->vdisplay > priv->soc_info->max_height) + return MODE_BAD_VVALUE; + + rate = clk_round_rate(priv->pix_clk, mode->clock * 1000); + if (rate < 0) + return MODE_CLOCK_RANGE; + + return MODE_OK; +} + static void ingenic_drm_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_crtc_state *oldstate) { @@ -655,6 +660,7 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = { .atomic_begin = ingenic_drm_crtc_atomic_begin, .atomic_flush = ingenic_drm_crtc_atomic_flush, .atomic_check = ingenic_drm_crtc_atomic_check, + .mode_valid = ingenic_drm_crtc_mode_valid, };
static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
Hi Paul.
On Tue, Jul 28, 2020 at 05:16:39PM +0200, Paul Cercueil wrote:
Here are a few cleanups to the ingenic-drm driver.
- some error paths were missing and have been added;
- the mode validation has been moved to the .mode_valid helper callback.
Cheers, -Paul
Paul Cercueil (2): drm/ingenic: Handle errors of drm_atomic_get_plane_state drm/ingenic: Validate mode in a .mode_valid callback
Both looks fine, you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
I assume you will apply the patches. Maybe wait for Daniel to take a look, he had some feedback on where to add checks. I assume this is covered by the second patch.
Sam
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 +++++++++++++++-------- 1 file changed, 27 insertions(+), 14 deletions(-)
-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jul 28, 2020 at 10:17:36PM +0200, Sam Ravnborg wrote:
Hi Paul.
On Tue, Jul 28, 2020 at 05:16:39PM +0200, Paul Cercueil wrote:
Here are a few cleanups to the ingenic-drm driver.
- some error paths were missing and have been added;
- the mode validation has been moved to the .mode_valid helper callback.
Cheers, -Paul
Paul Cercueil (2): drm/ingenic: Handle errors of drm_atomic_get_plane_state drm/ingenic: Validate mode in a .mode_valid callback
Both looks fine, you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
I assume you will apply the patches. Maybe wait for Daniel to take a look, he had some feedback on where to add checks. I assume this is covered by the second patch.
Yeah changelog for new versions would be great, but aside from that bickering patch 2 lgtm now.
Cheers, Daniel
Sam
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 +++++++++++++++-------- 1 file changed, 27 insertions(+), 14 deletions(-)
-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Le mer. 29 juil. 2020 à 0:00, daniel@ffwll.ch a écrit :
On Tue, Jul 28, 2020 at 10:17:36PM +0200, Sam Ravnborg wrote:
Hi Paul.
On Tue, Jul 28, 2020 at 05:16:39PM +0200, Paul Cercueil wrote:
Here are a few cleanups to the ingenic-drm driver.
- some error paths were missing and have been added;
- the mode validation has been moved to the .mode_valid helper
callback.
Cheers, -Paul
Paul Cercueil (2): drm/ingenic: Handle errors of drm_atomic_get_plane_state drm/ingenic: Validate mode in a .mode_valid callback
Both looks fine, you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
I assume you will apply the patches. Maybe wait for Daniel to take a look, he had some feedback on where to add checks. I assume this is covered by the second patch.
Yeah changelog for new versions would be great, but aside from that bickering patch 2 lgtm now.
This patchset is V1, I'm fixing issues you saw in the ingenic-drm driver when reviewing a different patchset.
Thanks for the review, I'll apply now.
-Paul
On Wed, Jul 29, 2020 at 02:28:01AM +0200, Paul Cercueil wrote:
Le mer. 29 juil. 2020 à 0:00, daniel@ffwll.ch a écrit :
On Tue, Jul 28, 2020 at 10:17:36PM +0200, Sam Ravnborg wrote:
Hi Paul.
On Tue, Jul 28, 2020 at 05:16:39PM +0200, Paul Cercueil wrote:
Here are a few cleanups to the ingenic-drm driver.
- some error paths were missing and have been added;
- the mode validation has been moved to the .mode_valid helper
callback.
Cheers, -Paul
Paul Cercueil (2): drm/ingenic: Handle errors of drm_atomic_get_plane_state drm/ingenic: Validate mode in a .mode_valid callback
Both looks fine, you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
I assume you will apply the patches. Maybe wait for Daniel to take a look, he had some feedback on where to add checks. I assume this is covered by the second patch.
Yeah changelog for new versions would be great, but aside from that bickering patch 2 lgtm now.
This patchset is V1, I'm fixing issues you saw in the ingenic-drm driver when reviewing a different patchset.
Oh right that was pre-existing issue in which callback to use, apologies for the confusion. -Daniel
Thanks for the review, I'll apply now.
-Paul
dri-devel@lists.freedesktop.org
-
daniel＠ffwll.ch -
Paul Cercueil -
Sam Ravnborg