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(-)
--
2.27.0
drm_atomic_get_plane_state() can return errors, so we need to handle
these.
Signed-off-by: Paul Cercueil <[email protected]>
---
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) {
--
2.27.0
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 <[email protected]>
---
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 = {
--
2.27.0
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 <[email protected]>
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
> [email protected]
> 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 <[email protected]>
>
> 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
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Le mer. 29 juil. 2020 ? 0:00, [email protected] 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 <[email protected]>
>>
>> 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, [email protected] 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 <[email protected]>
> > >
> > > 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
> >
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch