Hi Adrien,just some coding-style remarks, and thanks for improving the Intel graphics driver!
Am 25.08.2010 17:06, schrieb pulkomandy@xxxxxxxxxxxxxxxxx:
Author: pulkomandy Date: 2010-08-25 17:06:49 +0200 (Wed, 25 Aug 2010) New Revision: 38354 Changeset: http://dev.haiku-os.org/changeset/38354
[...]
Modified: haiku/trunk/src/add-ons/accelerants/intel_extreme/mode.cpp =================================================================== --- haiku/trunk/src/add-ons/accelerants/intel_extreme/mode.cpp 2010-08-25 14:42:55 UTC (rev 38353) +++ haiku/trunk/src/add-ons/accelerants/intel_extreme/mode.cpp 2010-08-25 15:06:49 UTC (rev 38354) @@ -607,10 +607,17 @@ display_mode target = *mode; if (intel_propose_display_mode(&target, mode, mode)) return B_BAD_VALUE; + // TODO : it may be acceptable to continue when using panel fitting, + // since the data from propose_display_mode will not actually get any + // use
The if block should get parenthesis now, since the comment makes it a multi-line if-block.
uint32 colorMode, bytesPerRow, bitsPerPixel; get_color_space_format(target, colorMode, bytesPerRow, bitsPerPixel); + // TODO : do not go further if the mode is identical to the current one. + // This would avoid the screen being off when switching workspaces when they + // have the same resolution. + #if 0 static bool first = true; if (first) { @@ -642,7 +649,8 @@ uint32 base; if (intel_allocate_memory(bytesPerRow * target.virtual_height, 0, base)< B_OK) { - // oh, how did that happen? Unfortunately, there is no really good way back + // oh, how did that happen? Unfortunately, there is no really good way + // back if (intel_allocate_memory(sharedInfo.current_mode.virtual_height * sharedInfo.bytes_per_row, 0, base) == B_OK) { sharedInfo.frame_buffer = base; @@ -665,13 +673,49 @@ read32(INTEL_VGA_DISPLAY_CONTROL); if ((gInfo->head_mode& HEAD_MODE_B_DIGITAL) != 0) { + // For LVDS panels, we actuallyalways set the native mode in hardware + // Then we use the panel fitter to scale the picture to that. + display_mode hardware_target; + bool needs_scaling = false;
The correct variable names would be "hardwareTarget" and "needsScaling".
+ if (gInfo->has_edid) { + hardware_target.space = target.space; + hardware_target.virtual_width + = gInfo->edid_info.std_timing[0].h_size; + hardware_target.virtual_height + = gInfo->edid_info.std_timing[0].v_size; + // TODO : use the info from EDID 1.2 + /* Each detailed_monitor has a type, find the first one which is + * actually a detailed_timing + hardware_target.virtual_width = gInfo->edid_info + .detailed_monitor[0].data.detailed_timing.h_visible; + hardware_target.virtual_height = gInfo->edid_info + .detailed_monitor[0].data.detailed_timing.v_visible; + */
Why the mix of different comment styles?
+ TRACE(("intel_extreme : hardware mode will actually be %dx%d\n", + hardware_target.virtual_width, hardware_target.virtual_height)); + if ((hardware_target.virtual_width<= target.virtual_width + && hardware_target.virtual_height<= target.virtual_height + && hardware_target.space<= target.space) + || intel_propose_display_mode(&hardware_target, mode, mode)) { + hardware_target = target; + } else + needs_scaling = true; + } else { + // we don't have EDID data, try to set the requested mode directly + hardware_target = target; + } + pll_divisors divisors; - compute_pll_divisors(target, divisors, true); + if (needs_scaling) + compute_pll_divisors(hardware_target, divisors, true); + else + compute_pll_divisors(target, divisors, true); uint32 dpll = DISPLAY_PLL_NO_VGA_CONTROL | DISPLAY_PLL_ENABLED; if (gInfo->shared_info->device_type.InFamily(INTEL_TYPE_9xx)) { - dpll |= LVDS_PLL_MODE_LVDS; - // DPLL mode LVDS for i915+ + dpll |= LVDS_PLL_MODE_LVDS; + // DPLL mode LVDS for i915+ } // compute bitmask from p1 value @@ -683,7 +727,8 @@ break; } - write32(INTEL_PANEL_FIT_CONTROL, 0); + // Disable panel fitting, but enable 8 to 6-bit dithering
The sentence casing in comments is preferred (I think), but you do it inconsistently, you start many other comments that read like sentences lowercase (nitpicking, I know).
Best regards, -Stephan