| Message ID | 20191121071205.27511-7-james.qian.wang@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/komeda: Add side_by_side support | expand |
On Thu, Nov 21, 2019 at 07:12:55AM +0000, 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) <james.qian.wang@arm.com> > --- > 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(&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; > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 21, 2019 at 10:49:26AM +0100, Daniel Vetter wrote: > On Thu, Nov 21, 2019 at 07:12:55AM +0000, 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) <james.qian.wang@arm.com> > > --- > > 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(&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; > > -- > > 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.
On Thu, 21 Nov 2019 at 20:21, james qian wang (Arm Technology China) <james.qian.wang@arm.com> wrote: > > On Thu, Nov 21, 2019 at 10:49:26AM +0100, Daniel Vetter wrote: > > On Thu, Nov 21, 2019 at 07:12:55AM +0000, 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) <james.qian.wang@arm.com> > > > --- > > > 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.
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(&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;
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) <james.qian.wang@arm.com> --- 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(-)