| 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 |
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
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 --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, };