Re: [PATCH 10/23] drm/i915: Move HAS_RC6 definition to platform definition

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

On Thu, Jul 21, 2016 at 3:50 AM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> On 20/07/16 18:40, Carlos Santa wrote:
>>
>> Moving all GPU features to the platform struct definition allows for
>>         - standard place when adding new features from new platforms
>>         - possible to see supported features when dumping struct
>>           definitions
>>
>> Signed-off-by: Carlos Santa <carlos.santa@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h | 3 ++-
>>   drivers/gpu/drm/i915/i915_pci.c | 5 +++++
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index a326a88..75131a0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -775,6 +775,7 @@ struct intel_csr {
>>         func(has_guc) sep \
>>         func(has_guc_ucode) sep \
>>         func(has_guc_sched) sep \
>> +       func(has_rc6) sep \
>>         func(has_resource_streamer) sep \
>>         func(has_pipe_cxsr) sep \
>>         func(has_hotplug) sep \
>> @@ -2856,7 +2857,7 @@ struct drm_i915_cmd_table {
>>   #define HAS_FPGA_DBG_UNCLAIMED(dev)   (INTEL_INFO(dev)->has_fpga_dbg)
>>   #define HAS_PSR(dev)          (INTEL_INFO(dev)->has_psr)
>>   #define HAS_RUNTIME_PM(dev)   (INTEL_INFO(dev)->has_runtime_pm)
>> -#define HAS_RC6(dev)           (INTEL_INFO(dev)->gen >= 6)
>> +#define HAS_RC6(dev)           (INTEL_INFO(dev)->has_rc6)
>>   #define HAS_RC6p(dev)         (IS_GEN6(dev) || IS_IVYBRIDGE(dev))
>>
>>   #define HAS_CSR(dev)  (INTEL_INFO(dev)->has_csr)
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c
>> b/drivers/gpu/drm/i915/i915_pci.c
>> index f59ad4b..e10fb5c 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -200,6 +200,7 @@ static const struct intel_device_info
>> intel_ironlake_m_info = {
>>         .has_fbc = 1, \
>>         .has_runtime_pm = 1, \
>>         .has_core_ring_freq = 1, \
>> +       .has_rc6 = 1, \
>
>
> This platform claims to be Gen5 so no RC6.
>
> I have a slight reservation on all this because sometimes it is very useful
> to see that "INTEL_INFO(dev)->gen >= 6". When sprinkled around like here it
> becomes harder to figure out which feature is supported by which platforms.

Well, right now we have 3 issues that this series address:

1 - unify/standardize the way we introduce a feature in a giving platform.
2 - have a view to all features available on a giving platform.
3 - Make platform enabling with legacy features easy

the only downside is indeed for some cases it is getting hard now to answer:
"which platforms support this specific feature?"
But anyways they are all organized in only one file on structs and
macros that are
easy to navigate and understand.

>
> Once I tried something like this work (mind you I was concentrating only on
> HAS_ and IS_ macros which contain multiple conditionals - it was an
> excercise in reducing multiple conditionals at runtime), I decided to keep
> the macro but renamed it to have a leading underscore. And I did the
> assignment to device_info in an appropriate place, for example:
>
> device_info->has_rc6 = _HAS_RC6(dev_priv);

But if you do this you loose the

>
> For completeness:
>
> #define _HAS_RC6(dev)           (INTEL_INFO(dev)->gen >= 6)

Maybe a comment here would solve this downside:

/* RC6 is supported on all platforms starting on gen6. */
#define HAS_RC6(dev)            (INTEL_INFO(dev)->has_rc6)

or something like this...

Thanks,
Rodrigo.

>
> I was not too happy with that approach either, but I think the downside I
> mentioned above is real.
>
> Regards,
>
> Tvrtko
>
>
>>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>>         .has_llc = 1, \
>>         GEN_DEFAULT_PIPEOFFSETS, \
>> @@ -219,6 +220,7 @@ static const struct intel_device_info
>> intel_sandybridge_m_info = {
>>         .need_gfx_hws = 1, .has_hotplug = 1, \
>>         .has_fbc = 1, \
>>         .has_core_ring_freq = 1, \
>> +       .has_rc6 = 1, \
>>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>>         .has_llc = 1, \
>>         GEN_DEFAULT_PIPEOFFSETS, \
>> @@ -245,6 +247,7 @@ static const struct intel_device_info
>> intel_ivybridge_q_info = {
>>         .gen = 7, .num_pipes = 2, \
>>         .has_psr = 1, \
>>         .has_runtime_pm = 1, \
>> +       .has_rc6 = 1, \
>>         .need_gfx_hws = 1, .has_hotplug = 1, \
>>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>>         .display_mmio_offset = VLV_DISPLAY_BASE, \
>> @@ -320,6 +323,7 @@ static const struct intel_device_info
>> intel_cherryview_info = {
>>         .has_psr = 1,
>>         .has_runtime_pm = 1,
>>         .has_resource_streamer = 1,
>> +       .has_rc6 = 1,
>>         .display_mmio_offset = VLV_DISPLAY_BASE,
>>         GEN_CHV_PIPEOFFSETS,
>>         CURSOR_OFFSETS,
>> @@ -358,6 +362,7 @@ static const struct intel_device_info
>> intel_broxton_info = {
>>         .has_runtime_pm = 1,
>>         .has_pooled_eu = 0,
>>         .has_resource_streamer = 1,
>> +       .has_rc6 = 1,
>>         GEN_DEFAULT_PIPEOFFSETS,
>>         IVB_CURSOR_OFFSETS,
>>         BDW_COLORS,
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx