Hi, Laurent. > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Thursday, May 17, 2012 7:22 PM > To: Inki Dae > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/4] drm: exynos: Remove dummy encoder get_crtc > operation implementation > > Hi Inki, > > On Thursday 17 May 2012 17:21:46 Inki Dae wrote: > > On Thursday, May 17, 2012 12:09 AM Laurent Pinchart wrote: > > > > > > The encoder get_crtc operation is called to retrieve a pointer to the > > > CRTC the encoder is currenctly connected to, right after setting the > > > encoder::crtc field to the new CRTC. The implementation of this > > > operation returns the pointer to the new CRTC, which is then > pointlessly > > > compared to itself. > > > > > > As the operation is not mandatory, don't implement it. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > --- > > > > > > drivers/gpu/drm/exynos/exynos_drm_encoder.c | 7 ------- > > > 1 files changed, 0 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > > > b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > > > index 6e9ac7b..23d5ad3 100644 > > > --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > > > @@ -172,19 +172,12 @@ static void exynos_drm_encoder_commit(struct > > > drm_encoder *encoder) > > > > > > manager_ops->commit(manager->dev); > > > > > > } > > > > > > -static struct drm_crtc * > > > -exynos_drm_encoder_get_crtc(struct drm_encoder *encoder) > > > -{ > > > - return encoder->crtc; > > > -} > > > - > > > > > > static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = > { > > > > > > .dpms = exynos_drm_encoder_dpms, > > > .mode_fixup = exynos_drm_encoder_mode_fixup, > > > .mode_set = exynos_drm_encoder_mode_set, > > > .prepare = exynos_drm_encoder_prepare, > > > .commit = exynos_drm_encoder_commit, > > > > > > - .get_crtc = exynos_drm_encoder_get_crtc, > > > > > > }; > > > > We need get_crtc callback to disable encoder whose CRTC is about to > change. > > The get_crtc operation is called in a single location, in > drm_crtc_helper.c: > > /* Disable encoders whose CRTC is about to change */ > if (encoder_funcs->get_crtc && > encoder->crtc != (*encoder_funcs->get_crtc)(encoder)) > drm_encoder_disable(encoder); > > Your implementation returns encoder->crtc, so the second part of the test > is > always false. drm_encoder_disable() is never called here, the get_crtc > implementation is either unneeded or incorrect. > Ah, you're right, now get_crtc() returns always same crtc as encoder->crtc so as you pointed out, the condition is always false. I gonna test it again and apply it to exynos-drm-fixes if no problem. Thanks, Inki Dae > -- > Regards, > > Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
