Re: [PATCH 7/8] drm/display: Move HDMI helpers into display-helper module

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 07 Apr 2022, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 06 Apr 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 30.03.22 um 12:35 schrieb Jani Nikula:
>>> On Tue, 22 Mar 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>> index 144c495b99c4..e6e9e4557067 100644
>>>> --- a/include/drm/drm_edid.h
>>>> +++ b/include/drm/drm_edid.h
>>>> @@ -391,33 +391,6 @@ drm_load_edid_firmware(struct drm_connector *connector)
>>>>   
>>>>   bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2);
>>>>   
>>>> -int
>>>> -drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>>> -					 const struct drm_connector *connector,
>>>> -					 const struct drm_display_mode *mode);
>>>> -int
>>>> -drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>>>> -					    const struct drm_connector *connector,
>>>> -					    const struct drm_display_mode *mode);
>>>> -
>>>> -void
>>>> -drm_hdmi_avi_infoframe_colorimetry(struct hdmi_avi_infoframe *frame,
>>>> -				   const struct drm_connector_state *conn_state);
>>>> -
>>>> -void
>>>> -drm_hdmi_avi_infoframe_bars(struct hdmi_avi_infoframe *frame,
>>>> -			    const struct drm_connector_state *conn_state);
>>>> -
>>>> -void
>>>> -drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>>>> -				   const struct drm_connector *connector,
>>>> -				   const struct drm_display_mode *mode,
>>>> -				   enum hdmi_quantization_range rgb_quant_range);
>>>> -
>>>> -int
>>>> -drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
>>>> -				    const struct drm_connector_state *conn_state);
>>>> -
>>>>   /**
>>>>    * drm_eld_mnl - Get ELD monitor name length in bytes.
>>>>    * @eld: pointer to an eld memory structure with mnl set
>>>> @@ -587,6 +560,10 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>>>>   struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>>>>   					   int hsize, int vsize, int fresh,
>>>>   					   bool rb);
>>>> +
>>>> +u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match);
>>>> +enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
>>>> +enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8 video_code);
>>> 
>>> I think these were fine as static, but not really great interfaces to
>>> export. There's zero input checking on the vic in the latter, because
>>> internally we could be sure they were fine.
>>
>> I see. If nothing else, HDMI could be removed from the patchset. OTOH 
>> having these HDMI functions as part of the edid code doesn't seem right 
>> either.

To clarify, I think the HDMI functionality should probably be
moved. It's just the new interfaces I'm worried about.

BR,
Jani.


>>
>>> 
>>> I also wish we could limit the usage to the module you're adding; this
>>> is now available to all drivers which should be discouraged.
>>
>> Why is that discouraged? Quite a few drivers use these interfaces.
>
> No driver needed to directly use the functions you're now additionally
> exporting from drm_edid.c. I'd hope no driver starts to use them either.
>
> BR,
> Jani.
>
>
>
>>
>> Best regards
>> Thomas
>>> 
>>> 
>>> BR,
>>> Jani.
>>> 
>>> 

-- 
Jani Nikula, Intel Open Source Graphics Center