| Message ID | 1370123603-26750-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 01, 2013 at 11:53:23PM +0200, Daniel Vetter wrote: > Since this is run in the compute config stage we need to check > the new_ pointers, not the current modeset layout. Also there > was a little logic bug in properly skipping connectors. This has > been broken when moving the pipe bpp selection in > > commit 4e53c2e010e531b4a014692199e978482d471c7e > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Wed Mar 27 00:44:58 2013 +0100 > > drm/i915: precompute pipe bpp before touching the hw > > To avoid too much casting switch from drm_ to intel_ types. > > Also add a bit of debug output to help reconstructing what's going > on. > > v2: Try to clarify this a bit: > - s/pipe_config_set_bpp/compute_baseline_pipe_bpp/ to make it clearer > at which stage this function is run. Also add a comment about what > it does. > - Extract the sink clamping into it's own function. > > v3: Actually make it compile. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> It reads easier than the first (just now hides the bug fix ;-). I'm still worried that the confusion between connector->display_info and connector->base.display_info is far too easy to mistake. Can you describe succinctly the difference between the two, and ideally capture that in a new name for the intel_connector->display_info? -Chris
On Sun, Jun 2, 2013 at 12:09 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Jun 01, 2013 at 11:53:23PM +0200, Daniel Vetter wrote: >> Since this is run in the compute config stage we need to check >> the new_ pointers, not the current modeset layout. Also there >> was a little logic bug in properly skipping connectors. This has >> been broken when moving the pipe bpp selection in >> >> commit 4e53c2e010e531b4a014692199e978482d471c7e >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> >> Date: Wed Mar 27 00:44:58 2013 +0100 >> >> drm/i915: precompute pipe bpp before touching the hw >> >> To avoid too much casting switch from drm_ to intel_ types. >> >> Also add a bit of debug output to help reconstructing what's going >> on. >> >> v2: Try to clarify this a bit: >> - s/pipe_config_set_bpp/compute_baseline_pipe_bpp/ to make it clearer >> at which stage this function is run. Also add a comment about what >> it does. >> - Extract the sink clamping into it's own function. >> >> v3: Actually make it compile. >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > It reads easier than the first (just now hides the bug fix ;-). I'm > still worried that the confusion between connector->display_info and > connector->base.display_info is far too easy to mistake. Can you > describe succinctly the difference between the two, and ideally capture > that in a new name for the intel_connector->display_info? The newly-added base is just due to the drm_connector->intel_connector switch. So I seem to have indeed burried the actual bugfix completely :( I'll split the refactoring into a prep patch and resend. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f410ede..d4b56cb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7567,13 +7567,39 @@ static void intel_modeset_commit_output_state(struct drm_device *dev) } } +static void +connected_sink_compute_bpp(struct intel_connector * connector, + struct intel_crtc_config *pipe_config) +{ + int bpp = pipe_config->pipe_bpp; + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp constrains\n", + connector->base.base.id, + drm_get_connector_name(&connector->base)); + + /* Don't use an invalid EDID bpc value */ + if (connector->base.display_info.bpc && + connector->base.display_info.bpc * 3 < bpp) { + DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n", + bpp, connector->base.display_info.bpc*3); + pipe_config->pipe_bpp = connector->base.display_info.bpc*3; + } + + /* Clamp bpp to 8 on screens without EDID 1.4 */ + if (connector->base.display_info.bpc == 0 && bpp > 24) { + DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n", + bpp); + pipe_config->pipe_bpp = 24; + } +} + static int -pipe_config_set_bpp(struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct intel_crtc_config *pipe_config) +compute_baseline_pipe_bpp(struct intel_crtc *crtc, + struct drm_framebuffer *fb, + struct intel_crtc_config *pipe_config) { - struct drm_device *dev = crtc->dev; - struct drm_connector *connector; + struct drm_device *dev = crtc->base.dev; + struct intel_connector *connector; int bpp; switch (fb->pixel_format) { @@ -7616,24 +7642,12 @@ pipe_config_set_bpp(struct drm_crtc *crtc, /* Clamp display bpp to EDID value */ list_for_each_entry(connector, &dev->mode_config.connector_list, - head) { - if (connector->encoder && connector->encoder->crtc != crtc) + base.head) { + if (!connector->new_encoder || + connector->new_encoder->new_crtc != crtc) continue; - /* Don't use an invalid EDID bpc value */ - if (connector->display_info.bpc && - connector->display_info.bpc * 3 < bpp) { - DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n", - bpp, connector->display_info.bpc*3); - pipe_config->pipe_bpp = connector->display_info.bpc*3; - } - - /* Clamp bpp to 8 on screens without EDID 1.4 */ - if (connector->display_info.bpc == 0 && bpp > 24) { - DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n", - bpp); - pipe_config->pipe_bpp = 24; - } + connected_sink_compute_bpp(connector, pipe_config); } return bpp; @@ -7714,7 +7728,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe; pipe_config->shared_dpll = DPLL_ID_PRIVATE; - plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config); + /* Compute a starting value for pipe_config->pipe_bpp taking the source + * plane pixel format and any sink constraints into account. Returns the + * source plane bpp so that dithering can be selected on mismatches + * after encoders and crtc also have had their say. */ + plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc), + fb, pipe_config); if (plane_bpp < 0) goto fail;
Since this is run in the compute config stage we need to check the new_ pointers, not the current modeset layout. Also there was a little logic bug in properly skipping connectors. This has been broken when moving the pipe bpp selection in commit 4e53c2e010e531b4a014692199e978482d471c7e Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Wed Mar 27 00:44:58 2013 +0100 drm/i915: precompute pipe bpp before touching the hw To avoid too much casting switch from drm_ to intel_ types. Also add a bit of debug output to help reconstructing what's going on. v2: Try to clarify this a bit: - s/pipe_config_set_bpp/compute_baseline_pipe_bpp/ to make it clearer at which stage this function is run. Also add a comment about what it does. - Extract the sink clamping into it's own function. v3: Actually make it compile. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 22 deletions(-)