diff mbox series

[v1,2/2] drm/komeda: Refactor sysfs node "config_id"

Message ID 20191126105412.5978-3-james.qian.wang@arm.com (mailing list archive)
State New, archived
Headers show
Series drm/komeda: refactor sysfs node "config_id" | expand

Commit Message

James Qian Wang Nov. 26, 2019, 10:54 a.m. UTC
From: "James Qian Wang (Arm Technology China)" <james.qian.wang@arm.com>

Split sysfs config_id bitfiles to multiple separated sysfs files.

Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
 .../drm/arm/display/include/malidp_product.h  | 13 ---
 .../gpu/drm/arm/display/komeda/komeda_sysfs.c | 80 ++++++++++++++-----
 2 files changed, 62 insertions(+), 31 deletions(-)

Comments

Daniel Vetter Nov. 26, 2019, 12:08 p.m. UTC | #1
On Tue, Nov 26, 2019 at 10:54:47AM +0000, james qian wang (Arm Technology China) wrote:
> From: "James Qian Wang (Arm Technology China)" <james.qian.wang@arm.com>
> 
> Split sysfs config_id bitfiles to multiple separated sysfs files.
> 
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

I guess Dave&my questions werent quite clear, this looks like uapi that's
consumed by hwc, so the userspace needs to be open source. Plus it needs
to be discussed/reviewed like any other kms uapi extensions, with a
critical eye whether this makes sense to add to a supposedly cross-vendor
interface.

I suspect the right thing to do here is to push the revert. From a quick
look at git history this landed together with the other kms properties in
komeda which we reverted already.
-Daniel

> ---
>  .../drm/arm/display/include/malidp_product.h  | 13 ---
>  .../gpu/drm/arm/display/komeda/komeda_sysfs.c | 80 ++++++++++++++-----
>  2 files changed, 62 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> index dbd3d4765065..b21f4aa15c95 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> @@ -21,17 +21,4 @@
>  #define MALIDP_D71_PRODUCT_ID	0x0071
>  #define MALIDP_D32_PRODUCT_ID	0x0032
>  
> -union komeda_config_id {
> -	struct {
> -		__u32	max_line_sz:16,
> -			n_pipelines:2,
> -			n_scalers:2, /* number of scalers per pipeline */
> -			n_layers:3, /* number of layers per pipeline */
> -			n_richs:3, /* number of rich layers per pipeline */
> -			side_by_side:1, /* if HW works on side_by_side mode */
> -			reserved_bits:5;
> -	};
> -	__u32 value;
> -};
> -
>  #endif /* _MALIDP_PRODUCT_H_ */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c b/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
> index 740f095b4ca5..5effab795dc1 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
> @@ -18,28 +18,67 @@ core_id_show(struct device *dev, struct device_attribute *attr, char *buf)
>  static DEVICE_ATTR_RO(core_id);
>  
>  static ssize_t
> -config_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> +line_size_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct komeda_dev *mdev = dev_to_mdev(dev);
>  	struct komeda_pipeline *pipe = mdev->pipelines[0];
> -	union komeda_config_id config_id;
> -	int i;
> -
> -	memset(&config_id, 0, sizeof(config_id));
> -
> -	config_id.max_line_sz = pipe->layers[0]->hsize_in.end;
> -	config_id.side_by_side = mdev->side_by_side;
> -	config_id.n_pipelines = mdev->n_pipelines;
> -	config_id.n_scalers = pipe->n_scalers;
> -	config_id.n_layers = pipe->n_layers;
> -	config_id.n_richs = 0;
> -	for (i = 0; i < pipe->n_layers; i++) {
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", pipe->layers[0]->hsize_in.end);
> +}
> +static DEVICE_ATTR_RO(line_size);
> +
> +static ssize_t
> +n_pipelines_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct komeda_dev *mdev = dev_to_mdev(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", mdev->n_pipelines);
> +}
> +static DEVICE_ATTR_RO(n_pipelines);
> +
> +static ssize_t
> +n_layers_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct komeda_dev *mdev = dev_to_mdev(dev);
> +	struct komeda_pipeline *pipe = mdev->pipelines[0];
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", pipe->n_layers);
> +}
> +static DEVICE_ATTR_RO(n_layers);
> +
> +static ssize_t
> +n_rich_layers_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct komeda_dev *mdev = dev_to_mdev(dev);
> +	struct komeda_pipeline *pipe = mdev->pipelines[0];
> +	int i, n_richs = 0;
> +
> +	for (i = 0; i < pipe->n_layers; i++)
>  		if (pipe->layers[i]->layer_type == KOMEDA_FMT_RICH_LAYER)
> -			config_id.n_richs++;
> -	}
> -	return snprintf(buf, PAGE_SIZE, "0x%08x\n", config_id.value);
> +			n_richs++;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", n_richs);
> +}
> +static DEVICE_ATTR_RO(n_rich_layers);
> +
> +static ssize_t
> +n_scalers_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct komeda_dev *mdev = dev_to_mdev(dev);
> +	struct komeda_pipeline *pipe = mdev->pipelines[0];
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", pipe->n_scalers);
> +}
> +static DEVICE_ATTR_RO(n_scalers);
> +
> +static ssize_t
> +side_by_side_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct komeda_dev *mdev = dev_to_mdev(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", mdev->side_by_side);
>  }
> -static DEVICE_ATTR_RO(config_id);
> +static DEVICE_ATTR_RO(side_by_side);
>  
>  static ssize_t
>  aclk_hz_show(struct device *dev, struct device_attribute *attr, char *buf)
> @@ -52,7 +91,12 @@ static DEVICE_ATTR_RO(aclk_hz);
>  
>  static struct attribute *komeda_sysfs_entries[] = {
>  	&dev_attr_core_id.attr,
> -	&dev_attr_config_id.attr,
> +	&dev_attr_line_size.attr,
> +	&dev_attr_n_pipelines.attr,
> +	&dev_attr_n_layers.attr,
> +	&dev_attr_n_rich_layers.attr,
> +	&dev_attr_n_scalers.attr,
> +	&dev_attr_side_by_side.attr,
>  	&dev_attr_aclk_hz.attr,
>  	NULL,
>  };
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
James Qian Wang Dec. 10, 2019, 10:59 a.m. UTC | #2
On Tue, Nov 26, 2019 at 01:08:05PM +0100, Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 10:54:47AM +0000, james qian wang (Arm Technology China) wrote:
> > From: "James Qian Wang (Arm Technology China)" <james.qian.wang@arm.com>
> >
> > Split sysfs config_id bitfiles to multiple separated sysfs files.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
>
> I guess Dave&my questions werent quite clear, this looks like uapi that's
> consumed by hwc, so the userspace needs to be open source. Plus it needs
> to be discussed/reviewed like any other kms uapi extensions, with a
> critical eye whether this makes sense to add to a supposedly cross-vendor
> interface.
>

