On Thu, Jul 21, 2016 at 3:50 AM, Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
