Re: [PATCH v4 6/6] drm/komeda: Expose side_by_side by sysfs/config_id

[james qian wang (Arm Technology China)]

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@xxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxxxxxxx
> > 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.