On Tue, Jun 14, 2022 at 9:23 AM Thomas Zimmermann <tzimmerm...@suse.de> wrote:
>
> Hi
>
> Am 13.06.22 um 14:34 schrieb Patrik Jakobsson:
> > These functions mostly do the same thing so unify them. Change a check
> > of !IS_MRST() to IS_PSB() to not change the behaviour for CDV.
> >
> > Signed-off-by: Patrik Jakobsson <patrik.r.jakobs...@gmail.com>
> > ---
> > drivers/gpu/drm/gma500/cdv_intel_lvds.c | 51 +------------------
> > drivers/gpu/drm/gma500/gma_lvds.c | 59 ++++++++++++++++++++++
> > drivers/gpu/drm/gma500/gma_lvds.h | 3 ++
> > drivers/gpu/drm/gma500/oaktrail_lvds.c | 2 +-
> > drivers/gpu/drm/gma500/psb_intel_lvds.c | 65 +------------------------
> > 5 files changed, 65 insertions(+), 115 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> > b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> > index 61dc65964e21..65532308f1e7 100644
> > --- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> > +++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> > @@ -39,55 +39,6 @@
> > #define PSB_BACKLIGHT_PWM_CTL_SHIFT (16)
> > #define PSB_BACKLIGHT_PWM_POLARITY_BIT_CLEAR (0xFFFE)
> >
> > -static bool cdv_intel_lvds_mode_fixup(struct drm_encoder *encoder,
> > - const struct drm_display_mode *mode,
> > - struct drm_display_mode *adjusted_mode)
> > -{
> > - struct drm_device *dev = encoder->dev;
> > - struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> > - struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
> > - struct drm_encoder *tmp_encoder;
> > - struct drm_display_mode *panel_fixed_mode =
> > mode_dev->panel_fixed_mode;
> > -
> > - /* Should never happen!! */
> > - list_for_each_entry(tmp_encoder, &dev->mode_config.encoder_list,
> > - head) {
> > - if (tmp_encoder != encoder
> > - && tmp_encoder->crtc == encoder->crtc) {
> > - pr_err("Can't enable LVDS and another encoder on the
> > same pipe\n");
> > - return false;
> > - }
> > - }
> > -
> > - /*
> > - * If we have timings from the BIOS for the panel, put them in
> > - * to the adjusted mode. The CRTC will be set up for this mode,
> > - * with the panel scaling set up to source from the H/VDisplay
> > - * of the original mode.
> > - */
> > - if (panel_fixed_mode != NULL) {
> > - adjusted_mode->hdisplay = panel_fixed_mode->hdisplay;
> > - adjusted_mode->hsync_start = panel_fixed_mode->hsync_start;
> > - adjusted_mode->hsync_end = panel_fixed_mode->hsync_end;
> > - adjusted_mode->htotal = panel_fixed_mode->htotal;
> > - adjusted_mode->vdisplay = panel_fixed_mode->vdisplay;
> > - adjusted_mode->vsync_start = panel_fixed_mode->vsync_start;
> > - adjusted_mode->vsync_end = panel_fixed_mode->vsync_end;
> > - adjusted_mode->vtotal = panel_fixed_mode->vtotal;
> > - adjusted_mode->clock = panel_fixed_mode->clock;
> > - drm_mode_set_crtcinfo(adjusted_mode,
> > - CRTC_INTERLACE_HALVE_V);
> > - }
> > -
> > - /*
> > - * XXX: It would be nice to support lower refresh rates on the
> > - * panels to reduce power consumption, and perhaps match the
> > - * user's requested refresh rate.
> > - */
> > -
> > - return true;
> > -}
> > -
> > static void cdv_intel_lvds_prepare(struct drm_encoder *encoder)
> > {
> > struct drm_device *dev = encoder->dev;
> > @@ -255,7 +206,7 @@ static int cdv_intel_lvds_set_property(struct
> > drm_connector *connector,
> > static const struct drm_encoder_helper_funcs
> > cdv_intel_lvds_helper_funcs = {
> > .dpms = gma_lvds_encoder_dpms,
> > - .mode_fixup = cdv_intel_lvds_mode_fixup,
> > + .mode_fixup = gma_lvds_mode_fixup,
> > .prepare = cdv_intel_lvds_prepare,
> > .mode_set = cdv_intel_lvds_mode_set,
> > .commit = cdv_intel_lvds_commit,
> > diff --git a/drivers/gpu/drm/gma500/gma_lvds.c
> > b/drivers/gpu/drm/gma500/gma_lvds.c
> > index 19679ee36062..32ecf5835828 100644
> > --- a/drivers/gpu/drm/gma500/gma_lvds.c
> > +++ b/drivers/gpu/drm/gma500/gma_lvds.c
> > @@ -209,3 +209,62 @@ void gma_lvds_restore(struct drm_connector *connector)
> > }
> > }
> >
> > +bool gma_lvds_mode_fixup(struct drm_encoder *encoder,
> > + const struct drm_display_mode *mode,
> > + struct drm_display_mode *adjusted_mode)
> > +{
> > + struct drm_device *dev = encoder->dev;
> > + struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> > + struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
> > + struct gma_crtc *gma_crtc = to_gma_crtc(encoder->crtc);
> > + struct drm_encoder *tmp_encoder;
> > + struct drm_display_mode *panel_fixed_mode =
> > mode_dev->panel_fixed_mode;
> > +
> > + /* PSB requires the LVDS is on pipe B, MRST has only one pipe anyway
> > */
> > + if (IS_PSB(dev) && gma_crtc->pipe == 0) {
> > + pr_err("Can't support LVDS on pipe A\n");
> > + return false;
> > + }
> > + if (IS_MRST(dev) && gma_crtc->pipe != 0) {
> > + pr_err("Must use PIPE A\n");
> > + return false;
> > + }
>
> In my experience, per-model branching is horrible for maintenance.
>
> I suggest to keep a _lvds_mode_fixup function for each model, each wit
> the rsp model. The rest of gma_lvds_mode_fixup() can than be a shared
> helper for all those model-specific functions.
Hi Thomas, thanks for having a look.
I wasn't sure if I wanted to refactor the code in this series or not
so I ended up just doing the unification. I fully agree that the
IS_*() checks are horrible and need fixing in most cases. And as Sam
mentioned in 1/19 there are ways to clean up the code as well.
For the above code the even better way to do it is to use the device's
lvds_mask. That way the code can be device-independent and all chips
can use it.
I can respin this series with refactoring/cleanup included or I can
send out a v2 with the initialized macro and send the
refactoring/cleanup as a separate series.
Which do you prefer?
-Patrik
>
> > + /* Should never happen!! */
> > + list_for_each_entry(tmp_encoder, &dev->mode_config.encoder_list,
> > + head) {
> > + if (tmp_encoder != encoder
> > + && tmp_encoder->crtc == encoder->crtc) {
> > + pr_err("Can't enable LVDS and another encoder on the
> > same pipe\n");
> > + return false;
> > + }
> > + }
> > +
> > + /*
> > + * If we have timings from the BIOS for the panel, put them in
> > + * to the adjusted mode. The CRTC will be set up for this mode,
> > + * with the panel scaling set up to source from the H/VDisplay
> > + * of the original mode.
> > + */
> > + if (panel_fixed_mode != NULL) {
> > + adjusted_mode->hdisplay = panel_fixed_mode->hdisplay;
> > + adjusted_mode->hsync_start = panel_fixed_mode->hsync_start;
> > + adjusted_mode->hsync_end = panel_fixed_mode->hsync_end;
> > + adjusted_mode->htotal = panel_fixed_mode->htotal;
> > + adjusted_mode->vdisplay = panel_fixed_mode->vdisplay;
> > + adjusted_mode->vsync_start = panel_fixed_mode->vsync_start;
> > + adjusted_mode->vsync_end = panel_fixed_mode->vsync_end;
> > + adjusted_mode->vtotal = panel_fixed_mode->vtotal;
> > + adjusted_mode->clock = panel_fixed_mode->clock;
> > + drm_mode_set_crtcinfo(adjusted_mode,
> > + CRTC_INTERLACE_HALVE_V);
> > + }
> > +
> > + /*
> > + * XXX: It would be nice to support lower refresh rates on the
> > + * panels to reduce power consumption, and perhaps match the
> > + * user's requested refresh rate.
> > + */
> > +
> > + return true;
> > +}
> > +
> > diff --git a/drivers/gpu/drm/gma500/gma_lvds.h
> > b/drivers/gpu/drm/gma500/gma_lvds.h
> > index eee0da00a0cf..950ab7b88ad6 100644
> > --- a/drivers/gpu/drm/gma500/gma_lvds.h
> > +++ b/drivers/gpu/drm/gma500/gma_lvds.h
> > @@ -30,5 +30,8 @@ enum drm_mode_status gma_lvds_mode_valid(struct
> > drm_connector *connector,
> > void gma_lvds_encoder_dpms(struct drm_encoder *encoder, int mode);
> > void gma_lvds_save(struct drm_connector *connector);
> > void gma_lvds_restore(struct drm_connector *connector);
> > +bool gma_lvds_mode_fixup(struct drm_encoder *encoder,
> > + const struct drm_display_mode *mode,
> > + struct drm_display_mode *adjusted_mode);
> >
> > #endif
> > diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c
> > b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> > index 00ec4fea4c12..16699b0fbbc9 100644
> > --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
> > +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> > @@ -134,7 +134,7 @@ static void oaktrail_lvds_commit(struct drm_encoder
> > *encoder)
> >
> > static const struct drm_encoder_helper_funcs oaktrail_lvds_helper_funcs =
> > {
> > .dpms = gma_lvds_encoder_dpms,
> > - .mode_fixup = psb_intel_lvds_mode_fixup,
> > + .mode_fixup = gma_lvds_mode_fixup,
> > .prepare = oaktrail_lvds_prepare,
> > .mode_set = oaktrail_lvds_mode_set,
> > .commit = oaktrail_lvds_commit,
> > diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c
> > b/drivers/gpu/drm/gma500/psb_intel_lvds.c
> > index 6e5abb670bff..317bd1fcfa2a 100644
> > --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
> > +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
> > @@ -132,69 +132,6 @@ void psb_intel_lvds_set_brightness(struct drm_device
> > *dev, int level)
> > psb_lvds_pwm_set_brightness(dev, level);
> > }
> >
> > -bool psb_intel_lvds_mode_fixup(struct drm_encoder *encoder,
> > - const struct drm_display_mode *mode,
> > - struct drm_display_mode *adjusted_mode)
> > -{
> > - struct drm_device *dev = encoder->dev;
> > - struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> > - struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
> > - struct gma_crtc *gma_crtc = to_gma_crtc(encoder->crtc);
> > - struct drm_encoder *tmp_encoder;
> > - struct drm_display_mode *panel_fixed_mode =
> > mode_dev->panel_fixed_mode;
> > - struct gma_encoder *gma_encoder = to_gma_encoder(encoder);
> > -
> > - if (gma_encoder->type == INTEL_OUTPUT_MIPI2)
> > - panel_fixed_mode = mode_dev->panel_fixed_mode2;
>
> What happened to this test? I cannot see it in the new helper.
We don't have MIPI support so I removed the test. I forgot to mention
it in this patch. Other patches should have a note about it when I
remove MIPI code.
>
> Best regards
> Thomas
>
> > -
> > - /* PSB requires the LVDS is on pipe B, MRST has only one pipe anyway
> > */
> > - if (!IS_MRST(dev) && gma_crtc->pipe == 0) {
> > - pr_err("Can't support LVDS on pipe A\n");
> > - return false;
> > - }
> > - if (IS_MRST(dev) && gma_crtc->pipe != 0) {
> > - pr_err("Must use PIPE A\n");
> > - return false;
> > - }
> > - /* Should never happen!! */
> > - list_for_each_entry(tmp_encoder, &dev->mode_config.encoder_list,
> > - head) {
> > - if (tmp_encoder != encoder
> > - && tmp_encoder->crtc == encoder->crtc) {
> > - pr_err("Can't enable LVDS and another encoder on the
> > same pipe\n");
> > - return false;
> > - }
> > - }
> > -
> > - /*
> > - * If we have timings from the BIOS for the panel, put them in
> > - * to the adjusted mode. The CRTC will be set up for this mode,
> > - * with the panel scaling set up to source from the H/VDisplay
> > - * of the original mode.
> > - */
> > - if (panel_fixed_mode != NULL) {
> > - adjusted_mode->hdisplay = panel_fixed_mode->hdisplay;
> > - adjusted_mode->hsync_start = panel_fixed_mode->hsync_start;
> > - adjusted_mode->hsync_end = panel_fixed_mode->hsync_end;
> > - adjusted_mode->htotal = panel_fixed_mode->htotal;
> > - adjusted_mode->vdisplay = panel_fixed_mode->vdisplay;
> > - adjusted_mode->vsync_start = panel_fixed_mode->vsync_start;
> > - adjusted_mode->vsync_end = panel_fixed_mode->vsync_end;
> > - adjusted_mode->vtotal = panel_fixed_mode->vtotal;
> > - adjusted_mode->clock = panel_fixed_mode->clock;
> > - drm_mode_set_crtcinfo(adjusted_mode,
> > - CRTC_INTERLACE_HALVE_V);
> > - }
> > -
> > - /*
> > - * XXX: It would be nice to support lower refresh rates on the
> > - * panels to reduce power consumption, and perhaps match the
> > - * user's requested refresh rate.
> > - */
> > -
> > - return true;
> > -}
> > -
> > static void psb_intel_lvds_prepare(struct drm_encoder *encoder)
> > {
> > struct drm_device *dev = encoder->dev;
> > @@ -365,7 +302,7 @@ int psb_intel_lvds_set_property(struct drm_connector
> > *connector,
> >
> > static const struct drm_encoder_helper_funcs psb_intel_lvds_helper_funcs
> > = {
> > .dpms = gma_lvds_encoder_dpms,
> > - .mode_fixup = psb_intel_lvds_mode_fixup,
> > + .mode_fixup = gma_lvds_mode_fixup,
> > .prepare = psb_intel_lvds_prepare,
> > .mode_set = psb_intel_lvds_mode_set,
> > .commit = psb_intel_lvds_commit,
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
