<div dir="ltr"><div>On Mon, Mar 25, 2013 at 9:41 AM, Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com" target="_blank">jani.nikula@linux.intel.com</a>> wrote:</div><div>></div><div>></div><div>> Hi, please find some review comments inline.</div>
<div>></div><div><br></div><div>You make some good points, thanks.</div><div><br></div><div>> On Sat, 23 Mar 2013, Dylan Semler <<a href="mailto:dylan.semler@gmail.com" target="_blank">dylan.semler@gmail.com</a>> wrote:</div>
<div>> ></div><div>> > +/* Add an explicit mode based on a quirk</div><div>> > + */</div><div>> > +static int</div><div>> > +do_force_quirk_modes(struct drm_connector *connector, int hdisplay,</div>
<div>> > + int vdisplay, int vrefresh, bool reduced)</div><div>></div><div>> Nitpick: This adds one mode, not many _modes_.</div><div><br></div><div>sure, I'll fix.</div><div><br></div><div>
<br></div><div>> > +</div><div>> > + /* loop through the probed modes and clear the preferred bit */</div><div>> > + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head)</div>
<div>> > + cur_mode->type &= ~DRM_MODE_TYPE_PREFERRED;</div><div>></div><div>> You're not deleting entries, so list_for_each_entry() would suffice,</div><div>> getting rid of the temp variable.</div>
<div>></div><div><br></div><div>I was going to use this but I wasn't sure if I understood all of the</div><div>differences between the two. I opted for _safe because I noticed it's used in</div><div>edid_fixup_preferred() even though that routine doesn't remove entries either.</div>
<div style>I'll fix it.</div>
<div><br></div><div>> > +</div><div>> > + mode = drm_cvt_mode(dev, hdisplay, vdisplay, vrefresh, reduced, 0, 0);</div><div>></div><div>> You could</div><div>></div><div>> if (!mode)</div>
<div>> return 0;</div><div>></div><div>> here and get rid of the num_modes variable.</div><div><div><br></div><div>Yes, you're right. I went with</div><div><br></div><div>if (mode) {</div><div>
...</div><div> return 1;</div><div>}</div><div>else</div><div> return 0;</div><div><br></div><div>I would naively expect this to be better. Is there a reason to prefer if</div><div>(!mode)?</div><div><br></div>
<div>> > +static int</div><div>> > +add_force_quirk_modes(struct drm_connector *connector, struct edid *edid,</div><div>> > + u32 quirks)</div><div>> > +{</div><div>> > + struct edid_quirk_force_mode *quirk_mode;</div>
<div>> > + int i, num_modes = 0;</div><div>> > +</div><div>> > + for (i = 0; i < ARRAY_SIZE(edid_quirk_force_mode_list); i++) {</div><div>> > + quirk_mode = &edid_quirk_force_mode_list[i];</div>
<div>> > +</div><div>> > + if (edid_vendor(edid, quirk_mode->vendor) &&</div><div>> > + (EDID_PRODUCT_ID(edid) == quirk_mode->product_id)) {</div><div>> > + num_modes = do_force_quirk_modes(connector,</div>
<div>> > + quirk_mode->hdisplay,</div><div>> > + quirk_mode->vdisplay,</div><div>> > + quirk_mode->vrefresh,</div>
<div>> > + quirk_mode->reduced);</div><div>></div><div>> I was wondering why you don't bail out here. Maybe you want to be able</div><div>> to add more than one quirk mode? In that case you should +=, not = the</div>
<div>> num_modes.</div><div>></div><div>> (Note that then do_force_quirk_modes makes the last one in the array the</div><div>> preferred mode, clearing the DRM_MODE_TYPE_PREFERRED from the previously</div><div>
> added quirk modes.)</div><div><br></div><div>I thought about whether multiple forced modes would ever be needed. I can't</div><div>imagine it would but it's easy enough to allow for it so we might as well</div>
<div>implement it now.</div><div><br></div></div><div><div><div>I think we'd want for it to be clear which of the forced modes will be used and</div><div>so the current behavior of making the last one in the array preferred may be</div>
<div>the best. I will add a comment by the edid_quirk_force_mode_list describing</div><div>this.</div></div><div style><br></div><div><br></div><div>> > @@ -2803,6 +2879,8 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)</div>
<div>> ></div><div>> > if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))</div><div>> > edid_fixup_preferred(connector, quirks);</div><div>> > + if (quirks & EDID_QUIRK_FORCE_MODE)</div>
<div>> > + num_modes += add_force_quirk_modes(connector, edid, quirks);</div><div>></div><div>> You don't use quirks within add_force_quirk_modes() for anything.</div><div><br></div><div>good point. I'll remove it.</div>
</div><div><br></div><div><br></div><div style>Thanks,</div><div style>Dylan</div></div>