[PATCH] drm/sysfs: expose the "force" connector attribute

[PATCH] drm/sysfs: expose the "force" connector attribute

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 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;
+
+	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. */
-- 
1.9.0

Re: [PATCH] drm/sysfs: expose the "force" connector attribute

[David Herrmann]

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

Re: [PATCH] drm/sysfs: expose the "force" connector attribute

[Thomas Wood]

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

Re: [PATCH] drm/sysfs: expose the "force" connector attribute

[David Herrmann]

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

Re: [PATCH] drm/sysfs: expose the "force" connector attribute

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/sysfs: expose the "force" connector attribute

[David Herrmann]

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

Re: [PATCH] drm/sysfs: expose the "force" connector attribute

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch