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

2019-12-10 Thread james qian wang (Arm Technology China) 
On Tue, Nov 26, 2019 at 01:08:05PM +0100, Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 10:54:47AM +, james qian wang (Arm Technology 
> China) wrote:
> > From: "James Qian Wang (Arm Technology China)" 
> >
> > Split sysfs config_id bitfiles to multiple separated sysfs files.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) 
> > 
>
> I guess Dave 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(_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,

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

2019-11-26 Thread Daniel Vetter 
On Tue, Nov 26, 2019 at 10:54:47AM +, james qian wang (Arm Technology 
China) wrote:
> From: "James Qian Wang (Arm Technology China)" 
> 
> Split sysfs config_id bitfiles to multiple separated sysfs files.
> 
> Signed-off-by: James Qian Wang (Arm Technology China) 
> 

I guess Dave 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_ID0x0071
>  #define MALIDP_D32_PRODUCT_ID0x0032
>  
> -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(_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",

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

2019-11-26 Thread james qian wang (Arm Technology China) 
From: "James Qian Wang (Arm Technology China)" 

Split sysfs config_id bitfiles to multiple separated sysfs files.

Signed-off-by: James Qian Wang (Arm Technology China) 
---
 .../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(_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[] = {
_attr_core_id.attr,
-   _attr_config_id.attr,
+   _attr_line_size.attr,
+   _attr_n_pipelines.attr,
+   _attr_n_layers.attr,
+   _attr_n_rich_layers.attr,
+   _attr_n_scalers.attr,
+   _attr_side_by_side.attr,
_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