* [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 @ 2012-01-18 15:06 Sean Paul 2012-05-28 17:20 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2012-01-18 15:06 UTC (permalink / raw) To: dri-devel; +Cc: marcheu, olofj, mdhayter Add a quirk which adds a new downclocked mode to the EDID of Samsung LTN121AT10-301 panels. This allows the intel driver to apply downclocking and save power. Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_edid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7425e5c..e2e3d9b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -65,6 +65,8 @@ #define EDID_QUIRK_FIRST_DETAILED_PREFERRED (1 << 5) /* use +hsync +vsync for detailed mode */ #define EDID_QUIRK_DETAILED_SYNC_PP (1 << 6) +/* the panel supports, but does not include a lower clocked mode for lvds */ +#define EDID_QUIRK_ADD_DOWNCLOCK_MODE (1 << 7) struct detailed_mode_closure { struct drm_connector *connector; @@ -119,6 +121,18 @@ static struct edid_quirk { /* Samsung SyncMaster 22[5-6]BW */ { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 }, { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 }, + + /* Samsung TFT-LCD LTN121AT10-301 */ + { "SEC", 0x3142, EDID_QUIRK_ADD_DOWNCLOCK_MODE }, +}; + +static struct downclock_rate { + char *vendor; + int product_id; + int clock; +} downclock_rate_list[] = { + /* Samsung TFT-LCD LTN121AT10-301 */ + { "SEC", 0x3142, 56428}, }; /*** DDC fetch and block validation ***/ @@ -481,6 +495,36 @@ static void edid_fixup_preferred(struct drm_connector *connector, preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; } +static int edid_add_downclock(struct drm_connector *connector, + struct edid *edid) +{ + struct drm_display_mode *t, *cur_mode, *downclock_mode; + struct downclock_rate *rate; + int i; + + for (i = 0; i < ARRAY_SIZE(downclock_rate_list); i++) { + rate = &downclock_rate_list[i]; + + if (edid_vendor(edid, rate->vendor) && + (EDID_PRODUCT_ID(edid) == rate->product_id)) + break; + } + if (i == ARRAY_SIZE(downclock_rate_list)) + return 0; + + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { + if (!(cur_mode->type & DRM_MODE_TYPE_PREFERRED)) + continue; + + downclock_mode = drm_mode_duplicate(connector->dev, cur_mode); + downclock_mode->type &= ~DRM_MODE_TYPE_PREFERRED; + downclock_mode->clock = rate->clock; + drm_mode_probed_add(connector, downclock_mode); + return 1; + } + return 0; +} + struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh) { @@ -1554,6 +1598,9 @@ 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_ADD_DOWNCLOCK_MODE) + num_modes += edid_add_downclock(connector, edid); + drm_add_display_info(edid, &connector->display_info); return num_modes; -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-01-18 15:06 [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 Sean Paul @ 2012-05-28 17:20 ` Sean Paul 2012-05-29 14:43 ` Alex Deucher 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2012-05-28 17:20 UTC (permalink / raw) To: dri-devel, airlied; +Cc: marcheu, olofj, mdhayter On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: > Add a quirk which adds a new downclocked mode to the EDID of Samsung > LTN121AT10-301 panels. This allows the intel driver to apply downclocking > and save power. > Is there any feedback on the patch? I'd like to get it merged if possible. Thanks, Sean > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_edid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 47 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 7425e5c..e2e3d9b 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -65,6 +65,8 @@ > #define EDID_QUIRK_FIRST_DETAILED_PREFERRED (1 << 5) > /* use +hsync +vsync for detailed mode */ > #define EDID_QUIRK_DETAILED_SYNC_PP (1 << 6) > +/* the panel supports, but does not include a lower clocked mode for lvds */ > +#define EDID_QUIRK_ADD_DOWNCLOCK_MODE (1 << 7) > > struct detailed_mode_closure { > struct drm_connector *connector; > @@ -119,6 +121,18 @@ static struct edid_quirk { > /* Samsung SyncMaster 22[5-6]BW */ > { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 }, > { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 }, > + > + /* Samsung TFT-LCD LTN121AT10-301 */ > + { "SEC", 0x3142, EDID_QUIRK_ADD_DOWNCLOCK_MODE }, > +}; > + > +static struct downclock_rate { > + char *vendor; > + int product_id; > + int clock; > +} downclock_rate_list[] = { > + /* Samsung TFT-LCD LTN121AT10-301 */ > + { "SEC", 0x3142, 56428}, > }; > > /*** DDC fetch and block validation ***/ > @@ -481,6 +495,36 @@ static void edid_fixup_preferred(struct drm_connector *connector, > preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; > } > > +static int edid_add_downclock(struct drm_connector *connector, > + struct edid *edid) > +{ > + struct drm_display_mode *t, *cur_mode, *downclock_mode; > + struct downclock_rate *rate; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(downclock_rate_list); i++) { > + rate = &downclock_rate_list[i]; > + > + if (edid_vendor(edid, rate->vendor) && > + (EDID_PRODUCT_ID(edid) == rate->product_id)) > + break; > + } > + if (i == ARRAY_SIZE(downclock_rate_list)) > + return 0; > + > + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { > + if (!(cur_mode->type & DRM_MODE_TYPE_PREFERRED)) > + continue; > + > + downclock_mode = drm_mode_duplicate(connector->dev, cur_mode); > + downclock_mode->type &= ~DRM_MODE_TYPE_PREFERRED; > + downclock_mode->clock = rate->clock; > + drm_mode_probed_add(connector, downclock_mode); > + return 1; > + } > + return 0; > +} > + > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, > int hsize, int vsize, int fresh) > { > @@ -1554,6 +1598,9 @@ 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_ADD_DOWNCLOCK_MODE) > + num_modes += edid_add_downclock(connector, edid); > + > drm_add_display_info(edid, &connector->display_info); > > return num_modes; > -- > 1.7.7.3 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-28 17:20 ` Sean Paul @ 2012-05-29 14:43 ` Alex Deucher 2012-05-29 20:33 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Alex Deucher @ 2012-05-29 14:43 UTC (permalink / raw) To: Sean Paul; +Cc: marcheu, olofj, dri-devel, mdhayter On Mon, May 28, 2012 at 1:20 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: >> Add a quirk which adds a new downclocked mode to the EDID of Samsung >> LTN121AT10-301 panels. This allows the intel driver to apply downclocking >> and save power. >> > > Is there any feedback on the patch? I'd like to get it merged if possible. This seems like something that should be specific to the intel driver rather than risking problems with this monitor on other drivers. Alex > > Thanks, > > Sean > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> --- >> drivers/gpu/drm/drm_edid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 47 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 7425e5c..e2e3d9b 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -65,6 +65,8 @@ >> #define EDID_QUIRK_FIRST_DETAILED_PREFERRED (1 << 5) >> /* use +hsync +vsync for detailed mode */ >> #define EDID_QUIRK_DETAILED_SYNC_PP (1 << 6) >> +/* the panel supports, but does not include a lower clocked mode for lvds */ >> +#define EDID_QUIRK_ADD_DOWNCLOCK_MODE (1 << 7) >> >> struct detailed_mode_closure { >> struct drm_connector *connector; >> @@ -119,6 +121,18 @@ static struct edid_quirk { >> /* Samsung SyncMaster 22[5-6]BW */ >> { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 }, >> { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 }, >> + >> + /* Samsung TFT-LCD LTN121AT10-301 */ >> + { "SEC", 0x3142, EDID_QUIRK_ADD_DOWNCLOCK_MODE }, >> +}; >> + >> +static struct downclock_rate { >> + char *vendor; >> + int product_id; >> + int clock; >> +} downclock_rate_list[] = { >> + /* Samsung TFT-LCD LTN121AT10-301 */ >> + { "SEC", 0x3142, 56428}, >> }; >> >> /*** DDC fetch and block validation ***/ >> @@ -481,6 +495,36 @@ static void edid_fixup_preferred(struct drm_connector *connector, >> preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; >> } >> >> +static int edid_add_downclock(struct drm_connector *connector, >> + struct edid *edid) >> +{ >> + struct drm_display_mode *t, *cur_mode, *downclock_mode; >> + struct downclock_rate *rate; >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(downclock_rate_list); i++) { >> + rate = &downclock_rate_list[i]; >> + >> + if (edid_vendor(edid, rate->vendor) && >> + (EDID_PRODUCT_ID(edid) == rate->product_id)) >> + break; >> + } >> + if (i == ARRAY_SIZE(downclock_rate_list)) >> + return 0; >> + >> + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { >> + if (!(cur_mode->type & DRM_MODE_TYPE_PREFERRED)) >> + continue; >> + >> + downclock_mode = drm_mode_duplicate(connector->dev, cur_mode); >> + downclock_mode->type &= ~DRM_MODE_TYPE_PREFERRED; >> + downclock_mode->clock = rate->clock; >> + drm_mode_probed_add(connector, downclock_mode); >> + return 1; >> + } >> + return 0; >> +} >> + >> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, >> int hsize, int vsize, int fresh) >> { >> @@ -1554,6 +1598,9 @@ 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_ADD_DOWNCLOCK_MODE) >> + num_modes += edid_add_downclock(connector, edid); >> + >> drm_add_display_info(edid, &connector->display_info); >> >> return num_modes; >> -- >> 1.7.7.3 >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-29 14:43 ` Alex Deucher @ 2012-05-29 20:33 ` Sean Paul 2012-05-29 21:23 ` Alex Deucher 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2012-05-29 20:33 UTC (permalink / raw) To: Alex Deucher; +Cc: marcheu, olofj, dri-devel, mdhayter On Tue, May 29, 2012 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote: > On Mon, May 28, 2012 at 1:20 PM, Sean Paul <seanpaul@chromium.org> wrote: >> On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: >>> Add a quirk which adds a new downclocked mode to the EDID of Samsung >>> LTN121AT10-301 panels. This allows the intel driver to apply downclocking >>> and save power. >>> >> >> Is there any feedback on the patch? I'd like to get it merged if possible. > > This seems like something that should be specific to the intel driver > rather than risking problems with this monitor on other drivers. > Thanks for the feedback, Alex. I had originally put this in the intel driver, but moved it since it was a nasty hack. Furthermore, it was less obvious that the mode was available, since it was hidden to all layers above i915. The panel is just fine with the lower refresh rate, so it shouldn't have any issues with other drivers. Sean > Alex > >> >> Thanks, >> >> Sean >> >>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>> --- >>> drivers/gpu/drm/drm_edid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 47 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>> index 7425e5c..e2e3d9b 100644 >>> --- a/drivers/gpu/drm/drm_edid.c >>> +++ b/drivers/gpu/drm/drm_edid.c >>> @@ -65,6 +65,8 @@ >>> #define EDID_QUIRK_FIRST_DETAILED_PREFERRED (1 << 5) >>> /* use +hsync +vsync for detailed mode */ >>> #define EDID_QUIRK_DETAILED_SYNC_PP (1 << 6) >>> +/* the panel supports, but does not include a lower clocked mode for lvds */ >>> +#define EDID_QUIRK_ADD_DOWNCLOCK_MODE (1 << 7) >>> >>> struct detailed_mode_closure { >>> struct drm_connector *connector; >>> @@ -119,6 +121,18 @@ static struct edid_quirk { >>> /* Samsung SyncMaster 22[5-6]BW */ >>> { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 }, >>> { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 }, >>> + >>> + /* Samsung TFT-LCD LTN121AT10-301 */ >>> + { "SEC", 0x3142, EDID_QUIRK_ADD_DOWNCLOCK_MODE }, >>> +}; >>> + >>> +static struct downclock_rate { >>> + char *vendor; >>> + int product_id; >>> + int clock; >>> +} downclock_rate_list[] = { >>> + /* Samsung TFT-LCD LTN121AT10-301 */ >>> + { "SEC", 0x3142, 56428}, >>> }; >>> >>> /*** DDC fetch and block validation ***/ >>> @@ -481,6 +495,36 @@ static void edid_fixup_preferred(struct drm_connector *connector, >>> preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; >>> } >>> >>> +static int edid_add_downclock(struct drm_connector *connector, >>> + struct edid *edid) >>> +{ >>> + struct drm_display_mode *t, *cur_mode, *downclock_mode; >>> + struct downclock_rate *rate; >>> + int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(downclock_rate_list); i++) { >>> + rate = &downclock_rate_list[i]; >>> + >>> + if (edid_vendor(edid, rate->vendor) && >>> + (EDID_PRODUCT_ID(edid) == rate->product_id)) >>> + break; >>> + } >>> + if (i == ARRAY_SIZE(downclock_rate_list)) >>> + return 0; >>> + >>> + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { >>> + if (!(cur_mode->type & DRM_MODE_TYPE_PREFERRED)) >>> + continue; >>> + >>> + downclock_mode = drm_mode_duplicate(connector->dev, cur_mode); >>> + downclock_mode->type &= ~DRM_MODE_TYPE_PREFERRED; >>> + downclock_mode->clock = rate->clock; >>> + drm_mode_probed_add(connector, downclock_mode); >>> + return 1; >>> + } >>> + return 0; >>> +} >>> + >>> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, >>> int hsize, int vsize, int fresh) >>> { >>> @@ -1554,6 +1598,9 @@ 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_ADD_DOWNCLOCK_MODE) >>> + num_modes += edid_add_downclock(connector, edid); >>> + >>> drm_add_display_info(edid, &connector->display_info); >>> >>> return num_modes; >>> -- >>> 1.7.7.3 >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-29 20:33 ` Sean Paul @ 2012-05-29 21:23 ` Alex Deucher 2012-05-30 0:38 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Alex Deucher @ 2012-05-29 21:23 UTC (permalink / raw) To: Sean Paul; +Cc: marcheu, olofj, dri-devel, mdhayter On Tue, May 29, 2012 at 4:33 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, May 29, 2012 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote: >> On Mon, May 28, 2012 at 1:20 PM, Sean Paul <seanpaul@chromium.org> wrote: >>> On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: >>>> Add a quirk which adds a new downclocked mode to the EDID of Samsung >>>> LTN121AT10-301 panels. This allows the intel driver to apply downclocking >>>> and save power. >>>> >>> >>> Is there any feedback on the patch? I'd like to get it merged if possible. >> >> This seems like something that should be specific to the intel driver >> rather than risking problems with this monitor on other drivers. >> > > Thanks for the feedback, Alex. I had originally put this in the intel > driver, but moved it since it was a nasty hack. Furthermore, it was > less obvious that the mode was available, since it was hidden to all > layers above i915. The panel is just fine with the lower refresh rate, > so it shouldn't have any issues with other drivers. Well it doesn't really seem like a quirk per se, the monitor works just fine without it. You are basically just adding an arbitrary new user defined mode that happens to work on the monitor. Seems like you should just be adding the mode with xrandr or equivalent. Alex > > Sean > > >> Alex >> >>> >>> Thanks, >>> >>> Sean >>> >>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>> --- >>>> drivers/gpu/drm/drm_edid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 files changed, 47 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>>> index 7425e5c..e2e3d9b 100644 >>>> --- a/drivers/gpu/drm/drm_edid.c >>>> +++ b/drivers/gpu/drm/drm_edid.c >>>> @@ -65,6 +65,8 @@ >>>> #define EDID_QUIRK_FIRST_DETAILED_PREFERRED (1 << 5) >>>> /* use +hsync +vsync for detailed mode */ >>>> #define EDID_QUIRK_DETAILED_SYNC_PP (1 << 6) >>>> +/* the panel supports, but does not include a lower clocked mode for lvds */ >>>> +#define EDID_QUIRK_ADD_DOWNCLOCK_MODE (1 << 7) >>>> >>>> struct detailed_mode_closure { >>>> struct drm_connector *connector; >>>> @@ -119,6 +121,18 @@ static struct edid_quirk { >>>> /* Samsung SyncMaster 22[5-6]BW */ >>>> { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 }, >>>> { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 }, >>>> + >>>> + /* Samsung TFT-LCD LTN121AT10-301 */ >>>> + { "SEC", 0x3142, EDID_QUIRK_ADD_DOWNCLOCK_MODE }, >>>> +}; >>>> + >>>> +static struct downclock_rate { >>>> + char *vendor; >>>> + int product_id; >>>> + int clock; >>>> +} downclock_rate_list[] = { >>>> + /* Samsung TFT-LCD LTN121AT10-301 */ >>>> + { "SEC", 0x3142, 56428}, >>>> }; >>>> >>>> /*** DDC fetch and block validation ***/ >>>> @@ -481,6 +495,36 @@ static void edid_fixup_preferred(struct drm_connector *connector, >>>> preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; >>>> } >>>> >>>> +static int edid_add_downclock(struct drm_connector *connector, >>>> + struct edid *edid) >>>> +{ >>>> + struct drm_display_mode *t, *cur_mode, *downclock_mode; >>>> + struct downclock_rate *rate; >>>> + int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(downclock_rate_list); i++) { >>>> + rate = &downclock_rate_list[i]; >>>> + >>>> + if (edid_vendor(edid, rate->vendor) && >>>> + (EDID_PRODUCT_ID(edid) == rate->product_id)) >>>> + break; >>>> + } >>>> + if (i == ARRAY_SIZE(downclock_rate_list)) >>>> + return 0; >>>> + >>>> + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { >>>> + if (!(cur_mode->type & DRM_MODE_TYPE_PREFERRED)) >>>> + continue; >>>> + >>>> + downclock_mode = drm_mode_duplicate(connector->dev, cur_mode); >>>> + downclock_mode->type &= ~DRM_MODE_TYPE_PREFERRED; >>>> + downclock_mode->clock = rate->clock; >>>> + drm_mode_probed_add(connector, downclock_mode); >>>> + return 1; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, >>>> int hsize, int vsize, int fresh) >>>> { >>>> @@ -1554,6 +1598,9 @@ 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_ADD_DOWNCLOCK_MODE) >>>> + num_modes += edid_add_downclock(connector, edid); >>>> + >>>> drm_add_display_info(edid, &connector->display_info); >>>> >>>> return num_modes; >>>> -- >>>> 1.7.7.3 >>>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-29 21:23 ` Alex Deucher @ 2012-05-30 0:38 ` Sean Paul 2012-05-30 5:06 ` Rafał Miłecki 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2012-05-30 0:38 UTC (permalink / raw) To: Alex Deucher; +Cc: marcheu, olofj, dri-devel, mdhayter On Tue, May 29, 2012 at 5:23 PM, Alex Deucher <alexdeucher@gmail.com> wrote: > On Tue, May 29, 2012 at 4:33 PM, Sean Paul <seanpaul@chromium.org> wrote: >> On Tue, May 29, 2012 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote: >>> On Mon, May 28, 2012 at 1:20 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>> On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: >>>>> Add a quirk which adds a new downclocked mode to the EDID of Samsung >>>>> LTN121AT10-301 panels. This allows the intel driver to apply downclocking >>>>> and save power. >>>>> >>>> >>>> Is there any feedback on the patch? I'd like to get it merged if possible. >>> >>> This seems like something that should be specific to the intel driver >>> rather than risking problems with this monitor on other drivers. >>> >> >> Thanks for the feedback, Alex. I had originally put this in the intel >> driver, but moved it since it was a nasty hack. Furthermore, it was >> less obvious that the mode was available, since it was hidden to all >> layers above i915. The panel is just fine with the lower refresh rate, >> so it shouldn't have any issues with other drivers. > > Well it doesn't really seem like a quirk per se, the monitor works > just fine without it. You are basically just adding an arbitrary new > user defined mode that happens to work on the monitor. Seems like you > should just be adding the mode with xrandr or equivalent. > Unfortunately, adding it via xrandr won't take advantage of the i915 lvds downclocking feature, which is the motivation for the patch. At any rate, it doesn't look like there's any interest in picking this patch up, so we'll just carry it locally in chromium-os :( Sean > Alex > >> >> Sean >> >> >>> Alex >>> >>>> >>>> Thanks, >>>> >>>> Sean >>>> >>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>> --- >>>>> drivers/gpu/drm/drm_edid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 files changed, 47 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>>>> index 7425e5c..e2e3d9b 100644 >>>>> --- a/drivers/gpu/drm/drm_edid.c >>>>> +++ b/drivers/gpu/drm/drm_edid.c >>>>> @@ -65,6 +65,8 @@ >>>>> #define EDID_QUIRK_FIRST_DETAILED_PREFERRED (1 << 5) >>>>> /* use +hsync +vsync for detailed mode */ >>>>> #define EDID_QUIRK_DETAILED_SYNC_PP (1 << 6) >>>>> +/* the panel supports, but does not include a lower clocked mode for lvds */ >>>>> +#define EDID_QUIRK_ADD_DOWNCLOCK_MODE (1 << 7) >>>>> >>>>> struct detailed_mode_closure { >>>>> struct drm_connector *connector; >>>>> @@ -119,6 +121,18 @@ static struct edid_quirk { >>>>> /* Samsung SyncMaster 22[5-6]BW */ >>>>> { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 }, >>>>> { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 }, >>>>> + >>>>> + /* Samsung TFT-LCD LTN121AT10-301 */ >>>>> + { "SEC", 0x3142, EDID_QUIRK_ADD_DOWNCLOCK_MODE }, >>>>> +}; >>>>> + >>>>> +static struct downclock_rate { >>>>> + char *vendor; >>>>> + int product_id; >>>>> + int clock; >>>>> +} downclock_rate_list[] = { >>>>> + /* Samsung TFT-LCD LTN121AT10-301 */ >>>>> + { "SEC", 0x3142, 56428}, >>>>> }; >>>>> >>>>> /*** DDC fetch and block validation ***/ >>>>> @@ -481,6 +495,36 @@ static void edid_fixup_preferred(struct drm_connector *connector, >>>>> preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; >>>>> } >>>>> >>>>> +static int edid_add_downclock(struct drm_connector *connector, >>>>> + struct edid *edid) >>>>> +{ >>>>> + struct drm_display_mode *t, *cur_mode, *downclock_mode; >>>>> + struct downclock_rate *rate; >>>>> + int i; >>>>> + >>>>> + for (i = 0; i < ARRAY_SIZE(downclock_rate_list); i++) { >>>>> + rate = &downclock_rate_list[i]; >>>>> + >>>>> + if (edid_vendor(edid, rate->vendor) && >>>>> + (EDID_PRODUCT_ID(edid) == rate->product_id)) >>>>> + break; >>>>> + } >>>>> + if (i == ARRAY_SIZE(downclock_rate_list)) >>>>> + return 0; >>>>> + >>>>> + list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { >>>>> + if (!(cur_mode->type & DRM_MODE_TYPE_PREFERRED)) >>>>> + continue; >>>>> + >>>>> + downclock_mode = drm_mode_duplicate(connector->dev, cur_mode); >>>>> + downclock_mode->type &= ~DRM_MODE_TYPE_PREFERRED; >>>>> + downclock_mode->clock = rate->clock; >>>>> + drm_mode_probed_add(connector, downclock_mode); >>>>> + return 1; >>>>> + } >>>>> + return 0; >>>>> +} >>>>> + >>>>> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, >>>>> int hsize, int vsize, int fresh) >>>>> { >>>>> @@ -1554,6 +1598,9 @@ 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_ADD_DOWNCLOCK_MODE) >>>>> + num_modes += edid_add_downclock(connector, edid); >>>>> + >>>>> drm_add_display_info(edid, &connector->display_info); >>>>> >>>>> return num_modes; >>>>> -- >>>>> 1.7.7.3 >>>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-30 0:38 ` Sean Paul @ 2012-05-30 5:06 ` Rafał Miłecki 2012-05-30 12:05 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2012-05-30 5:06 UTC (permalink / raw) To: Sean Paul; +Cc: marcheu, olofj, dri-devel, mdhayter 2012/5/30 Sean Paul <seanpaul@chromium.org>: > On Tue, May 29, 2012 at 5:23 PM, Alex Deucher <alexdeucher@gmail.com> wrote: >> On Tue, May 29, 2012 at 4:33 PM, Sean Paul <seanpaul@chromium.org> wrote: >>> On Tue, May 29, 2012 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote: >>>> On Mon, May 28, 2012 at 1:20 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>>> On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: >>>>>> Add a quirk which adds a new downclocked mode to the EDID of Samsung >>>>>> LTN121AT10-301 panels. This allows the intel driver to apply downclocking >>>>>> and save power. >>>>>> >>>>> >>>>> Is there any feedback on the patch? I'd like to get it merged if possible. >>>> >>>> This seems like something that should be specific to the intel driver >>>> rather than risking problems with this monitor on other drivers. >>>> >>> >>> Thanks for the feedback, Alex. I had originally put this in the intel >>> driver, but moved it since it was a nasty hack. Furthermore, it was >>> less obvious that the mode was available, since it was hidden to all >>> layers above i915. The panel is just fine with the lower refresh rate, >>> so it shouldn't have any issues with other drivers. >> >> Well it doesn't really seem like a quirk per se, the monitor works >> just fine without it. You are basically just adding an arbitrary new >> user defined mode that happens to work on the monitor. Seems like you >> should just be adding the mode with xrandr or equivalent. >> > > Unfortunately, adding it via xrandr won't take advantage of the i915 > lvds downclocking feature, which is the motivation for the patch. > > At any rate, it doesn't look like there's any interest in picking this > patch up, so we'll just carry it locally in chromium-os :( May I ask why? AFAIU you're just copying PREFERRED mode and changing it's clock. xrandr allows you to set custom clock, doesn't it? -- Rafał _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-30 5:06 ` Rafał Miłecki @ 2012-05-30 12:05 ` Sean Paul 2012-05-30 14:16 ` Adam Jackson 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2012-05-30 12:05 UTC (permalink / raw) To: Rafał Miłecki; +Cc: marcheu, olofj, dri-devel, mdhayter On Wed, May 30, 2012 at 1:06 AM, Rafał Miłecki <zajec5@gmail.com> wrote: > 2012/5/30 Sean Paul <seanpaul@chromium.org>: >> On Tue, May 29, 2012 at 5:23 PM, Alex Deucher <alexdeucher@gmail.com> wrote: >>> On Tue, May 29, 2012 at 4:33 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>> On Tue, May 29, 2012 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote: >>>>> On Mon, May 28, 2012 at 1:20 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>>>> On Wed, Jan 18, 2012 at 10:06 AM, Sean Paul <seanpaul@chromium.org> wrote: >>>>>>> Add a quirk which adds a new downclocked mode to the EDID of Samsung >>>>>>> LTN121AT10-301 panels. This allows the intel driver to apply downclocking >>>>>>> and save power. >>>>>>> >>>>>> >>>>>> Is there any feedback on the patch? I'd like to get it merged if possible. >>>>> >>>>> This seems like something that should be specific to the intel driver >>>>> rather than risking problems with this monitor on other drivers. >>>>> >>>> >>>> Thanks for the feedback, Alex. I had originally put this in the intel >>>> driver, but moved it since it was a nasty hack. Furthermore, it was >>>> less obvious that the mode was available, since it was hidden to all >>>> layers above i915. The panel is just fine with the lower refresh rate, >>>> so it shouldn't have any issues with other drivers. >>> >>> Well it doesn't really seem like a quirk per se, the monitor works >>> just fine without it. You are basically just adding an arbitrary new >>> user defined mode that happens to work on the monitor. Seems like you >>> should just be adding the mode with xrandr or equivalent. >>> >> >> Unfortunately, adding it via xrandr won't take advantage of the i915 >> lvds downclocking feature, which is the motivation for the patch. >> >> At any rate, it doesn't look like there's any interest in picking this >> patch up, so we'll just carry it locally in chromium-os :( > > May I ask why? AFAIU you're just copying PREFERRED mode and changing > it's clock. xrandr allows you to set custom clock, doesn't it? > Yes, definitely. The reason I can't set it via xrandr (easily) is because we look for lvds downclock modes (in i915) on the driver init. Since the driver initializes way before we have a chance to add a new mode via xrandr, the driver won't have a downclock mode. I suppose the other option is to hack the i915 driver to allow a downclocked mode to be added after it's been initialized. I haven't looked into this solution, it might be worth investigating. Sean > -- > Rafał _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-30 12:05 ` Sean Paul @ 2012-05-30 14:16 ` Adam Jackson 2012-05-30 15:21 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Adam Jackson @ 2012-05-30 14:16 UTC (permalink / raw) To: Sean Paul; +Cc: marcheu, olofj, dri-devel, mdhayter On 5/30/12 8:05 AM, Sean Paul wrote: > Yes, definitely. The reason I can't set it via xrandr (easily) is > because we look for lvds downclock modes (in i915) on the driver init. > Since the driver initializes way before we have a chance to add a new > mode via xrandr, the driver won't have a downclock mode. > > I suppose the other option is to hack the i915 driver to allow a > downclocked mode to be added after it's been initialized. I haven't > looked into this solution, it might be worth investigating. Just so I'm clear, is what you're looking for "I want this pair of timings, with the driver magically switching between them"? If all you wanted was the lower clock speed all the time you could just, you know, do that, so I assume that's not what you're after. If binding two timings together like that is what you want, then that seems like a pretty reasonable device-specific ioctl at first glance. I think the only thing to be careful of would be copying the slower timings into the driver private of the faster, rather than keeping a pointer or copy of the object id, since modes aren't refcounted. How big of a power savings do you see with this? Wondering if it's worth trying to make some common tooling for finding downclocked modes, if it's going to be worthwhile on multiple panels. - ajax ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-30 14:16 ` Adam Jackson @ 2012-05-30 15:21 ` Sean Paul 2012-05-31 7:09 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2012-05-30 15:21 UTC (permalink / raw) To: Adam Jackson; +Cc: marcheu, olofj, dri-devel, mdhayter On Wed, May 30, 2012 at 10:16 AM, Adam Jackson <ajax@redhat.com> wrote: > On 5/30/12 8:05 AM, Sean Paul wrote: > >> Yes, definitely. The reason I can't set it via xrandr (easily) is >> because we look for lvds downclock modes (in i915) on the driver init. >> Since the driver initializes way before we have a chance to add a new >> mode via xrandr, the driver won't have a downclock mode. >> >> I suppose the other option is to hack the i915 driver to allow a >> downclocked mode to be added after it's been initialized. I haven't >> looked into this solution, it might be worth investigating. > > > Just so I'm clear, is what you're looking for "I want this pair of timings, > with the driver magically switching between them"? If all you wanted was > the lower clock speed all the time you could just, you know, do that, so I > assume that's not what you're after. > Yep, you got it. > If binding two timings together like that is what you want, then that seems > like a pretty reasonable device-specific ioctl at first glance. I think the > only thing to be careful of would be copying the slower timings into the > driver private of the faster, rather than keeping a pointer or copy of the > object id, since modes aren't refcounted. > > How big of a power savings do you see with this? Wondering if it's worth > trying to make some common tooling for finding downclocked modes, if it's > going to be worthwhile on multiple panels. > I saw about 200mW savings with downclocking enabled. It's hard to say how many panels this might apply to, some panels which report downclocked states still flicker, and others that don't have it in the EDID work just fine (like this Samsung panel). LVDS downclocking is enabled by default in Fedora and ChromiumOS, so making this more accessible might be useful for users of these distros. Sean > - ajax ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-30 15:21 ` Sean Paul @ 2012-05-31 7:09 ` Daniel Vetter 2012-05-31 13:24 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2012-05-31 7:09 UTC (permalink / raw) To: Sean Paul; +Cc: marcheu, olofj, dri-devel, mdhayter On Wed, May 30, 2012 at 11:21:32AM -0400, Sean Paul wrote: > On Wed, May 30, 2012 at 10:16 AM, Adam Jackson <ajax@redhat.com> wrote: > > On 5/30/12 8:05 AM, Sean Paul wrote: > > > >> Yes, definitely. The reason I can't set it via xrandr (easily) is > >> because we look for lvds downclock modes (in i915) on the driver init. > >> Since the driver initializes way before we have a chance to add a new > >> mode via xrandr, the driver won't have a downclock mode. > >> > >> I suppose the other option is to hack the i915 driver to allow a > >> downclocked mode to be added after it's been initialized. I haven't > >> looked into this solution, it might be worth investigating. > > > > > > Just so I'm clear, is what you're looking for "I want this pair of timings, > > with the driver magically switching between them"? If all you wanted was > > the lower clock speed all the time you could just, you know, do that, so I > > assume that's not what you're after. > > > > Yep, you got it. > > > If binding two timings together like that is what you want, then that seems > > like a pretty reasonable device-specific ioctl at first glance. I think the > > only thing to be careful of would be copying the slower timings into the > > driver private of the faster, rather than keeping a pointer or copy of the > > object id, since modes aren't refcounted. > > > > How big of a power savings do you see with this? Wondering if it's worth > > trying to make some common tooling for finding downclocked modes, if it's > > going to be worthwhile on multiple panels. > > > > I saw about 200mW savings with downclocking enabled. It's hard to say > how many panels this might apply to, some panels which report > downclocked states still flicker, and others that don't have it in the > EDID work just fine (like this Samsung panel). > > LVDS downclocking is enabled by default in Fedora and ChromiumOS, so > making this more accessible might be useful for users of these > distros. I think adding lvds connector properties for downclocking would make sense. I guess we need and enable knob and a frequency value. At boot time we'd fill these with the detected values, but then userspace would have full control (i.e. the enable knob should overwrite the i915 module options). That way downclocking would also be much easier to test (and maybe we could try to enable it by default eventually). -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 2012-05-31 7:09 ` Daniel Vetter @ 2012-05-31 13:24 ` Sean Paul 0 siblings, 0 replies; 12+ messages in thread From: Sean Paul @ 2012-05-31 13:24 UTC (permalink / raw) To: Daniel Vetter; +Cc: marcheu, olofj, dri-devel, mdhayter On Thu, May 31, 2012 at 3:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, May 30, 2012 at 11:21:32AM -0400, Sean Paul wrote: >> On Wed, May 30, 2012 at 10:16 AM, Adam Jackson <ajax@redhat.com> wrote: >> > On 5/30/12 8:05 AM, Sean Paul wrote: >> > >> >> Yes, definitely. The reason I can't set it via xrandr (easily) is >> >> because we look for lvds downclock modes (in i915) on the driver init. >> >> Since the driver initializes way before we have a chance to add a new >> >> mode via xrandr, the driver won't have a downclock mode. >> >> >> >> I suppose the other option is to hack the i915 driver to allow a >> >> downclocked mode to be added after it's been initialized. I haven't >> >> looked into this solution, it might be worth investigating. >> > >> > >> > Just so I'm clear, is what you're looking for "I want this pair of timings, >> > with the driver magically switching between them"? If all you wanted was >> > the lower clock speed all the time you could just, you know, do that, so I >> > assume that's not what you're after. >> > >> >> Yep, you got it. >> >> > If binding two timings together like that is what you want, then that seems >> > like a pretty reasonable device-specific ioctl at first glance. I think the >> > only thing to be careful of would be copying the slower timings into the >> > driver private of the faster, rather than keeping a pointer or copy of the >> > object id, since modes aren't refcounted. >> > >> > How big of a power savings do you see with this? Wondering if it's worth >> > trying to make some common tooling for finding downclocked modes, if it's >> > going to be worthwhile on multiple panels. >> > >> >> I saw about 200mW savings with downclocking enabled. It's hard to say >> how many panels this might apply to, some panels which report >> downclocked states still flicker, and others that don't have it in the >> EDID work just fine (like this Samsung panel). >> >> LVDS downclocking is enabled by default in Fedora and ChromiumOS, so >> making this more accessible might be useful for users of these >> distros. > > I think adding lvds connector properties for downclocking would make > sense. I guess we need and enable knob and a frequency value. At boot time > we'd fill these with the detected values, but then userspace would have > full control (i.e. the enable knob should overwrite the i915 module > options). That way downclocking would also be much easier to test (and > maybe we could try to enable it by default eventually). Excellent, thanks to everyone for the feedback! I'll put together a patch when I get some free cycles. Sean > -Daniel > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-05-31 13:24 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-18 15:06 [PATCH] drm: Add downclock quirk for Samsung LTN121AT10-301 Sean Paul 2012-05-28 17:20 ` Sean Paul 2012-05-29 14:43 ` Alex Deucher 2012-05-29 20:33 ` Sean Paul 2012-05-29 21:23 ` Alex Deucher 2012-05-30 0:38 ` Sean Paul 2012-05-30 5:06 ` Rafał Miłecki 2012-05-30 12:05 ` Sean Paul 2012-05-30 14:16 ` Adam Jackson 2012-05-30 15:21 ` Sean Paul 2012-05-31 7:09 ` Daniel Vetter 2012-05-31 13:24 ` Sean Paul
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.