| Message ID | 1400506620-12509-1-git-send-email-thomas.wood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, May 19, 2014 at 3:37 PM, Thomas Wood <thomas.wood@intel.com> wrote: > Signed-off-by: Thomas Wood <thomas.wood@intel.com> The commit-msg lacks any discussion why this change is done. What is the reason to do that? Isn't the kernel-command-line enough? Why is this a regular feature instead of a debugfs attribute? > --- > drivers/gpu/drm/drm_sysfs.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index c22c309..257816e 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -338,11 +338,60 @@ static ssize_t select_subconnector_show(struct device *device, > drm_get_dvi_i_select_name((int)subconnector)); > } > > +static ssize_t force_show(struct device *device, > + struct device_attribute *attr, > + char *buf) > +{ > + struct drm_connector *connector = to_drm_connector(device); > + char *status; "const char *" or gcc might warn on string assignments. > + > + switch (connector->force) { > + case DRM_FORCE_ON: > + status = "on"; > + break; > + > + case DRM_FORCE_ON_DIGITAL: > + status = "digital"; > + break; > + > + case DRM_FORCE_OFF: > + status = "off"; > + break; > + > + case DRM_FORCE_UNSPECIFIED: > + status = "unspecified"; > + break; > + > + default: > + return 0; > + } > + > + return snprintf(buf, PAGE_SIZE, "%s\n", status); > +} > + > +ssize_t force_store(struct device *device, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct drm_connector *connector = to_drm_connector(device); > + > + if (strcmp(buf, "on") == 0) > + connector->force = DRM_FORCE_ON; > + else if (strcmp(buf, "digital") == 0) > + connector->force = DRM_FORCE_ON_DIGITAL; > + else if (strcmp(buf, "off") == 0) > + connector->force = DRM_FORCE_OFF; > + else if (strcmp(buf, "unspecified") == 0) > + connector->force = DRM_FORCE_UNSPECIFIED; else return -EINVAL; This really looks like a debug-feature to me. If it's a real feature, we _must_ rescan connectors here, otherwise it seems odd writing "on" into that file but nothing happens until the next modeset. Thanks David > + > + return count; > +} > + > static struct device_attribute connector_attrs[] = { > __ATTR_RO(status), > __ATTR_RO(enabled), > __ATTR_RO(dpms), > __ATTR_RO(modes), > + __ATTR_RW(force), > }; > > /* These attributes are for both DVI-I connectors and all types of tv-out. */ > -- > 1.9.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 19 May 2014 15:13, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Mon, May 19, 2014 at 3:37 PM, Thomas Wood <thomas.wood@intel.com> wrote: >> Signed-off-by: Thomas Wood <thomas.wood@intel.com> > > The commit-msg lacks any discussion why this change is done. What is > the reason to do that? Isn't the kernel-command-line enough? Why is > this a regular feature instead of a debugfs attribute? It was intended as a debug/testing feature to allow tests in intel-gpu-tools to enable or disable connectors: http://lists.freedesktop.org/archives/intel-gfx/2014-May/045556.html I'll update the commit message for the next version of the patch. > >> --- >> drivers/gpu/drm/drm_sysfs.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index c22c309..257816e 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -338,11 +338,60 @@ static ssize_t select_subconnector_show(struct device *device, >> drm_get_dvi_i_select_name((int)subconnector)); >> } >> >> +static ssize_t force_show(struct device *device, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct drm_connector *connector = to_drm_connector(device); >> + char *status; > > "const char *" or gcc might warn on string assignments. > >> + >> + switch (connector->force) { >> + case DRM_FORCE_ON: >> + status = "on"; >> + break; >> + >> + case DRM_FORCE_ON_DIGITAL: >> + status = "digital"; >> + break; >> + >> + case DRM_FORCE_OFF: >> + status = "off"; >> + break; >> + >> + case DRM_FORCE_UNSPECIFIED: >> + status = "unspecified"; >> + break; >> + >> + default: >> + return 0; >> + } >> + >> + return snprintf(buf, PAGE_SIZE, "%s\n", status); >> +} >> + >> +ssize_t force_store(struct device *device, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct drm_connector *connector = to_drm_connector(device); >> + >> + if (strcmp(buf, "on") == 0) >> + connector->force = DRM_FORCE_ON; >> + else if (strcmp(buf, "digital") == 0) >> + connector->force = DRM_FORCE_ON_DIGITAL; >> + else if (strcmp(buf, "off") == 0) >> + connector->force = DRM_FORCE_OFF; >> + else if (strcmp(buf, "unspecified") == 0) >> + connector->force = DRM_FORCE_UNSPECIFIED; > > else > return -EINVAL; > > > This really looks like a debug-feature to me. If it's a real feature, > we _must_ rescan connectors here, otherwise it seems odd writing "on" > into that file but nothing happens until the next modeset. > > Thanks > David > >> + >> + return count; >> +} >> + >> static struct device_attribute connector_attrs[] = { >> __ATTR_RO(status), >> __ATTR_RO(enabled), >> __ATTR_RO(dpms), >> __ATTR_RO(modes), >> + __ATTR_RW(force), >> }; >> >> /* These attributes are for both DVI-I connectors and all types of tv-out. */ >> -- >> 1.9.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi On Mon, May 19, 2014 at 4:41 PM, Thomas Wood <thomas.wood@intel.com> wrote: > On 19 May 2014 15:13, David Herrmann <dh.herrmann@gmail.com> wrote: >> Hi >> >> On Mon, May 19, 2014 at 3:37 PM, Thomas Wood <thomas.wood@intel.com> wrote: >>> Signed-off-by: Thomas Wood <thomas.wood@intel.com> >> >> The commit-msg lacks any discussion why this change is done. What is >> the reason to do that? Isn't the kernel-command-line enough? Why is >> this a regular feature instead of a debugfs attribute? > > > It was intended as a debug/testing feature to allow tests in > intel-gpu-tools to enable or disable connectors: > > http://lists.freedesktop.org/archives/intel-gfx/2014-May/045556.html > > > I'll update the commit message for the next version of the patch. Thanks! But please make it a debugfs feature, if possible. We shouldn't expose interfaces in sysfs that aren't part of the core API. Note that this might require you to encode the connector-name in the debugfs-attribute-name. Thanks David
On Mon, May 19, 2014 at 04:44:15PM +0200, David Herrmann wrote: > Hi > > On Mon, May 19, 2014 at 4:41 PM, Thomas Wood <thomas.wood@intel.com> wrote: > > On 19 May 2014 15:13, David Herrmann <dh.herrmann@gmail.com> wrote: > >> Hi > >> > >> On Mon, May 19, 2014 at 3:37 PM, Thomas Wood <thomas.wood@intel.com> wrote: > >>> Signed-off-by: Thomas Wood <thomas.wood@intel.com> > >> > >> The commit-msg lacks any discussion why this change is done. What is > >> the reason to do that? Isn't the kernel-command-line enough? Why is > >> this a regular feature instead of a debugfs attribute? > > > > > > It was intended as a debug/testing feature to allow tests in > > intel-gpu-tools to enable or disable connectors: > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-May/045556.html > > > > > > I'll update the commit message for the next version of the patch. > > Thanks! But please make it a debugfs feature, if possible. We > shouldn't expose interfaces in sysfs that aren't part of the core API. > Note that this might require you to encode the connector-name in the > debugfs-attribute-name. Imo having the read and write side in completely different parts doesn't make a lot of sense. Hence I think doing this in sysfs is ok. Also users might want to frob this for testing, and usually debugfs is a bit further away on most systems. -Daniel
Hi On Mon, May 19, 2014 at 4:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, May 19, 2014 at 04:44:15PM +0200, David Herrmann wrote: >> On Mon, May 19, 2014 at 4:41 PM, Thomas Wood <thomas.wood@intel.com> wrote: >> > It was intended as a debug/testing feature to allow tests in >> > intel-gpu-tools to enable or disable connectors: >> > >> > http://lists.freedesktop.org/archives/intel-gfx/2014-May/045556.html >> > >> > >> > I'll update the commit message for the next version of the patch. >> >> Thanks! But please make it a debugfs feature, if possible. We >> shouldn't expose interfaces in sysfs that aren't part of the core API. >> Note that this might require you to encode the connector-name in the >> debugfs-attribute-name. > > Imo having the read and write side in completely different parts doesn't > make a lot of sense. Hence I think doing this in sysfs is ok. Also users > might want to frob this for testing, and usually debugfs is a bit further > away on most systems. So the read-side is not debug-only? That wasn't clear to me. In that case, I'm fine with keeping it in sysfs, although I'm not entirely sure why anyone is interested in "force" information. Thanks David
On Mon, May 19, 2014 at 6:22 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > > On Mon, May 19, 2014 at 4:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, May 19, 2014 at 04:44:15PM +0200, David Herrmann wrote: >>> On Mon, May 19, 2014 at 4:41 PM, Thomas Wood <thomas.wood@intel.com> wrote: >>> > It was intended as a debug/testing feature to allow tests in >>> > intel-gpu-tools to enable or disable connectors: >>> > >>> > http://lists.freedesktop.org/archives/intel-gfx/2014-May/045556.html >>> > >>> > >>> > I'll update the commit message for the next version of the patch. >>> >>> Thanks! But please make it a debugfs feature, if possible. We >>> shouldn't expose interfaces in sysfs that aren't part of the core API. >>> Note that this might require you to encode the connector-name in the >>> debugfs-attribute-name. >> >> Imo having the read and write side in completely different parts doesn't >> make a lot of sense. Hence I think doing this in sysfs is ok. Also users >> might want to frob this for testing, and usually debugfs is a bit further >> away on most systems. > > So the read-side is not debug-only? That wasn't clear to me. In that > case, I'm fine with keeping it in sysfs, although I'm not entirely > sure why anyone is interested in "force" information. Oh, I've mixed this up with the corresponding patch to overwrite the edid, which we want to keep exposing through sysfs ofc. I guess debugfs for this is indeed fine then since it's completely new. Might be a bit of fun for lifetimes (especially with dp mst), but meh on that one - we can't make this worse really. So I agree that debugfs makes more sense. For the filename bikeshed a simpley "connector_<name>_force seems good enough. Or maybe a connector-<name> subdir if we want to dwell in overkill. -Daniel
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index c22c309..257816e 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -338,11 +338,60 @@ static ssize_t select_subconnector_show(struct device *device, drm_get_dvi_i_select_name((int)subconnector)); } +static ssize_t force_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + struct drm_connector *connector = to_drm_connector(device); + char *status; + + switch (connector->force) { + case DRM_FORCE_ON: + status = "on"; + break; + + case DRM_FORCE_ON_DIGITAL: + status = "digital"; + break; + + case DRM_FORCE_OFF: + status = "off"; + break; + + case DRM_FORCE_UNSPECIFIED: + status = "unspecified"; + break; + + default: + return 0; + } + + return snprintf(buf, PAGE_SIZE, "%s\n", status); +} + +ssize_t force_store(struct device *device, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct drm_connector *connector = to_drm_connector(device); + + if (strcmp(buf, "on") == 0) + connector->force = DRM_FORCE_ON; + else if (strcmp(buf, "digital") == 0) + connector->force = DRM_FORCE_ON_DIGITAL; + else if (strcmp(buf, "off") == 0) + connector->force = DRM_FORCE_OFF; + else if (strcmp(buf, "unspecified") == 0) + connector->force = DRM_FORCE_UNSPECIFIED; + + return count; +} + static struct device_attribute connector_attrs[] = { __ATTR_RO(status), __ATTR_RO(enabled), __ATTR_RO(dpms), __ATTR_RO(modes), + __ATTR_RW(force), }; /* These attributes are for both DVI-I connectors and all types of tv-out. */
Signed-off-by: Thomas Wood <thomas.wood@intel.com> --- drivers/gpu/drm/drm_sysfs.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)