Hi Dave & Daniel:

I think some komeda sysfs nodes can be added as cross-vendor.

  - core_id:
    which actually is like vendor_id/subsystem_device_id/revision in
    PCI dev, Maybe we can add a util funcs to fake these sysfs nodes
    for none PCI-dev like komeda

    device_info_sysfs_add(struct device *dev,
        u16 vendor, u16 subsystem_vendor,
        u16 subsystem_device, u8 revision);

    The arguments:
      vendor: I'd like to use the PCI vendor_ID, since with that user
              can see a unique ID, and easy to indentify different devices.
      subsystem_vendor/device/revision: device specific.

  - line_size.
    This actually is mode_config->max_width, but current drm still use
    this value to restrict the fb->width, but our HW supports crop, we
    don't have such limition. we can not set the real line_size to
    max_width.
    And I saw there is a fix:
    https://patchwork.freedesktop.org/patch/333454/
    with this fix, we can directly use mode_config->max_width

Beside that we still needs some komeda specific node like:
 - aclk_hz: for expose display engine clock.
 - num_scalers: per pipeline scalers
 - num_pipes: number of display pipelines.

For the open sourced user space, we're trying to switch to
drm_hwcomposer, and drop our internal hwcomposer. but that may need time.
Can we start from porting our specific test to IGT for komeda private uapi
coverage.

