Date:   Tue, 10 Dec 2019 10:59:06 +0000
From:   "james qian wang (Arm Technology China)" <james.qian.wang@....com>
To:     Liviu Dudau <Liviu.Dudau@....com>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        Brian Starkey <Brian.Starkey@....com>,
        Mihail Atanassov <Mihail.Atanassov@....com>, nd <nd@....com>,
        "Oscar Zhang (Arm Technology China)" <Oscar.Zhang@....com>,
        "Tiannan Zhu (Arm Technology China)" <Tiannan.Zhu@....com>,
        "Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
        "Channing Chen (Arm Technology China)" <Channing.Chen@....com>,
        "Thomas Sun (Arm Technology China)" <thomas.Sun@....com>,
        "Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
        Ben Davis <Ben.Davis@....com>
Subject: Re: [PATCH v1 2/2] drm/komeda: Refactor sysfs node "config_id"

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

Powered by blists - more mailing lists