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;
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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
