On Tue, Jun 14, 2022 at 9:23 AM Thomas Zimmermann tzimmermann@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.jakobsson@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