Thanks
James
> I suspect the right thing to do here is to push the revert. From a quick
> look at git history this landed together with the other kms properties in
> komeda which we reverted already.
> -Daniel
>
> > ---
> >  .../drm/arm/display/include/malidp_product.h  | 13 ---
> >  .../gpu/drm/arm/display/komeda/komeda_sysfs.c | 80 ++++++++++++++-----
> >  2 files changed, 62 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > index dbd3d4765065..b21f4aa15c95 100644
> > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > @@ -21,17 +21,4 @@
> >  #define MALIDP_D71_PRODUCT_ID      0x0071
> >  #define MALIDP_D32_PRODUCT_ID      0x0032
> >
> > -union komeda_config_id {
> > -   struct {
> > -           __u32   max_line_sz:16,
> > -                   n_pipelines:2,
> > -                   n_scalers:2, /* number of scalers per pipeline */
> > -                   n_layers:3, /* number of layers per pipeline */
> > -                   n_richs:3, /* number of rich layers per pipeline */
> > -                   side_by_side:1, /* if HW works on side_by_side mode */
> > -                   reserved_bits:5;
> > -   };
> > -   __u32 value;
> > -};
> > -
> >  #endif /* _MALIDP_PRODUCT_H_ */
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c b/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
> > index 740f095b4ca5..5effab795dc1 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
> > @@ -18,28 +18,67 @@ core_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> >  static DEVICE_ATTR_RO(core_id);
> >
> >  static ssize_t
> > -config_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +line_size_show(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> >     struct komeda_dev *mdev = dev_to_mdev(dev);
> >     struct komeda_pipeline *pipe = mdev->pipelines[0];
> > -   union komeda_config_id config_id;
> > -   int i;
> > -
> > -   memset(&config_id, 0, sizeof(config_id));
> > -
> > -   config_id.max_line_sz = pipe->layers[0]->hsize_in.end;
> > -   config_id.side_by_side = mdev->side_by_side;
> > -   config_id.n_pipelines = mdev->n_pipelines;
> > -   config_id.n_scalers = pipe->n_scalers;
> > -   config_id.n_layers = pipe->n_layers;
> > -   config_id.n_richs = 0;
> > -   for (i = 0; i < pipe->n_layers; i++) {
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", pipe->layers[0]->hsize_in.end);
> > +}
> > +static DEVICE_ATTR_RO(line_size);
> > +
> > +static ssize_t
> > +n_pipelines_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +   struct komeda_dev *mdev = dev_to_mdev(dev);
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", mdev->n_pipelines);
> > +}
> > +static DEVICE_ATTR_RO(n_pipelines);
> > +
> > +static ssize_t
> > +n_layers_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +   struct komeda_dev *mdev = dev_to_mdev(dev);
> > +   struct komeda_pipeline *pipe = mdev->pipelines[0];
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", pipe->n_layers);
> > +}
> > +static DEVICE_ATTR_RO(n_layers);
> > +
> > +static ssize_t
> > +n_rich_layers_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +   struct komeda_dev *mdev = dev_to_mdev(dev);
> > +   struct komeda_pipeline *pipe = mdev->pipelines[0];
> > +   int i, n_richs = 0;
> > +
> > +   for (i = 0; i < pipe->n_layers; i++)
> >             if (pipe->layers[i]->layer_type == KOMEDA_FMT_RICH_LAYER)
> > -                   config_id.n_richs++;
> > -   }
> > -   return snprintf(buf, PAGE_SIZE, "0x%08x\n", config_id.value);
> > +                   n_richs++;
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", n_richs);
> > +}
> > +static DEVICE_ATTR_RO(n_rich_layers);
> > +
> > +static ssize_t
> > +n_scalers_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +   struct komeda_dev *mdev = dev_to_mdev(dev);
> > +   struct komeda_pipeline *pipe = mdev->pipelines[0];
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", pipe->n_scalers);
> > +}
> > +static DEVICE_ATTR_RO(n_scalers);
> > +
> > +static ssize_t
> > +side_by_side_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +   struct komeda_dev *mdev = dev_to_mdev(dev);
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", mdev->side_by_side);
> >  }
> > -static DEVICE_ATTR_RO(config_id);
> > +static DEVICE_ATTR_RO(side_by_side);
> >
> >  static ssize_t
> >  aclk_hz_show(struct device *dev, struct device_attribute *attr, char *buf)
> > @@ -52,7 +91,12 @@ static DEVICE_ATTR_RO(aclk_hz);
> >
> >  static struct attribute *komeda_sysfs_entries[] = {
> >     &dev_attr_core_id.attr,
> > -   &dev_attr_config_id.attr,
> > +   &dev_attr_line_size.attr,
> > +   &dev_attr_n_pipelines.attr,
> > +   &dev_attr_n_layers.attr,
> > +   &dev_attr_n_rich_layers.attr,
> > +   &dev_attr_n_scalers.attr,
> > +   &dev_attr_side_by_side.attr,
> >     &dev_attr_aclk_hz.attr,
> >     NULL,
> >  };
> > --
> > 2.20.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
index dbd3d4765065..b21f4aa15c95 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_product.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
@@ -21,17 +21,4 @@ 
 #define MALIDP_D71_PRODUCT_ID	0x0071
 #define MALIDP_D32_PRODUCT_ID	0x0032
 
