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

[Dylan Semler]

On Mon, Mar 25, 2013 at 9:41 AM, Jani Nikula <jani.nikula at 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 at 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;

I would naively expect this to be better.  Is there a reason to prefer if
(!mode)?

> > +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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130325/9219dc95/attachment.html>