Re: [PATCH v3 1/2] drm: Enhance EDID quirks to explicitly set a mode

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 25 Mar 2013, Dylan Semler <dylan.semler@gmail.com> wrote:
> On Mon, Mar 25, 2013 at 9:41 AM, Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
>>
>>
>> Hi, please find some review comments inline.
>>
>
> You make some good points, thanks.
>
>> On Sat, 23 Mar 2013, Dylan Semler <dylan.semler@gmail.com> 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