[PATCH 2/4] drm: exynos: Remove dummy encoder get_crtc operation implementation

[Inki Dae]

Hi, Laurent.

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart at ideasonboard.com]
> Sent: Thursday, May 17, 2012 7:22 PM
> To: Inki Dae
> Cc: dri-devel at lists.freedesktop.org
> 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 at ideasonboard.com>
> > > ---
> > >
> > >  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