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