Re: [PATCH v4 6/6] drm/komeda: Expose side_by_side by sysfs/config_id
On Thu, 21 Nov 2019 at 20:21, james qian wang (Arm Technology China)
wrote:
>
> On Thu, Nov 21, 2019 at 10:49:26AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 21, 2019 at 07:12:55AM +, james qian wang (Arm Technology
> > China) wrote:
> > > There are some restrictions if HW works on side_by_side, expose it via
> > > config_id to user.
> > >
> > > Signed-off-by: James Qian Wang (Arm Technology China)
> > >
> > > ---
> > > drivers/gpu/drm/arm/display/include/malidp_product.h | 3 ++-
> > > drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 1 +
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > index 1053b11352eb..96e2e4016250 100644
> > > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > @@ -27,7 +27,8 @@ union komeda_config_id {
> > > 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 */
> > > - reserved_bits:6;
> > > + side_by_side:1, /* if HW works on side_by_side mode */
> > > + reserved_bits:5;
> > > };
> > > __u32 value;
> > > };
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > index c3fa4835cb8d..4dd4699d4e3d 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > > @@ -83,6 +83,7 @@ config_id_show(struct device *dev, struct
> > > device_attribute *attr, char *buf)
> >
> > Uh, this sysfs file here looks a lot like uapi for some compositor to
> > decide what to do. Do you have the userspace for this?
>
> Yes, our HWC driver uses this config_id and product_id for identifying the
> HW caps.
>
This seems like it should be done more in the kernel, why does
userspace needs all that info, to make more informed decisions?
How would drm_hwcomposer get the same result?
I'd prefer we just remove the sysfs nodes from upstream unless we have
an upstream user for them.
Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 6/6] drm/komeda: Expose side_by_side by sysfs/config_id
On Thu, Nov 21, 2019 at 10:49:26AM +0100, Daniel Vetter wrote:
> On Thu, Nov 21, 2019 at 07:12:55AM +, james qian wang (Arm Technology
> China) wrote:
> > There are some restrictions if HW works on side_by_side, expose it via
> > config_id to user.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China)
> >
> > ---
> > drivers/gpu/drm/arm/display/include/malidp_product.h | 3 ++-
> > drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 1 +
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > index 1053b11352eb..96e2e4016250 100644
> > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > @@ -27,7 +27,8 @@ union komeda_config_id {
> > 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 */
> > - reserved_bits:6;
> > + side_by_side:1, /* if HW works on side_by_side mode */
> > + reserved_bits:5;
> > };
> > __u32 value;
> > };
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > index c3fa4835cb8d..4dd4699d4e3d 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > @@ -83,6 +83,7 @@ config_id_show(struct device *dev, struct
> > device_attribute *attr, char *buf)
>
> Uh, this sysfs file here looks a lot like uapi for some compositor to
> decide what to do. Do you have the userspace for this?
Yes, our HWC driver uses this config_id and product_id for identifying the
HW caps.
> Also a few more thoughts on this:
> - You can't just add more fields to uapi structs.
> - This doesn't really feel like it was ever reviewed to fit into atomic.
> - sysfs should be one value per file, not a smorgasbrod of things stuffed
> into a binary structure.
I will sent a series and split this struct to multiple files.
| This doesn't really feel like it was ever reviewed to fit into atomic
These values don't have atomic problem, since config_id is for
representing the HW caps info, which are not configurable.
Thanks
James
> -Daniel
>
> > 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;
> > --
> > 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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 6/6] drm/komeda: Expose side_by_side by sysfs/config_id
On Thu, Nov 21, 2019 at 07:12:55AM +, james qian wang (Arm Technology
China) wrote:
> There are some restrictions if HW works on side_by_side, expose it via
> config_id to user.
>
> Signed-off-by: James Qian Wang (Arm Technology China)
>
> ---
> drivers/gpu/drm/arm/display/include/malidp_product.h | 3 ++-
> drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h
> b/drivers/gpu/drm/arm/display/include/malidp_product.h
> index 1053b11352eb..96e2e4016250 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> @@ -27,7 +27,8 @@ union komeda_config_id {
> 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 */
> - reserved_bits:6;
> + side_by_side:1, /* if HW works on side_by_side mode */
> + reserved_bits:5;
> };
> __u32 value;
> };
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index c3fa4835cb8d..4dd4699d4e3d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -83,6 +83,7 @@ config_id_show(struct device *dev, struct device_attribute
> *attr, char *buf)
Uh, this sysfs file here looks a lot like uapi for some compositor to
decide what to do. Do you have the userspace for this?
Also a few more thoughts on this:
- You can't just add more fields to uapi structs.
- This doesn't really feel like it was ever reviewed to fit into atomic.
- sysfs should be one value per file, not a smorgasbrod of things stuffed
into a binary structure.
-Daniel
> 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;
> --
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 6/6] drm/komeda: Expose side_by_side by sysfs/config_id
There are some restrictions if HW works on side_by_side, expose it via
config_id to user.
Signed-off-by: James Qian Wang (Arm Technology China)
---
drivers/gpu/drm/arm/display/include/malidp_product.h | 3 ++-
drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h
b/drivers/gpu/drm/arm/display/include/malidp_product.h
index 1053b11352eb..96e2e4016250 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_product.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
@@ -27,7 +27,8 @@ union komeda_config_id {
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 */
- reserved_bits:6;
+ side_by_side:1, /* if HW works on side_by_side mode */
+ reserved_bits:5;
};
__u32 value;
};
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index c3fa4835cb8d..4dd4699d4e3d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -83,6 +83,7 @@ config_id_show(struct device *dev, struct device_attribute
*attr, char *buf)
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;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