-union komeda_config_id {
-	struct {
-		__u32	max_line_sz:16,
-			n_pipelines:2,
-			n_scalers:2, /* number of scalers per pipeline */
-			n_layers:3, /* number of layers per pipeline */
-			n_richs:3, /* number of rich layers per pipeline */
-			side_by_side:1, /* if HW works on side_by_side mode */
-			reserved_bits:5;
-	};
-	__u32 value;
-};
-
 #endif /* _MALIDP_PRODUCT_H_ */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c b/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
index 740f095b4ca5..5effab795dc1 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_sysfs.c
@@ -18,28 +18,67 @@  core_id_show(struct device *dev, struct device_attribute *attr, char *buf)
 static DEVICE_ATTR_RO(core_id);
 
 static ssize_t
-config_id_show(struct device *dev, struct device_attribute *attr, char *buf)
+line_size_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct komeda_dev *mdev = dev_to_mdev(dev);
 	struct komeda_pipeline *pipe = mdev->pipelines[0];
-	union komeda_config_id config_id;
-	int i;
-
-	memset(&config_id, 0, sizeof(config_id));
-
-	config_id.max_line_sz = pipe->layers[0]->hsize_in.end;
-	config_id.side_by_side = mdev->side_by_side;
-	config_id.n_pipelines = mdev->n_pipelines;
-	config_id.n_scalers = pipe->n_scalers;
-	config_id.n_layers = pipe->n_layers;
-	config_id.n_richs = 0;
-	for (i = 0; i < pipe->n_layers; i++) {
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", pipe->layers[0]->hsize_in.end);
+}
+static DEVICE_ATTR_RO(line_size);
+
+static ssize_t
+n_pipelines_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct komeda_dev *mdev = dev_to_mdev(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", mdev->n_pipelines);
+}
+static DEVICE_ATTR_RO(n_pipelines);
+
+static ssize_t
+n_layers_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct komeda_dev *mdev = dev_to_mdev(dev);
+	struct komeda_pipeline *pipe = mdev->pipelines[0];
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", pipe->n_layers);
+}
+static DEVICE_ATTR_RO(n_layers);
+
+static ssize_t
+n_rich_layers_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct komeda_dev *mdev = dev_to_mdev(dev);
+	struct komeda_pipeline *pipe = mdev->pipelines[0];
+	int i, n_richs = 0;
+
+	for (i = 0; i < pipe->n_layers; i++)
 		if (pipe->layers[i]->layer_type == KOMEDA_FMT_RICH_LAYER)
-			config_id.n_richs++;
-	}
-	return snprintf(buf, PAGE_SIZE, "0x%08x\n", config_id.value);
+			n_richs++;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", n_richs);
+}
+static DEVICE_ATTR_RO(n_rich_layers);
+
+static ssize_t
+n_scalers_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct komeda_dev *mdev = dev_to_mdev(dev);
+	struct komeda_pipeline *pipe = mdev->pipelines[0];
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", pipe->n_scalers);
+}
+static DEVICE_ATTR_RO(n_scalers);
+
+static ssize_t
+side_by_side_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct komeda_dev *mdev = dev_to_mdev(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", mdev->side_by_side);
 }
-static DEVICE_ATTR_RO(config_id);
+static DEVICE_ATTR_RO(side_by_side);
 
 static ssize_t
 aclk_hz_show(struct device *dev, struct device_attribute *attr, char *buf)
@@ -52,7 +91,12 @@  static DEVICE_ATTR_RO(aclk_hz);
 
 static struct attribute *komeda_sysfs_entries[] = {
 	&dev_attr_core_id.attr,
-	&dev_attr_config_id.attr,
+	&dev_attr_line_size.attr,
+	&dev_attr_n_pipelines.attr,
+	&dev_attr_n_layers.attr,
+	&dev_attr_n_rich_layers.attr,
+	&dev_attr_n_scalers.attr,
+	&dev_attr_side_by_side.attr,
 	&dev_attr_aclk_hz.attr,
 	NULL,
 };