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

[Daniel Vetter]

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@xxxxxxx>
> 
> Split sysfs config_id bitfiles to multiple separated sysfs files.
> 
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@xxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch