Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Convert ct buffer to iosys_map
On 09/03/22 07:08, Lucas De Marchi wrote:
> On Tue, Mar 08, 2022 at 03:08:05PM +0530, Mullati Siva wrote:
>> From: Siva Mullati
>>
>> Convert CT commands and descriptors to use iosys_map rather
>> than plain pointer and save it in the intel_guc_ct_buffer struct.
>> This will help with ct_write and ct_read for cmd send and receive
>> after the initialization by abstracting the IO vs system memory.
>>
>> Signed-off-by: Siva Mullati
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 170 +-
>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 9 +-
>> 2 files changed, 110 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> index f01325cd1b62..457deca1c25a 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> @@ -44,6 +44,11 @@ static inline struct drm_device *ct_to_drm(struct
>> intel_guc_ct *ct)
>> #define CT_PROBE_ERROR(_ct, _fmt, ...) \
>> i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
>>
>> +#define ct_desc_read(desc_map_, field_) \
>> + iosys_map_rd_field(desc_map_, 0, struct guc_ct_buffer_desc, field_)
>
> one thing I found useful in intel_guc_ads, was to use the first argument
> as the struct type instead of map. That's because then I enforce the
> right type to be passed rather than a random iosys_map. See :
>
> #define ads_blob_read(guc_, field_) \
> iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_)
>
> First arg is expected to be `struct intel_guc *`. Yes, I didn't do this
> for info_map_{read,write}, because there were situation in which I had
> to pass a info from outside (forcefully from system memory). If you
> don't have such case, I think it would be better to pass the typed
> pointer.
>
understood, will change it as a "typed pointer".
>> +#define ct_desc_write(desc_map_, field_, val_) \
>> + iosys_map_wr_field(desc_map_, 0, struct guc_ct_buffer_desc, field_,
>> val_)
>> +
>> /**
>> * DOC: CTB Blob
>> *
>> @@ -113,9 +118,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>> init_waitqueue_head(>wq);
>> }
>>
>> -static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc)
>> +static void guc_ct_buffer_desc_init(struct iosys_map *desc)
>
> if you can apply the comment above, then leave it as
> struct intel_guc_ct_buffer.
>
yes
>> {
>> - memset(desc, 0, sizeof(*desc));
>> + iosys_map_memset(desc, 0, 0, sizeof(struct guc_ct_buffer_desc));
>> }
>>
>> static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb)
>> @@ -128,17 +133,24 @@ static void guc_ct_buffer_reset(struct
>> intel_guc_ct_buffer *ctb)
>> space = CIRC_SPACE(ctb->tail, ctb->head, ctb->size) - ctb->resv_space;
>> atomic_set(>space, space);
>>
>> - guc_ct_buffer_desc_init(ctb->desc);
>> + guc_ct_buffer_desc_init(>desc_map);
>> }
>>
>> static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,
>> - struct guc_ct_buffer_desc *desc,
>> - u32 *cmds, u32 size_in_bytes, u32 resv_space)
>> + void *desc, void *cmds, u32 size_in_bytes,
>> + u32 resv_space, bool lmem)
>
> bool arguments are really hard to read, because you always have to
> lookup the function prototype to understand what that is about.
>
Tried to avoid bool but could not find a better alternative code path.
Please suggest, if you have something.
> Here we could turn struct guc_ct_buffer_desc *desc into the map, and let
> the caller prepare it for iomem or system memory.
>
Agreed
>> {
>> GEM_BUG_ON(size_in_bytes % 4);
>>
>> - ctb->desc = desc;
>> - ctb->cmds = cmds;
>> + if (lmem) {
>> + iosys_map_set_vaddr_iomem(>desc_map,
>> + (void __iomem *)desc);
>> + iosys_map_set_vaddr_iomem(>cmds_map,
>> + (void __iomem *)cmds);
>> + } else {
>> + iosys_map_set_vaddr(>desc_map, desc);
>> + iosys_map_set_vaddr(>cmds_map, cmds);
>> + }
>> ctb->size = size_in_bytes / 4;
>> ctb->resv_space = resv_space / 4;
>>
>> @@ -218,13 +230,12 @@ static int ct_register_buffer(struct intel_guc_ct *ct,
>> bool send,
>> int intel_guc_ct_init(struct intel_guc_ct *ct)
>> {
>> struct intel_guc *guc = ct_to_guc(ct);
>> - struct guc_ct_buffer_desc *desc;
>> u32 blob_size;
>> u32 cmds_size;
>> u32 resv_space;
>> - void *blob;
>> - u32 *cmds;
>> + void *blob, *desc, *cmds;
>> int err;
>> + bool lmem;
>>
>> err = i915_inject_probe_error(guc_to_gt(guc)->i915, -ENXIO);
>> if (err)
>> @@ -242,6 +253,8 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
>>
>> CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma),
>> blob_size);
>>
>> + lmem = i915_gem_object_is_lmem(ct->vma->obj);
>> +
>> /* store pointers to desc and cmds for send ctb */
>> desc = blob;
>> cmds
Re: [Intel-gfx] [PATCH 1/2] iosys-map: Add a helper for pointer difference
On 09/03/22 06:43, Lucas De Marchi wrote:
> On Tue, Mar 08, 2022 at 03:08:04PM +0530, Mullati Siva wrote:
>> From: Siva Mullati
>>
>> iosys_map_ptrdiff to get the difference in address of
>> same memory type.
>>
>> Signed-off-by: Siva Mullati
>> ---
>> include/linux/iosys-map.h | 21 +
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>> index e69a002d5aa4..8987f69ec1e9 100644
>> --- a/include/linux/iosys-map.h
>> +++ b/include/linux/iosys-map.h
>> @@ -8,6 +8,7 @@
>>
>> #include
>> #include
>> +#include
>>
>> /**
>> * DOC: overview
>> @@ -208,6 +209,26 @@ static inline bool iosys_map_is_equal(const struct
>> iosys_map *lhs,
>> return lhs->vaddr == rhs->vaddr;
>> }
>>
>> +/**
>> + * iosys_map_ptrdiff - Difference of two iosys mapping addresses of same
>> memory type
>> + * @lhs: The iosys_map structure
>> + * @rhs: A iosys_map structure to compare with
>> + *
>> + * Two iosys mapping structures of same memory type with the differences
>> + * in address within that memory.
>> + *
>> + * Returns:
>> + * Address difference of two memory locations with same memory type.
>> + */
>> +static inline ptrdiff_t iosys_map_ptrdiff(const struct iosys_map *lhs,
>> + const struct iosys_map *rhs)
>> +{
>> + if (lhs->is_iomem)
>
> the other interfaces always check both arguments to make sure they are
> both iomem. So if we want this interface we will want to check like
> that.
>
Agreed, will use the other interface.
> I'm not sure this is really needed, but will depend on the secon patch,
> let's wait to settle this discussion there.
>
Since you have already covered the other patch, intel_guc_ct_enable() where
this API has been used for calculating offset for cmds and desc. Do you this as
good approach or is okay to directly access the ctb.send.cmds_map.vaddr?
>
> thanks
> Lucas De Marchi
Re: [Intel-gfx] [PATCH] drm/i915/pmu: Drop redundant IS_VALLEYVIEW check in __get_rc6()
On 12-03-2022 01:30, Ashutosh Dixit wrote: Because VLV_GT_RENDER_RC6 == GEN6_GT_GFX_RC6, the IS_VALLEYVIEW() check is not needed. Neither is the check present in other code paths which call intel_rc6_residency_ns() (in functions gen6_drpc(), rc6_residency() and rc6_residency_ms_show()). Signed-off-by: Ashutosh Dixit --- drivers/gpu/drm/i915/i915_pmu.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index cfc21042499d..3e3b09588fd3 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -148,10 +148,7 @@ static u64 __get_rc6(struct intel_gt *gt) struct drm_i915_private *i915 = gt->i915; u64 val; - val = intel_rc6_residency_ns(>rc6, -IS_VALLEYVIEW(i915) ? -VLV_GT_RENDER_RC6 : -GEN6_GT_GFX_RC6); + val = intel_rc6_residency_ns(>rc6, GEN6_GT_GFX_RC6); This change looks fine. Reviewed-by: Badal Nilawar if (HAS_RC6p(i915)) val += intel_rc6_residency_ns(>rc6, GEN6_GT_GFX_RC6p);
Re: [Intel-gfx] [PATCH v5 7/7] drm/i915/gt: Adding new sysfs frequency attributes
On 3/13/2022 5:38 PM, Andi Shyti wrote:
Hi Michal,
[...]
+static ssize_t punit_req_freq_mhz_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buff)
+{
+ struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
+ struct intel_rps *rps = >rps;
+ u32 preq = intel_rps_read_punit_req_frequency(rps);
+
+ return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
%u since preq is u32
and use sysfs_emit (also in below show functions)
sure! I'll change them.
[...]
static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj,
const struct attribute * const *attrs)
{
@@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct
kobject *kobj)
if (ret)
drm_warn(>i915->drm,
"failed to create gt%u RPS sysfs files", gt->info.id);
+
+ ret = sysfs_create_files(kobj, freq_attrs);
+ if (ret)
+ drm_warn(>i915->drm,
+"failed to create gt%u throttle sysfs files",
+gt->info.id);
nit: would be nice to see %pe why it failed
[...]
I will add it to the other cases as well.
+static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32)
this doesn't look like "rps" helper, rather like "gt" so it should have
different prefix and maybe even be exported by the gt or uncore ?
unless you wanted:
static u32 __rps_read_mmio(struct intel_rps *rps, i915_reg_t reg32)
{
struct intel_gt *gt = rps_to_gt(rps);
+{
+ intel_wakeref_t wakeref;
+ u32 val;
+
+ with_intel_runtime_pm(gt->uncore->rpm, wakeref)
+ val = intel_uncore_read(gt->uncore, reg32);
+
+ return val;
+}
Yes, you are right!
@Sujaritha: shall I move "__rps_read_mmio()" in intel_gt.c and
call it intel_gt_read_mmio()?
[...]
Sure since it is kind of a gt helper, makes sense to have gt prefix.
+u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps)
+{
+ struct intel_gt *gt = rps_to_gt(rps);
+ u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) &
VR_THERMALERT_MASK;
+
+ return thermalert;
+}
shouldn't we return bool by all of these functions as used/expected in
show() counterparts ?
Suja?
[...]
Yes we can make this bool as well.
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8)
+#define GT0_PERF_LIMIT_REASONS_MASK 0x0de3
this mask is different that other (FIELD_PREP/GET wont work) so maybe we
should name it in special way ?
As far as I understood this is still a mask and used as such.
This mask is actually telling that there is some throttling going
on.
It looks weird because there are some unwanted bits in between
the interesting bits.
+#define PROCHOT_MASK BIT(1)
+#define THERMAL_LIMIT_MASK BIT(2)
+#define RATL_MASKBIT(6)
+#define VR_THERMALERT_MASK BIT(7)
+#define VR_TDC_MASK BIT(8)
+#define POWER_LIMIT_4_MASK BIT(9)
+#define POWER_LIMIT_1_MASK BIT(11)
+#define POWER_LIMIT_2_MASK BIT(12)
REG_BIT ?
yes!
Thanks, Michal!
Andi
Re: [Intel-gfx] [PATCH v5 7/7] drm/i915/gt: Adding new sysfs frequency attributes
Hi Michal,
[...]
> > +static ssize_t punit_req_freq_mhz_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buff)
> > +{
> > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > + struct intel_rps *rps = >rps;
> > + u32 preq = intel_rps_read_punit_req_frequency(rps);
> > +
> > + return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
>
> %u since preq is u32
>
> and use sysfs_emit (also in below show functions)
sure! I'll change them.
[...]
> > static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj,
> > const struct attribute * const *attrs)
> > {
> > @@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt,
> > struct kobject *kobj)
> > if (ret)
> > drm_warn(>i915->drm,
> > "failed to create gt%u RPS sysfs files", gt->info.id);
> > +
> > + ret = sysfs_create_files(kobj, freq_attrs);
> > + if (ret)
> > + drm_warn(>i915->drm,
> > +"failed to create gt%u throttle sysfs files",
> > +gt->info.id);
>
> nit: would be nice to see %pe why it failed
[...]
I will add it to the other cases as well.
> > +static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32)
>
> this doesn't look like "rps" helper, rather like "gt" so it should have
> different prefix and maybe even be exported by the gt or uncore ?
>
> unless you wanted:
>
> static u32 __rps_read_mmio(struct intel_rps *rps, i915_reg_t reg32)
> {
> struct intel_gt *gt = rps_to_gt(rps);
>
> > +{
> > + intel_wakeref_t wakeref;
> > + u32 val;
> > +
> > + with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> > + val = intel_uncore_read(gt->uncore, reg32);
> > +
> > + return val;
> > +}
Yes, you are right!
@Sujaritha: shall I move "__rps_read_mmio()" in intel_gt.c and
call it intel_gt_read_mmio()?
[...]
> > +u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps)
> > +{
> > + struct intel_gt *gt = rps_to_gt(rps);
> > + u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) &
> > VR_THERMALERT_MASK;
> > +
> > + return thermalert;
> > +}
>
> shouldn't we return bool by all of these functions as used/expected in
> show() counterparts ?
Suja?
[...]
> > +#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8)
> > +#define GT0_PERF_LIMIT_REASONS_MASK 0x0de3
>
> this mask is different that other (FIELD_PREP/GET wont work) so maybe we
> should name it in special way ?
As far as I understood this is still a mask and used as such.
This mask is actually telling that there is some throttling going
on.
It looks weird because there are some unwanted bits in between
the interesting bits.
> > +#define PROCHOT_MASK BIT(1)
> > +#define THERMAL_LIMIT_MASK BIT(2)
> > +#define RATL_MASKBIT(6)
> > +#define VR_THERMALERT_MASK BIT(7)
> > +#define VR_TDC_MASK BIT(8)
> > +#define POWER_LIMIT_4_MASK BIT(9)
> > +#define POWER_LIMIT_1_MASK BIT(11)
> > +#define POWER_LIMIT_2_MASK BIT(12)
>
> REG_BIT ?
yes!
Thanks, Michal!
Andi
Re: [Intel-gfx] [PATCH v5 6/7] drm/i915/gt: Create per-tile RPS sysfs interfaces
Hi Andrzej,
[...]
> > +static ssize_t act_freq_mhz_show(struct device *dev,
> > +struct device_attribute *attr, char *buff)
> > +{
> > + s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr,
> > + __act_freq_mhz_show);
> > +
> > + return sysfs_emit(buff, "%u\n", (u32) actual_freq);
>
> Again, variable can be just u32.
yes
[...]
> > +static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val)
> > +{
> > + struct intel_rps *rps = >rps;
> > + bool boost = false;
> > +
> > + /* Validate against (static) hardware limits */
> > + val = intel_freq_opcode(rps, val);
> > + if (val < rps->min_freq || val > rps->max_freq)
> > + return -EINVAL;
> > +
> > + mutex_lock(>lock);
> > + if (val != rps->boost_freq) {
> > + rps->boost_freq = val;
> > + boost = atomic_read(>num_waiters);
> > + }
> > + mutex_unlock(>lock);
> > + if (boost)
> > + schedule_work(>work);
> > +
> > + return 0;
>
> Why not intel_rps_set_boost_frequency?
> Why copy/paste from rps_set_boost_freq?
you are right... this must be some ugly leftover. Thanks!
[...]
> > +/* For now we have a static number of RP states */
> > +static ssize_t rps_rp_mhz_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buff)
> > +{
> > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > + struct intel_rps *rps = >rps;
> > + u32 val;
> > +
> > + if (attr == _attr_gt_RP0_freq_mhz ||
> > + attr == _attr_rps_RP0_freq_mhz) {
> > + val = intel_rps_get_rp0_frequency(rps);
> > + } else if (attr == _attr_gt_RP1_freq_mhz ||
> > + attr == _attr_rps_RP1_freq_mhz) {
> > + val = intel_rps_get_rp1_frequency(rps);
> > + } else if (attr == _attr_gt_RPn_freq_mhz ||
> > + attr == _attr_rps_RPn_freq_mhz) {
> > + val = intel_rps_get_rpn_frequency(rps);
> > + } else {
> > + GEM_WARN_ON(1);
> > + return -ENODEV;
>
> I guess this is not necessary, otherwise similar path should be in other
> helpers.
yeah... it's a bit hacky, I must agree... will split it properly.
[...]
Thanks,
Andi
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Andrzej,
> > Now tiles have their own sysfs interfaces under the gt/
> > directory. Because RC6 is a property that can be configured on a
> > tile basis, then each tile should have its own interface
> >
> > The new sysfs structure will have a similar layout for the 4 tile
> > case:
> >
> > /sys/.../card0
> > ├── gt
> > │ ├── gt0
> > │ │ ├── id
> > │ │ ├── rc6_enable
> > │ │ ├── rc6_residency_ms
> > . . .
> > . . .
> > . .
> > │ └── gtN
> > │ ├── id
> > │ ├── rc6_enable
> > │ ├── rc6_residency_ms
> > │ .
> > │ .
> > │
> > └── power/-+
> >├── rc6_enable|Original interface
> >├── rc6_residency_ms +-> kept as existing ABI;
> >. |it multiplexes over
> >. |the GTs
> > -+
> >
> > The existing interfaces have been kept in their original location
> > to preserve the existing ABI. They act on all the GTs: when
> > reading they provide the average value from all the GTs.
>
> If ABI should be kept forever, depreciation msg should be removed from
> intel_gt_sysfs_get_drvdata.
yes... to be removed!
> > +#ifdef CONFIG_PM
> > +static s64
> > +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute
> > *attr,
> > + s64 (func)(struct intel_gt *gt))
> > +{
> > + struct intel_gt *gt;
> > + s64 ret = 0;
> > +
> > + if (!is_object_gt(>kobj)) {
> > + int i, num_gt = 0;
> > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > +
> > + for_each_gt(gt, i915, i) {
> > + ret += func(gt);
> > + num_gt++;
> > + }
> > +
> > + ret /= num_gt;
> > + } else {
> > + gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > + ret = func(gt);
> > + }
> > +
> > + return ret;
> > +}
>
> Return value is always converted to u32 or int, maybe we could use just int
> ?
there is one case when the value to be returned is an 'int' and
that is the "int intel_gpu_freq()". That's why I supposed that
s64 was the right size. But I don't see how it can be negative so
that I will make it u32. Perhaps we need to change intel_gpu_freq
to be u32.
(Didn't want to take it too far, but I was also thinking that in
the future we might need to return error values)
> > +static ssize_t rc6_residency_ms_show(struct device *dev,
> > +struct device_attribute *attr,
> > +char *buff)
> > +{
> > + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
> > + __rc6_residency_ms_show);
> > +
> > + return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
>
> Here and below (where applicable) variable should be just u32, no need to
> conversion in sysfs_emit.
yep! same comment as above.
> > +static int __intel_gt_sysfs_create_group(struct kobject *kobj,
> > +const struct attribute_group *grp)
> > +{
> > + return is_object_gt(kobj) ?
> > + sysfs_create_group(kobj, [0]) :
> > + sysfs_merge_group(kobj, [1]);
> > +}
>
> Merging handling "gt/gt#/*" and "power/*" attributes into the same helpers
> seems unnatural to me, in many functions we have two branches based on value
> of is_object_gt, with the most hacky intel_gt_sysfs_get_drvdata.
> Splitting handling these attributes would allow to drop fragile is_object_gt
> helper and make functions more straightforward, probably at the cost of few
> lines more. On the other side I am not sure if it is worth to change
> everything to just address code purity concerns :)
I the amount of duplicated code would be too high and there have
been already complaints some times in the past (e.g. we have had
same discussion in the case of the debugfs files).
But it's true that is quite hard to find the correct balance
between duplicated code and compact code.
> So with variable type adjustement:
> Reviewed-by: Andrzej Hajda
Thanks!
Re: [Intel-gfx] [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface
> > > > > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> > > > > + const char *name)
> > > > > +{
> > > > > + struct kobject *kobj = >kobj;
> > > > > +
> > > > > + /*
> > > > > + * We are interested at knowing from where the interface
> > > > > + * has been called, whether it's called from gt/ or from
> > > > > + * the parent directory.
> > > > > + * From the interface position it depends also the value of
> > > > > + * the private data.
> > > > > + * If the interface is called from gt/ then private data is
> > > > > + * of the "struct intel_gt *" type, otherwise it's * a
> > > > > + * "struct drm_i915_private *" type.
> > > > > + */
> > > > > + if (!is_object_gt(kobj)) {
> > > > > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > > > > +
> > > > > + pr_devel_ratelimited(DEPRECATED
> > > > > + "%s (pid %d) is accessing deprecated %s "
> > > > > + "sysfs control, please use gt/gt/%s
> > > > > instead\n",
> > > > > + current->comm, task_pid_nr(current), name,
> > > > > name);
> > > > > + return to_gt(i915);
> > > > > + }
> > > > > +
> > > > > + return kobj_to_gt(kobj);
> > > > It took some time for me to understand what is going on here.
> > > > We have dev argument which sometimes can point to "struct device",
> > > > sometimes
> > > > to "struct kobj_gt", but it's type suggests differently, quite ugly.
> > > > I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as
> > > > in
> > > > case of intel_engines_add_sysfs. This way abstractions would look
> > > > better,
> > > > hopefully.
> > > How would it help?
> > >
> > > The difference is that I'm adding twice different interfaces with
> > > the same name and different location (i.e. different object). The
> > > legacy intrefaces inherit the object from drm and I'm preserving
> > > that reference.
> > >
> > > While the new objects would derive from the previous and they are
> > > pretty much like intel_engines_add_sysfs().
> >
> > I was not clear on the issue. Here in case of 'id' attribute it is defined
> > as device_attribute, but in kobj_type.sysfs_ops you assign formally
> > incompatible _sysfs_ops.
>
> 'kobj_sysfs_ops' is of the type 'kobj_type'.
>
> > kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary
> > compatible' with device_attribute and kobj is at beginning of struct device
> > as well, so it does not blow up, but I wouldn't say it is clean solution :)
> > If you look at intel_engines_add_sysfs you can see that all attributes are
> > defined as kobj_attribute.
>
> That's exactly the approach I use in the next patches for the
> power management files, I use "struct kobj_gt" wrapped around
> "struct kobject". But I'm using that only for the GT files.
>
> Are you, btw, suggesting to use this same approache also for the
> legacy files that for now have a pointer to the drm kobject? This
> way I would need to add more information, like the pointer to
> i915 and gt_id. This way I wouldn't need the files above that
> look hacky to you. Is this what you mean?
Still this wouldn't solve it because I am merging the legacy
interfaces to an existing kobject and creating new kobjects for
the new interfaces that go under gt. I would need some other
ugly hack to have things coming around.
Unless I misunderstood you.
Andi
Re: [Intel-gfx] [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface
Hi Andrzej,
I'm sorry, but I'm not fully understanding,
> > > > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> > > > + const char *name)
> > > > +{
> > > > + struct kobject *kobj = >kobj;
> > > > +
> > > > + /*
> > > > +* We are interested at knowing from where the interface
> > > > +* has been called, whether it's called from gt/ or from
> > > > +* the parent directory.
> > > > +* From the interface position it depends also the value of
> > > > +* the private data.
> > > > +* If the interface is called from gt/ then private data is
> > > > +* of the "struct intel_gt *" type, otherwise it's * a
> > > > +* "struct drm_i915_private *" type.
> > > > +*/
> > > > + if (!is_object_gt(kobj)) {
> > > > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > > > +
> > > > + pr_devel_ratelimited(DEPRECATED
> > > > + "%s (pid %d) is accessing deprecated %s "
> > > > + "sysfs control, please use gt/gt/%s
> > > > instead\n",
> > > > + current->comm, task_pid_nr(current), name,
> > > > name);
> > > > + return to_gt(i915);
> > > > + }
> > > > +
> > > > + return kobj_to_gt(kobj);
> > > It took some time for me to understand what is going on here.
> > > We have dev argument which sometimes can point to "struct device",
> > > sometimes
> > > to "struct kobj_gt", but it's type suggests differently, quite ugly.
> > > I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in
> > > case of intel_engines_add_sysfs. This way abstractions would look better,
> > > hopefully.
> > How would it help?
> >
> > The difference is that I'm adding twice different interfaces with
> > the same name and different location (i.e. different object). The
> > legacy intrefaces inherit the object from drm and I'm preserving
> > that reference.
> >
> > While the new objects would derive from the previous and they are
> > pretty much like intel_engines_add_sysfs().
>
> I was not clear on the issue. Here in case of 'id' attribute it is defined
> as device_attribute, but in kobj_type.sysfs_ops you assign formally
> incompatible _sysfs_ops.
'kobj_sysfs_ops' is of the type 'kobj_type'.
> kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary
> compatible' with device_attribute and kobj is at beginning of struct device
> as well, so it does not blow up, but I wouldn't say it is clean solution :)
> If you look at intel_engines_add_sysfs you can see that all attributes are
> defined as kobj_attribute.
That's exactly the approach I use in the next patches for the
power management files, I use "struct kobj_gt" wrapped around
"struct kobject". But I'm using that only for the GT files.
Are you, btw, suggesting to use this same approache also for the
legacy files that for now have a pointer to the drm kobject? This
way I would need to add more information, like the pointer to
i915 and gt_id. This way I wouldn't need the files above that
look hacky to you. Is this what you mean?
Andi
Re: [Intel-gfx] [PATCH v10 4/5] mei: gsc: add runtime pm handlers
> -Original Message-
> From: Vivi, Rodrigo
> Sent: Thursday, March 10, 2022 21:04
> To: Usyskin, Alexander
> Cc: Greg Kroah-Hartman ; Jani Nikula
> ; Joonas Lahtinen
> ; David Airlie ; Daniel
> Vetter ; Tvrtko Ursulin ;
> linux-ker...@vger.kernel.org; Winkler, Tomas ;
> Lubart, Vitaly ; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v10 4/5] mei: gsc: add runtime pm handlers
>
> On Tue, Mar 08, 2022 at 06:36:53PM +0200, Alexander Usyskin wrote:
> > From: Tomas Winkler
> >
> > Implement runtime handlers for mei-gsc, to track
> > idle state of the device properly.
> >
> > CC: Rodrigo Vivi
> > Signed-off-by: Tomas Winkler
> > Signed-off-by: Alexander Usyskin
> > ---
> > drivers/misc/mei/gsc-me.c | 67
> ++-
> > 1 file changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> > index cf427f6fdec9..dac482ddab51 100644
> > --- a/drivers/misc/mei/gsc-me.c
> > +++ b/drivers/misc/mei/gsc-me.c
> > @@ -152,7 +152,72 @@ static int __maybe_unused
> mei_gsc_pm_resume(struct device *device)
> > return 0;
> > }
> >
> > -static SIMPLE_DEV_PM_OPS(mei_gsc_pm_ops, mei_gsc_pm_suspend,
> mei_gsc_pm_resume);
> > +static int __maybe_unused mei_gsc_pm_runtime_idle(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > +
> > + if (!dev)
> > + return -ENODEV;
> > + if (mei_write_is_idle(dev))
> > + pm_runtime_autosuspend(device);
>
> This is not needed. The _idle() callback is called right before the
> autosuspend.
> so you just need to return -EBUSY if not idle.
>
It is taken from blueprint in pci-me.c
IIRC here we ask the autosuspend to kick in after DELAY,
not simply rejecting it unconditionally.
> But also I'm missing the call to enable the autosuspend and set the delay.
>
These calls are in the second patch in the series, at the end of probe.
> Is this flow really working and you are getting device suspended when not in
> use?
> (Maybe it is just my ignorance on other flow types here)
>
GSC low-power is guided by DG card, here we only signaling to parent (i915, I
think)
that GSC is idle or that we need resume to perform the operations.
> > +
> > + return -EBUSY;
> > +}
> > +
> > +static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > + struct mei_me_hw *hw;
> > + int ret;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + mutex_lock(>device_lock);
> > +
> > + if (mei_write_is_idle(dev)) {
> > + hw = to_me_hw(dev);
> > + hw->pg_state = MEI_PG_ON;
> > + ret = 0;
> > + } else {
> > + ret = -EAGAIN;
> > + }
>
> probably not needed this here... but it would be good if you use
> the runtime_pm{get,put} to protect your write operations as well...
>
We reuse big portions of mei and mei-me drivers and there
all needed runtime_pm calls are implemented.
The runtime pm callbacks are different as GSC do not have
actual HW registers to handle the low power states as CSME has.
> > +
> > + mutex_unlock(>device_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused mei_gsc_pm_runtime_resume(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > + struct mei_me_hw *hw;
> > + irqreturn_t irq_ret;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + mutex_lock(>device_lock);
> > +
> > + hw = to_me_hw(dev);
> > + hw->pg_state = MEI_PG_OFF;
> > +
> > + mutex_unlock(>device_lock);
> > +
> > + irq_ret = mei_me_irq_thread_handler(1, dev);
> > + if (irq_ret != IRQ_HANDLED)
> > + dev_err(dev->dev, "thread handler fail %d\n", irq_ret);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mei_gsc_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mei_gsc_pm_suspend,
> > + mei_gsc_pm_resume)
> > + SET_RUNTIME_PM_OPS(mei_gsc_pm_runtime_suspend,
> > + mei_gsc_pm_runtime_resume,
> > + mei_gsc_pm_runtime_idle)
> > +};
> >
> > static const struct auxiliary_device_id mei_gsc_id_table[] = {
> > {
> > --
> > 2.32.0
> >
--
Thanks,
Sasha
[Intel-gfx] ✗ Fi.CI.IGT: failure for enhanced edid driver compatibility (rev3)
== Series Details ==
Series: enhanced edid driver compatibility (rev3)
URL : https://patchwork.freedesktop.org/series/101241/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_11354_full -> Patchwork_22553_full
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_22553_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_22553_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Participating hosts (12 -> 12)
--
Additional (1): shard-dg1
Missing(1): pig-kbl-iris
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_22553_full:
### IGT changes ###
Possible regressions
* igt@gem_eio@kms:
- shard-apl: [PASS][1] -> [INCOMPLETE][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/shard-apl7/igt@gem_...@kms.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-apl1/igt@gem_...@kms.html
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@gem_ccs@block-copy-inplace:
- {shard-dg1}:NOTRUN -> [SKIP][3] +1 similar issue
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-dg1-18/igt@gem_...@block-copy-inplace.html
Known issues
Here are the changes found in Patchwork_22553_full that come from known
issues:
### IGT changes ###
Issues hit
* igt@gem_exec_balancer@parallel:
- shard-kbl: NOTRUN -> [DMESG-WARN][4] ([i915#5076])
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-kbl3/igt@gem_exec_balan...@parallel.html
* igt@gem_exec_balancer@parallel-contexts:
- shard-tglb: NOTRUN -> [DMESG-WARN][5] ([i915#5076])
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-tglb5/igt@gem_exec_balan...@parallel-contexts.html
* igt@gem_exec_fair@basic-none-vip@rcs0:
- shard-kbl: [PASS][6] -> [FAIL][7] ([i915#2842])
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/shard-kbl6/igt@gem_exec_fair@basic-none-...@rcs0.html
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-kbl6/igt@gem_exec_fair@basic-none-...@rcs0.html
* igt@gem_exec_suspend@basic-s3@smem:
- shard-apl: [PASS][8] -> [DMESG-WARN][9] ([i915#180]) +1 similar
issue
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/shard-apl8/igt@gem_exec_suspend@basic...@smem.html
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-apl6/igt@gem_exec_suspend@basic...@smem.html
* igt@gem_huc_copy@huc-copy:
- shard-tglb: [PASS][10] -> [SKIP][11] ([i915#2190])
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/shard-tglb3/igt@gem_huc_c...@huc-copy.html
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-tglb7/igt@gem_huc_c...@huc-copy.html
- shard-apl: NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#2190])
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-apl7/igt@gem_huc_c...@huc-copy.html
* igt@gem_workarounds@suspend-resume:
- shard-kbl: NOTRUN -> [DMESG-WARN][13] ([i915#180])
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-kbl6/igt@gem_workarou...@suspend-resume.html
* igt@kms_big_fb@4-tiled-max-hw-stride-32bpp-rotate-0-hflip:
- shard-tglb: NOTRUN -> [SKIP][14] ([i915#5286])
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-tglb5/igt@kms_big...@4-tiled-max-hw-stride-32bpp-rotate-0-hflip.html
* igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-hflip-async-flip:
- shard-kbl: NOTRUN -> [SKIP][15] ([fdo#109271] / [i915#3777]) +1
similar issue
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-kbl6/igt@kms_big...@x-tiled-max-hw-stride-32bpp-rotate-180-hflip-async-flip.html
* igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-async-flip:
- shard-skl: NOTRUN -> [FAIL][16] ([i915#3743])
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-skl7/igt@kms_big...@x-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html
* igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0-hflip-async-flip:
- shard-apl: NOTRUN -> [SKIP][17] ([fdo#109271] / [i915#3777])
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/shard-apl8/igt@kms_big...@y-tiled-max-hw-stride-64bpp-rotate-0-hflip-async-flip.html
- shard-skl: NOTRUN -> [SKIP][18] ([fdo#109271] / [i915#3777])
[18]:
Re: [Intel-gfx] Enable DisplayPort MST on low cost USB-C docks
Thanks for the reply! Posted and cross link:
https://gitlab.freedesktop.org/drm/intel/-/issues/5331
Am 21.02.22 um 12:26 schrieb Imre Deak:
Hi Matthias,
could you open a ticket at gitlab, providing a dmesg as described at
https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs
Thanks,
Imre
On Sun, Feb 20, 2022 at 03:33:56PM +0100, Matthias Walther wrote:
Hm, no ideas on this?
Am 28.01.22 um 20:50 schrieb Matthias Walther:
Hello,
there are a lot of quite similar, low cost USB-C docks with multiple
display output (usually 2x HDMI + 1x VGA) available on the big online
platforms such as Amazon, Ebay, and Aliexpress.
Internally the display outputs are connected via DisplayPort. If you
connect a monitor to one of the ports, it's detected as display port
connection in xrandr. Always the same dpX in xrandr, independently of
which physical port in use. This suggests that all physical outputs are
connected to the same DisplayPort output.
On Microsoft's Windows these docks support multi headed output, like a
different image on all displays (called expand mode in Windows). However
the vendor advertises, that on MacOS the adapter can only display the
same image on all ports of the adapter. This might be a hint, that the
adapter internally uses DisplayPort's Multi-Stream Transport (MST)
technology for the second and third display output (2nd HDMI, VGA), as
Apple does not support MST while Microsoft does. Linux behaves just like
MacOS here and only mirrors the image.
Linux is supposed to support MST since like around 2014. There are
parameters to enable it for i915, e. g. i915.enable_dp_mst={1,2}.
However unfortunately those USB-C docks do not support multi-headed
output on Linux. The second monitor is not detected, there is just a
mirrored image of the first monitor on monitor two and three.
Does Linux support MST over Thunderbolt 3/4? Is there maybe a hidden
command that the Windows driver uses to switch MST on in the dock's
chipset?
Any hints on how to debug this would be highly appreciated! Those
adapters become more and more popular, they are affordable and it would
be awesome to make them fully work with Linux.
Best,
Matthias
[Intel-gfx] ✓ Fi.CI.BAT: success for enhanced edid driver compatibility (rev3)
== Series Details ==
Series: enhanced edid driver compatibility (rev3)
URL : https://patchwork.freedesktop.org/series/101241/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_11354 -> Patchwork_22553
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/index.html
Participating hosts (43 -> 39)
--
Additional (1): fi-pnv-d510
Missing(5): shard-tglu fi-bsw-cyan shard-rkl shard-dg1 fi-bdw-samus
Possible new issues
---
Here are the unknown changes that may have been introduced in Patchwork_22553:
### IGT changes ###
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_selftest@live@gt_heartbeat:
- {bat-rpls-2}: [PASS][1] -> [INCOMPLETE][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/bat-rpls-2/igt@i915_selftest@live@gt_heartbeat.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/bat-rpls-2/igt@i915_selftest@live@gt_heartbeat.html
* igt@kms_busy@basic@flip:
- {bat-adlp-6}: [PASS][3] -> [DMESG-WARN][4]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/bat-adlp-6/igt@kms_busy@ba...@flip.html
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/bat-adlp-6/igt@kms_busy@ba...@flip.html
Known issues
Here are the changes found in Patchwork_22553 that come from known issues:
### IGT changes ###
Issues hit
* igt@gem_huc_copy@huc-copy:
- fi-pnv-d510:NOTRUN -> [SKIP][5] ([fdo#109271]) +58 similar issues
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/fi-pnv-d510/igt@gem_huc_c...@huc-copy.html
* igt@i915_selftest@live@hangcheck:
- fi-bdw-5557u: NOTRUN -> [INCOMPLETE][6] ([i915#3921])
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/fi-bdw-5557u/igt@i915_selftest@l...@hangcheck.html
* igt@kms_chamelium@vga-edid-read:
- fi-bdw-5557u: NOTRUN -> [SKIP][7] ([fdo#109271] / [fdo#111827]) +8
similar issues
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/fi-bdw-5557u/igt@kms_chamel...@vga-edid-read.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-bdw-5557u: NOTRUN -> [SKIP][8] ([fdo#109271]) +14 similar issues
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/fi-bdw-5557u/igt@kms_setm...@basic-clone-single-crtc.html
Possible fixes
* igt@i915_selftest@live@gt_lrc:
- {bat-rpls-2}: [DMESG-WARN][9] ([i915#4391]) -> [PASS][10] +1
similar issue
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/bat-rpls-2/igt@i915_selftest@live@gt_lrc.html
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/bat-rpls-2/igt@i915_selftest@live@gt_lrc.html
* igt@kms_flip@basic-flip-vs-modeset@a-edp1:
- {bat-adlp-6}: [DMESG-WARN][11] -> [PASS][12]
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11354/bat-adlp-6/igt@kms_flip@basic-flip-vs-mode...@a-edp1.html
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/bat-adlp-6/igt@kms_flip@basic-flip-vs-mode...@a-edp1.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
[i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
Build changes
-
* Linux: CI_DRM_11354 -> Patchwork_22553
CI-20190529: 20190529
CI_DRM_11354: 5b1b60c0955083406c4873898fc6dd4fd0e2f450 @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_6378: 9c79047f49acdb6450368ee13fd8b6d28f3fb8e1 @
https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_22553: 74f01f274c3782a4064190d8e57b9ca95d93a2d9 @
git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
74f01f274c37 drm/edid: check for HF-SCDB block
84dbfb1659a7 drm/edid: parse HF-EEODB CEA extension block
39376b26f8e4 drm/edid: read HF-EEODB ext block
17e8316a3054 drm/edid: parse multiple CEA extension block
c049087987bb drm/edid: seek for available CEA and DisplayID block from specific
EDID block index
== Logs ==
For more details see:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22553/index.html
[Intel-gfx] ✗ Fi.CI.SPARSE: warning for enhanced edid driver compatibility (rev3)
== Series Details == Series: enhanced edid driver compatibility (rev3) URL : https://patchwork.freedesktop.org/series/101241/ State : warning == Summary == $ dim sparse --fast origin/drm-tip Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately. - +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB" +./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:315:49: error: static assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for enhanced edid driver compatibility (rev3)
== Series Details == Series: enhanced edid driver compatibility (rev3) URL : https://patchwork.freedesktop.org/series/101241/ State : warning == Summary == $ dim checkpatch origin/drm-tip c049087987bb drm/edid: seek for available CEA and DisplayID block from specific EDID block index 17e8316a3054 drm/edid: parse multiple CEA extension block 39376b26f8e4 drm/edid: read HF-EEODB ext block 84dbfb1659a7 drm/edid: parse HF-EEODB CEA extension block -:51: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "!edid" #51: FILE: drivers/gpu/drm/drm_edid.c:3371: + if (edid == NULL || edid->extensions == 0 || *ext_index >= ext_blk_num) total: 0 errors, 0 warnings, 1 checks, 178 lines checked 74f01f274c37 drm/edid: check for HF-SCDB block
[Intel-gfx] [v7 5/5] drm/edid: check for HF-SCDB block
Find HF-SCDB information in CEA extensions block. And retrieve
Max_TMDS_Character_Rate that support by sink device.
v2: HF-SCDB and HF-VSDBS carry the same SCDS data. Reuse
drm_parse_hdmi_forum_vsdb() to parse this packet.
Cc: Jani Nikula
Cc: Ville Syrjala
Cc: Ankit Nautiyal
Cc: Drew Davenport
Cc: intel-gfx
Signed-off-by: Lee Shawn C
---
drivers/gpu/drm/drm_edid.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5de85ba20bdf..351a729bddb6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3350,6 +3350,7 @@ add_detailed_modes(struct drm_connector *connector,
struct edid *edid,
#define EXT_VIDEO_DATA_BLOCK_420 0x0E
#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
#define EXT_VIDEO_HF_EEODB_DATA_BLOCK 0x78
+#define EXT_VIDEO_HF_SCDB_DATA_BLOCK 0x79
#define EDID_BASIC_AUDIO (1 << 6)
#define EDID_CEA_YCRCB444 (1 << 5)
#define EDID_CEA_YCRCB422 (1 << 4)
@@ -4287,6 +4288,20 @@ static bool cea_db_is_vcdb(const u8 *db)
return true;
}
+static bool cea_db_is_hdmi_forum_scdb(const u8 *db)
+{
+ if (cea_db_tag(db) != USE_EXTENDED_TAG)
+ return false;
+
+ if (cea_db_payload_len(db) < 7)
+ return false;
+
+ if (cea_db_extended_tag(db) != EXT_VIDEO_HF_SCDB_DATA_BLOCK)
+ return false;
+
+ return true;
+}
+
static bool cea_db_is_y420cmdb(const u8 *db)
{
if (cea_db_tag(db) != USE_EXTENDED_TAG)
@@ -5312,7 +5327,8 @@ static void drm_parse_cea_ext(struct drm_connector
*connector,
if (cea_db_is_hdmi_vsdb(db))
drm_parse_hdmi_vsdb_video(connector, db);
- if (cea_db_is_hdmi_forum_vsdb(db))
+ if (cea_db_is_hdmi_forum_vsdb(db) ||
+ cea_db_is_hdmi_forum_scdb(db))
drm_parse_hdmi_forum_vsdb(connector, db);
if (cea_db_is_microsoft_vsdb(db))
drm_parse_microsoft_vsdb(connector, db);
--
2.17.1
[Intel-gfx] [v7 2/5] drm/edid: parse multiple CEA extension block
Try to find and parse more CEA ext blocks if edid->extensions
is greater than one.
v2: split prvious patch to two. And do CEA block parsing
in this one.
v3: simplify this patch based on previous change.
v4: refine patch v3.
Cc: Jani Nikula
Cc: Ville Syrjala
Cc: Ankit Nautiyal
Cc: Drew Davenport
Cc: intel-gfx
Signed-off-by: Lee Shawn C
---
drivers/gpu/drm/drm_edid.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 78c415aa6889..9fa84881fbba 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4320,16 +4320,22 @@ static void drm_parse_y420cmdb_bitmap(struct
drm_connector *connector,
static int
add_cea_modes(struct drm_connector *connector, struct edid *edid)
{
- const u8 *cea, *db, *hdmi = NULL, *video = NULL;
- u8 dbl, hdmi_len, video_len = 0;
int modes = 0, cea_ext_index = 0, displayid_ext_index = 0;
- cea = drm_find_cea_extension(edid, _ext_index,
_ext_index);
- if (cea && cea_revision(cea) >= 3) {
+ for (;;) {
+ const u8 *cea, *db, *hdmi = NULL, *video = NULL;
+ u8 dbl, hdmi_len = 0, video_len = 0;
int i, start, end;
+ cea = drm_find_cea_extension(edid, _ext_index,
_ext_index);
+ if (!cea)
+ break;
+
+ if (cea_revision(cea) < 3)
+ continue;
+
if (cea_db_offsets(cea, , ))
- return 0;
+ continue;
for_each_cea_db(cea, i, start, end) {
db = [i];
@@ -4351,15 +4357,15 @@ add_cea_modes(struct drm_connector *connector, struct
edid *edid)
dbl - 1);
}
}
- }
- /*
-* We parse the HDMI VSDB after having added the cea modes as we will
-* be patching their flags when the sink supports stereo 3D.
-*/
- if (hdmi)
- modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
- video_len);
+ /*
+* We parse the HDMI VSDB after having added the cea modes as
we will
+* be patching their flags when the sink supports stereo 3D.
+*/
+ if (hdmi)
+ modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len,
video,
+ video_len);
+ }
return modes;
}
--
2.17.1
[Intel-gfx] [v7 4/5] drm/edid: parse HF-EEODB CEA extension block
While adding CEA modes, try to get available EEODB block
number. Then based on it to parse numbers of ext blocks,
retrieve CEA information and add more CEA modes.
Cc: Jani Nikula
Cc: Ville Syrjala
Cc: Ankit Nautiyal
Cc: Drew Davenport
Cc: intel-gfx
Signed-off-by: Lee Shawn C
---
drivers/gpu/drm/drm_displayid.c | 5 ++-
drivers/gpu/drm/drm_edid.c | 68 +
include/drm/drm_edid.h | 2 +-
3 files changed, 58 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index 31c3e6d7d549..a769c55146f4 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -37,7 +37,10 @@ static const u8 *drm_find_displayid_extension(const struct
edid *edid,
int *length, int *idx,
int *ext_index)
{
- const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT,
ext_index);
+ const u8 *displayid = drm_find_edid_extension(edid,
+ DISPLAYID_EXT,
+ ext_index,
+ edid->extensions);
const struct displayid_header *base;
int ret;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5ae4e83fa5e3..5de85ba20bdf 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3360,23 +3360,25 @@ add_detailed_modes(struct drm_connector *connector,
struct edid *edid,
* Search EDID for CEA extension block.
*/
const u8 *drm_find_edid_extension(const struct edid *edid,
- int ext_id, int *ext_index)
+ int ext_id,
+ int *ext_index,
+ int ext_blk_num)
{
const u8 *edid_ext = NULL;
int i;
/* No EDID or EDID extensions */
- if (edid == NULL || edid->extensions == 0)
+ if (edid == NULL || edid->extensions == 0 || *ext_index >= ext_blk_num)
return NULL;
/* Find CEA extension */
- for (i = *ext_index; i < edid->extensions; i++) {
+ for (i = *ext_index; i < ext_blk_num; i++) {
edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1);
if (edid_ext[0] == ext_id)
break;
}
- if (i >= edid->extensions)
+ if (i >= ext_blk_num)
return NULL;
*ext_index = i + 1;
@@ -3385,14 +3387,19 @@ const u8 *drm_find_edid_extension(const struct edid
*edid,
}
static const u8 *drm_find_cea_extension(const struct edid *edid,
- int *cea_ext_index, int
*displayid_ext_index)
+ int *cea_ext_index,
+ int *displayid_ext_index,
+ int ext_blk_num)
{
const struct displayid_block *block;
struct displayid_iter iter;
const u8 *cea;
/* Look for a CEA extension block from ext_index */
- cea = drm_find_edid_extension(edid, CEA_EXT, cea_ext_index);
+ cea = drm_find_edid_extension(edid,
+ CEA_EXT,
+ cea_ext_index,
+ ext_blk_num);
if (cea)
return cea;
@@ -3676,7 +3683,10 @@ add_alternate_cea_modes(struct drm_connector *connector,
struct edid *edid)
int modes = 0, cea_ext_index = 0, displayid_ext_index = 0;
/* Don't add CEA modes if the CEA extension block is missing */
- if (!drm_find_cea_extension(edid, _ext_index, _ext_index))
+ if (!drm_find_cea_extension(edid,
+ _ext_index,
+ _ext_index,
+ edid->extensions))
return 0;
/*
@@ -4328,7 +4338,10 @@ size_t drm_edid_read_hf_eeodb_blk_count(const struct
edid *edid)
int i, start, end, cea_ext_index = 0, displayid_ext_index = 0;
if (edid->extensions) {
- cea = drm_find_cea_extension(edid, _ext_index,
_ext_index);
+ cea = drm_find_cea_extension(edid,
+_ext_index,
+_ext_index,
+edid->extensions);
if (cea && !cea_db_offsets(cea, , ))
for_each_cea_db(cea, i, start, end)
@@ -4384,13 +4397,20 @@ static int
add_cea_modes(struct drm_connector *connector, struct edid *edid)
{
int modes = 0, cea_ext_index = 0, displayid_ext_index = 0;
+ int ext_blk_num = drm_edid_read_hf_eeodb_blk_count(edid);
+
+ if (!ext_blk_num)
+ ext_blk_num = edid->extensions;
for (;;) {
[Intel-gfx] [v7 3/5] drm/edid: read HF-EEODB ext block
According to HDMI 2.1 spec.
"The HDMI Forum EDID Extension Override Data Block (HF-EEODB)
is utilized by Sink Devices to provide an alternate method to
indicate an EDID Extension Block count larger than 1, while
avoiding the need to present a VESA Block Map in the first
E-EDID Extension Block."
It is a mandatory for HDMI 2.1 protocol compliance as well.
This patch help to know how many HF_EEODB blocks report by sink
and read allo HF_EEODB blocks back.
v2: support to find CEA block, check EEODB block format, and return
available block number in drm_edid_read_hf_eeodb_blk_count().
Cc: Jani Nikula
Cc: Ville Syrjala
Cc: Ankit Nautiyal
Cc: Drew Davenport
Cc: intel-gfx
Signed-off-by: Lee Shawn C
---
drivers/gpu/drm/drm_connector.c | 8 +++-
drivers/gpu/drm/drm_edid.c | 71 +++--
include/drm/drm_edid.h | 1 +
3 files changed, 74 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a50c82bc2b2f..16011023c12e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2129,7 +2129,7 @@ int drm_connector_update_edid_property(struct
drm_connector *connector,
const struct edid *edid)
{
struct drm_device *dev = connector->dev;
- size_t size = 0;
+ size_t size = 0, hf_eeodb_blk_count;
int ret;
const struct edid *old_edid;
@@ -2137,8 +2137,12 @@ int drm_connector_update_edid_property(struct
drm_connector *connector,
if (connector->override_edid)
return 0;
- if (edid)
+ if (edid) {
size = EDID_LENGTH * (1 + edid->extensions);
+ hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count(edid);
+ if (hf_eeodb_blk_count)
+ size = EDID_LENGTH * (1 + hf_eeodb_blk_count);
+ }
/* Set the display info, using edid if available, otherwise
* resetting the values to defaults. This duplicates the work
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9fa84881fbba..5ae4e83fa5e3 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1992,6 +1992,7 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
{
int i, j = 0, valid_extensions = 0;
u8 *edid, *new;
+ size_t hf_eeodb_blk_count;
struct edid *override;
override = drm_get_override_edid(connector);
@@ -2051,7 +2052,35 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
}
kfree(edid);
+ return (struct edid *)new;
+ }
+
+ hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count((struct edid
*)edid);
+ if (hf_eeodb_blk_count >= 2) {
+ new = krealloc(edid, (hf_eeodb_blk_count + 1) * EDID_LENGTH,
GFP_KERNEL);
+ if (!new)
+ goto out;
edid = new;
+
+ valid_extensions = hf_eeodb_blk_count - 1;
+ for (j = 2; j <= hf_eeodb_blk_count; j++) {
+ u8 *block = edid + j * EDID_LENGTH;
+
+ for (i = 0; i < 4; i++) {
+ if (get_edid_block(data, block, j, EDID_LENGTH))
+ goto out;
+ if (drm_edid_block_valid(block, j, false, NULL))
+ break;
+ }
+
+ if (i == 4)
+ valid_extensions--;
+ }
+
+ if (valid_extensions != hf_eeodb_blk_count - 1) {
+ DRM_ERROR("Not able to retrieve proper EDID contain
HF-EEODB data.\n");
+ goto out;
+ }
}
return (struct edid *)edid;
@@ -3315,15 +3344,17 @@ add_detailed_modes(struct drm_connector *connector,
struct edid *edid,
#define VIDEO_BLOCK 0x02
#define VENDOR_BLOCK0x03
#define SPEAKER_BLOCK 0x04
-#define HDR_STATIC_METADATA_BLOCK 0x6
-#define USE_EXTENDED_TAG 0x07
-#define EXT_VIDEO_CAPABILITY_BLOCK 0x00
+#define EXT_VIDEO_CAPABILITY_BLOCK 0x00
+#define HDR_STATIC_METADATA_BLOCK 0x06
+#define USE_EXTENDED_TAG 0x07
#define EXT_VIDEO_DATA_BLOCK_420 0x0E
-#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
+#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
+#define EXT_VIDEO_HF_EEODB_DATA_BLOCK 0x78
#define EDID_BASIC_AUDIO (1 << 6)
#define EDID_CEA_YCRCB444 (1 << 5)
#define EDID_CEA_YCRCB422 (1 << 4)
#define EDID_CEA_VCDB_QS (1 << 6)
+#define HF_EEODB_LENGTH2
/*
* Search EDID for CEA extension block.
@@ -4274,9 +4305,41 @@ static bool cea_db_is_y420vdb(const u8 *db)
return true;
}
+static bool cea_db_is_hdmi_forum_eeodb(const u8 *db)
+{
+ if (cea_db_tag(db) != USE_EXTENDED_TAG)
+ return false;
+
+ if
[Intel-gfx] [v7 1/5] drm/edid: seek for available CEA and DisplayID block from specific EDID block index
drm_find_cea_extension() always look for a top level CEA block. Pass
ext_index from caller then this function to search next available
CEA ext block from a specific EDID block pointer.
v2: save proper extension block index if CTA data information
was found in DispalyID block.
v3: using different parameters to store CEA and DisplayID block index.
configure DisplayID extansion block index before search available
DisplayID block.
Cc: Jani Nikula
Cc: Ville Syrjala
Cc: Ankit Nautiyal
Cc: Drew Davenport
Cc: intel-gfx
Signed-off-by: Lee Shawn C
---
drivers/gpu/drm/drm_displayid.c | 10 +--
drivers/gpu/drm/drm_edid.c | 53 ++---
include/drm/drm_displayid.h | 4 +--
3 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index 32da557b960f..31c3e6d7d549 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -59,11 +59,14 @@ static const u8 *drm_find_displayid_extension(const struct
edid *edid,
}
void displayid_iter_edid_begin(const struct edid *edid,
- struct displayid_iter *iter)
+ struct displayid_iter *iter, int *ext_index)
{
memset(iter, 0, sizeof(*iter));
iter->edid = edid;
+
+ if (ext_index)
+ iter->ext_index = *ext_index;
}
static const struct displayid_block *
@@ -126,7 +129,10 @@ __displayid_iter_next(struct displayid_iter *iter)
}
}
-void displayid_iter_end(struct displayid_iter *iter)
+void displayid_iter_end(struct displayid_iter *iter, int *ext_index)
{
+ if (ext_index)
+ *ext_index = iter->ext_index;
+
memset(iter, 0, sizeof(*iter));
}
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 561f53831e29..78c415aa6889 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3353,28 +3353,27 @@ const u8 *drm_find_edid_extension(const struct edid
*edid,
return edid_ext;
}
-static const u8 *drm_find_cea_extension(const struct edid *edid)
+static const u8 *drm_find_cea_extension(const struct edid *edid,
+ int *cea_ext_index, int
*displayid_ext_index)
{
const struct displayid_block *block;
struct displayid_iter iter;
const u8 *cea;
- int ext_index = 0;
- /* Look for a top level CEA extension block */
- /* FIXME: make callers iterate through multiple CEA ext blocks? */
- cea = drm_find_edid_extension(edid, CEA_EXT, _index);
+ /* Look for a CEA extension block from ext_index */
+ cea = drm_find_edid_extension(edid, CEA_EXT, cea_ext_index);
if (cea)
return cea;
/* CEA blocks can also be found embedded in a DisplayID block */
- displayid_iter_edid_begin(edid, );
+ displayid_iter_edid_begin(edid, , displayid_ext_index);
displayid_iter_for_each(block, ) {
if (block->tag == DATA_BLOCK_CTA) {
cea = (const u8 *)block;
break;
}
}
- displayid_iter_end();
+ displayid_iter_end(, displayid_ext_index);
return cea;
}
@@ -3643,10 +3642,10 @@ add_alternate_cea_modes(struct drm_connector
*connector, struct edid *edid)
struct drm_device *dev = connector->dev;
struct drm_display_mode *mode, *tmp;
LIST_HEAD(list);
- int modes = 0;
+ int modes = 0, cea_ext_index = 0, displayid_ext_index = 0;
/* Don't add CEA modes if the CEA extension block is missing */
- if (!drm_find_cea_extension(edid))
+ if (!drm_find_cea_extension(edid, _ext_index, _ext_index))
return 0;
/*
@@ -4321,11 +4320,11 @@ static void drm_parse_y420cmdb_bitmap(struct
drm_connector *connector,
static int
add_cea_modes(struct drm_connector *connector, struct edid *edid)
{
- const u8 *cea = drm_find_cea_extension(edid);
- const u8 *db, *hdmi = NULL, *video = NULL;
+ const u8 *cea, *db, *hdmi = NULL, *video = NULL;
u8 dbl, hdmi_len, video_len = 0;
- int modes = 0;
+ int modes = 0, cea_ext_index = 0, displayid_ext_index = 0;
+ cea = drm_find_cea_extension(edid, _ext_index,
_ext_index);
if (cea && cea_revision(cea) >= 3) {
int i, start, end;
@@ -4563,6 +4562,7 @@ static void drm_edid_to_eld(struct drm_connector
*connector, struct edid *edid)
const u8 *cea;
const u8 *db;
int total_sad_count = 0;
+ int cea_ext_index = 0, displayid_ext_index = 0;
int mnl;
int dbl;
@@ -4571,7 +4571,7 @@ static void drm_edid_to_eld(struct drm_connector
*connector, struct edid *edid)
if (!edid)
return;
- cea = drm_find_cea_extension(edid);
+ cea = drm_find_cea_extension(edid, _ext_index,
_ext_index);
if (!cea) {
[Intel-gfx] [v7 0/5] enhanced edid driver compatibility
Support to parse multiple CEA extension blocks and HF-EEODB to extend drm edid driver's capability. v4: add one more patch to support HF-SCDB v5: HF-SCDB and HF-VSDBS carry the same SCDS data. Reuse drm_parse_hdmi_forum_vsdb() to parse this packet. v6: save proper extension block index if CTA data information was found in DispalyID block. v7: using different parameters to store CEA and DisplayID block index. configure DisplayID extansion block index before search available DisplayID block. Lee Shawn C (5): drm/edid: seek for available CEA and DisplayID block from specific EDID block index drm/edid: parse multiple CEA extension block drm/edid: read HF-EEODB ext block drm/edid: parse HF-EEODB CEA extension block drm/edid: check for HF-SCDB block drivers/gpu/drm/drm_connector.c | 8 +- drivers/gpu/drm/drm_displayid.c | 15 ++- drivers/gpu/drm/drm_edid.c | 216 +--- include/drm/drm_displayid.h | 4 +- include/drm/drm_edid.h | 3 +- 5 files changed, 194 insertions(+), 52 deletions(-) -- 2.17.1
Re: [Intel-gfx] [PATCH v10 3/5] mei: gsc: setup char driver alive in spite of firmware handshake failure
> -Original Message-
> From: Ceraolo Spurio, Daniele
> Sent: Thursday, March 10, 2022 02:28
> To: Usyskin, Alexander ; Greg Kroah-
> Hartman ; Jani Nikula
> ; Joonas Lahtinen
> ; Vivi, Rodrigo ;
> David Airlie ; Daniel Vetter ; Tvrtko
> Ursulin
> Cc: linux-ker...@vger.kernel.org; Winkler, Tomas
> ; Lubart, Vitaly ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v10 3/5] mei: gsc: setup char driver alive in
> spite of firmware handshake failure
>
>
>
> On 3/8/2022 8:36 AM, Alexander Usyskin wrote:
> > Setup char device in spite of firmware handshake failure.
> > In order to provide host access to the firmware status registers and other
> > information required for the manufacturing process.
>
> IMO this patch should be moved to after the patch that adds the logic to
> fetch the FW version, as that is interesting info for sysfs. Not a blocker.
>
Actually, the FW version is filled only if there is an established channel with
FW.
Firmware status registers are the crucial information for debug, and it filled
in previous patches.
--
Thanks,
Sasha
> >
> > Signed-off-by: Alexander Usyskin
> > Signed-off-by: Tomas Winkler
>
> Reviewed-by: Daniele Ceraolo Spurio
>
> Daniele
>
> > ---
> > drivers/misc/mei/gsc-me.c | 11 ++-
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> > index 0afae70e0609..cf427f6fdec9 100644
> > --- a/drivers/misc/mei/gsc-me.c
> > +++ b/drivers/misc/mei/gsc-me.c
> > @@ -79,11 +79,12 @@ static int mei_gsc_probe(struct auxiliary_device
> *aux_dev,
> > pm_runtime_set_active(device);
> > pm_runtime_enable(device);
> >
> > - if (mei_start(dev)) {
> > - dev_err(device, "init hw failure.\n");
> > - ret = -ENODEV;
> > - goto err;
> > - }
> > + /* Continue to char device setup in spite of firmware handshake
> failure.
> > +* In order to provide access to the firmware status registers to the
> user
> > +* space via sysfs.
> > +*/
> > + if (mei_start(dev))
> > + dev_warn(device, "init hw failure.\n");
> >
> > pm_runtime_set_autosuspend_delay(device,
> MEI_GSC_RPM_TIMEOUT);
> > pm_runtime_use_autosuspend(device);
Re: [Intel-gfx] [PATCH v10 2/5] mei: add support for graphics system controller (gsc) devices
> -Original Message-
> From: Ceraolo Spurio, Daniele
> Sent: Thursday, March 10, 2022 02:24
> To: Usyskin, Alexander ; Greg Kroah-
> Hartman ; Jani Nikula
> ; Joonas Lahtinen
> ; Vivi, Rodrigo ;
> David Airlie ; Daniel Vetter ; Tvrtko
> Ursulin
> Cc: linux-ker...@vger.kernel.org; Winkler, Tomas
> ; Lubart, Vitaly ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v10 2/5] mei: add support for graphics system
> controller (gsc) devices
>
>
> On 3/8/2022 8:36 AM, Alexander Usyskin wrote:
> > From: Tomas Winkler
> >
> > GSC is a graphics system controller, based on CSE, it provides
> > a chassis controller for graphics discrete cards, as well as it
> > supports media protection on selected devices.
> >
> > mei_gsc binds to a auxiliary devices exposed by Intel discrete
> > driver i915.
> >
> > Signed-off-by: Alexander Usyskin
> > Signed-off-by: Tomas Winkler
> > ---
> > drivers/misc/mei/Kconfig | 14 +++
> > drivers/misc/mei/Makefile | 3 +
> > drivers/misc/mei/gsc-me.c | 186
> ++
> > drivers/misc/mei/hw-me.c | 27 +-
> > drivers/misc/mei/hw-me.h | 2 +
> > 5 files changed, 230 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/misc/mei/gsc-me.c
> >
> > diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
> > index 0e0bcd0da852..d21486d69df2 100644
> > --- a/drivers/misc/mei/Kconfig
> > +++ b/drivers/misc/mei/Kconfig
> > @@ -46,6 +46,20 @@ config INTEL_MEI_TXE
> > Supported SoCs:
> > Intel Bay Trail
> >
> > +config INTEL_MEI_GSC
>
> On platforms with a GSC, INTEL_MEI_PXP depends on INTEL_MEI_GSC. Are
> you
> planning to add that dependency once the HECI1/PXP interface is exposed,
> or are you expecting i915 to check for both?
>
IMO - add dependency to mei_pxp after HECI1/PXP is exposed.
> > + tristate "Intel MEI GSC embedded device"
> > + depends on INTEL_MEI
> > + depends on INTEL_MEI_ME
> > + depends on X86 && PCI
> > + depends on DRM_I915
> > + help
> > + Intel auxiliary driver for GSC devices embedded in Intel graphics
> devices.
> > +
> > + An MEI device here called GSC can be embedded in an
> > + Intel graphics devices, to support a range of chassis
> > + tasks such as graphics card firmware update and security
> > + tasks.
> > +
> > source "drivers/misc/mei/hdcp/Kconfig"
> > source "drivers/misc/mei/pxp/Kconfig"
> >
> > diff --git a/drivers/misc/mei/Makefile b/drivers/misc/mei/Makefile
> > index d8e5165917f2..fb740d754900 100644
> > --- a/drivers/misc/mei/Makefile
> > +++ b/drivers/misc/mei/Makefile
> > @@ -18,6 +18,9 @@ obj-$(CONFIG_INTEL_MEI_ME) += mei-me.o
> > mei-me-objs := pci-me.o
> > mei-me-objs += hw-me.o
> >
> > +obj-$(CONFIG_INTEL_MEI_GSC) += mei-gsc.o
> > +mei-gsc-objs := gsc-me.o
> > +
> > obj-$(CONFIG_INTEL_MEI_TXE) += mei-txe.o
> > mei-txe-objs := pci-txe.o
> > mei-txe-objs += hw-txe.o
> > diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> > new file mode 100644
> > index ..0afae70e0609
> > --- /dev/null
> > +++ b/drivers/misc/mei/gsc-me.c
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
> > + *
> > + * Intel Management Engine Interface (Intel MEI) Linux driver
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "mei_dev.h"
> > +#include "hw-me.h"
> > +#include "hw-me-regs.h"
> > +
> > +#include "mei-trace.h"
> > +
> > +#define MEI_GSC_RPM_TIMEOUT 500
>
> MEI_ME_RPM_TIMEOUT already exists in hw-me.h with the same value. If
> you're not expecting them to diverge, we could just re-use the existing
> one. Not a blocker.
>
The GSC usecases somewhat different from CSME, so I prefer
to split this timeout, as GSC one may need tuning.
> > +
> > +static int mei_gsc_read_hfs(const struct mei_device *dev, int where, u32
> *val)
> > +{
> > + struct mei_me_hw *hw = to_me_hw(dev);
> > +
> > + *val = ioread32(hw->mem_addr + where + 0xC00);
> > +
> > + return 0;
> > +}
> > +
> > +static int mei_gsc_probe(struct auxiliary_device *aux_dev,
> > +const struct auxiliary_device_id *aux_dev_id)
> > +{
> > + struct mei_aux_device *adev =
> auxiliary_dev_to_mei_aux_dev(aux_dev);
> > + struct mei_device *dev;
>
> might be worth renaming this variable to mei_dev, to avoid confusion
> with "device" below. Not a blocker.
>
Similar functions in MEI always name it dev, so prefer to leave it for
consistency.
> > + struct mei_me_hw *hw;
> > + struct device *device;
> > + const struct mei_cfg *cfg;
> > + int ret;
> > +
> > + cfg = mei_me_get_cfg(aux_dev_id->driver_data);
> > + if (!cfg)
> > + return -ENODEV;
> > +
> > + device = _dev->dev;
> > +
> > + dev = mei_me_dev_init(device, cfg);
> > + if (IS_ERR(dev)) {
> > + ret = PTR_ERR(dev);
> > +
Re: [Intel-gfx] [PATCH v10 1/5] drm/i915/gsc: add gsc as a mei auxiliary device
> -Original Message- > From: Ceraolo Spurio, Daniele > Sent: Thursday, March 10, 2022 00:50 > To: Usyskin, Alexander ; Greg Kroah- > Hartman ; Jani Nikula > ; Joonas Lahtinen > ; Vivi, Rodrigo ; > David Airlie ; Daniel Vetter ; Tvrtko > Ursulin > Cc: intel-gfx@lists.freedesktop.org; linux-ker...@vger.kernel.org; Winkler, > Tomas ; Lubart, Vitaly > Subject: Re: [Intel-gfx] [PATCH v10 1/5] drm/i915/gsc: add gsc as a mei > auxiliary device > > > > On 3/8/2022 8:36 AM, Alexander Usyskin wrote: > > From: Tomas Winkler > > > > GSC is a graphics system controller, it provides > > a chassis controller for graphics discrete cards. > > > > There are two MEI interfaces in GSC: HECI1 and HECI2. > > > > Both interfaces are on the BAR0 at offsets 0x00258000 and 0x00259000. > > GSC is a GT Engine (class 4: instance 6). HECI1 interrupt is signaled > > via bit 15 and HECI2 via bit 14 in the interrupt register. > > > > This patch exports GSC as auxiliary device for mei driver to bind to > > for HECI2 interface. > > Do we need a test for this? E.g. to catch the unlikely case where we > stop exposing the GSC device. We are going to get some indirect coverage > once we start making use of the PXP interface from within the kernel, > would that be enough? > The IGT tests checks that there is no dmesg errors in many error flows. Do not think that we need special functional tests. > Also, IMO we need a line here to explain that we're adding the code for > HECI1 as well because we plan to follow up with it soon. > Will add such line > > > > CC: Rodrigo Vivi > > Signed-off-by: Tomas Winkler > > Signed-off-by: Vitaly Lubart > > Signed-off-by: Alexander Usyskin > > Acked-by: Tvrtko Ursulin > > --- > > MAINTAINERS | 1 + > > drivers/gpu/drm/i915/Kconfig | 1 + > > drivers/gpu/drm/i915/Makefile| 3 + > > drivers/gpu/drm/i915/gt/intel_gsc.c | 204 > +++ > > drivers/gpu/drm/i915/gt/intel_gsc.h | 37 > > drivers/gpu/drm/i915/gt/intel_gt.c | 3 + > > drivers/gpu/drm/i915/gt/intel_gt.h | 5 + > > drivers/gpu/drm/i915/gt/intel_gt_irq.c | 13 ++ > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + > > drivers/gpu/drm/i915/gt/intel_gt_types.h | 2 + > > drivers/gpu/drm/i915/i915_drv.h | 8 + > > drivers/gpu/drm/i915/i915_pci.c | 3 +- > > drivers/gpu/drm/i915/i915_reg.h | 2 + > > drivers/gpu/drm/i915/intel_device_info.h | 2 + > > include/linux/mei_aux.h | 19 +++ > > 15 files changed, 303 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpu/drm/i915/gt/intel_gsc.c > > create mode 100644 drivers/gpu/drm/i915/gt/intel_gsc.h > > create mode 100644 include/linux/mei_aux.h > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 2b1d296f92e9..d322e630d1d1 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9822,6 +9822,7 @@ S:Supported > > F:Documentation/driver-api/mei/* > > F:drivers/misc/mei/ > > F:drivers/watchdog/mei_wdt.c > > +F: include/linux/mei_aux.h > > F:include/linux/mei_cl_bus.h > > F:include/uapi/linux/mei.h > > F:samples/mei/* > > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig > > index 98c5450b8eac..2660a85175d9 100644 > > --- a/drivers/gpu/drm/i915/Kconfig > > +++ b/drivers/gpu/drm/i915/Kconfig > > @@ -30,6 +30,7 @@ config DRM_I915 > > select VMAP_PFN > > select DRM_TTM > > select DRM_BUDDY > > + select AUXILIARY_BUS > > help > > Choose this option if you have a system that has "Intel Graphics > > Media Accelerator" or "HD Graphics" integrated graphics, > > diff --git a/drivers/gpu/drm/i915/Makefile > b/drivers/gpu/drm/i915/Makefile > > index 1a771ee5b1d0..9be7b13d8822 100644 > > --- a/drivers/gpu/drm/i915/Makefile > > +++ b/drivers/gpu/drm/i915/Makefile > > @@ -196,6 +196,9 @@ i915-y += gt/uc/intel_uc.o \ > > gt/uc/intel_huc_debugfs.o \ > > gt/uc/intel_huc_fw.o > > > > +# graphics system controller (GSC) support > > +i915-y += gt/intel_gsc.o > > + > > # modesetting core code > > i915-y += \ > > display/hsw_ips.o \ > > diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c > b/drivers/gpu/drm/i915/gt/intel_gsc.c > > new file mode 100644 > > index ..152804e7c41a > > --- /dev/null > > +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c > > @@ -0,0 +1,204 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright(c) 2019-2022, Intel Corporation. All rights reserved. > > + */ > > + > > +#include > > +#include > > +#include "i915_reg.h" > > +#include "i915_drv.h" > > +#include "gt/intel_gt.h" > > +#include "intel_gsc.h" > > A bit of inconsistency here because intel_gsc.h and intel_gt.h are both > in the gt/ folder but you're only prefixing one with gt/. Also, we > usually try to keep includes in alphabetical order, but overall not a > blocker for me. > Well
