Hi Am 14.06.22 um 14:42 schrieb Patrik Jakobsson:
On Tue, Jun 14, 2022 at 9:23 AM Thomas Zimmermann <tzimmermann@xxxxxxx> 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.jakobsson@xxxxxxxxx> --- 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?
I think it would be better to do the refactoring in a separate series. Best regards Thomas
-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
-- 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
