Re: [PATCH 2/2] drm/msm/hdmi: add hdmi hdcp support

[jilaiw]

Hi Bjorn,

> On Tue, Dec 2, 2014 at 8:46 PM, Bjorn Andersson <bjorn@xxxxxxx> wrote:
>> On Mon, Dec 1, 2014 at 1:56 PM, Jilai Wang <jilaiw@xxxxxxxxxxxxxx>
>> wrote:
>>> Add HDMI HDCP support including HDCP PartI/II/III authentication.
>>>
>>> Signed-off-by: Jilai Wang <jilaiw@xxxxxxxxxxxxxx>
>>> ---
>>
>> Hi Jilai,
>>
>> [..]
>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>
>> [..]
>>
>>>
>>> @@ -119,6 +137,22 @@ struct hdmi *hdmi_init(struct drm_device *dev,
>>> struct drm_encoder *encoder)
>>>                 goto fail;
>>>         }
>>>
>>> +       /* HDCP needs physical address of hdmi register */
>>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> +               config->mmio_name);
>>
>> Is this guaranteed to be available at all times? You should probably
>> do some error handling here.
>
> Hi Bjorn,
>
> (as mentioned on irc, but just repeating here for posterity)
>
> this is actually ok in this case, since the previous ioremap would have
> failed.
>
>>> +       hdmi->mmio_phy_addr = res->start;
>>> +
>>> +       if (config->qfprom_mmio_name) {
>>
>> Should this check really be here? This will always be set if CONFIG_OF
>> is set
>> and never otherwise and that seems strange to me.
>>
>> Perhaps you should add the string to the !CONFIG_OF platforms as well or
>> simply
>> add #ifdef CONFIG_OF around this section if that's what you really want.
>> (but
>> seems more like you forgot the non-of case).
>
> or just not cared enough to make HDCP (since it is optional) work on
> old pre-DT vendor kernel ;-)
>
> fwiw, eventually the !CONFIG_OF stuff will go away.. it just exists
> because some devices and some use-cases still need old downstream
> kernels :-(
>
>>> +               hdmi->qfprom_mmio = msm_ioremap(pdev,
>>> +                       config->qfprom_mmio_name, "HDMI_QFPROM");
>>
>>
>> Is this a special hdmi qfprom or are you ioremapping _the_ qfprom here?
>>
>> If so I did suggest that we expose it as a syscon but I think Stephen
>> Boyd had
>> some other ideas.
>
> afaict (but Jilai can correct me), it is *the* qfprom.. that seems to
> be how things worked in android kernels.  Some better mechanism would
> be really nice.
>
>>> +               if (IS_ERR(hdmi->qfprom_mmio)) {
>>> +                       dev_info(&pdev->dev, "can't find qfprom
>>> resource\n");
>>> +                       hdmi->qfprom_mmio = NULL;
>>> +               }
>>> +       } else {
>>> +               hdmi->qfprom_mmio = NULL;
>>
>> hdmi_qfprom_read() seems to be called and read from qfprom_mmio no
>> matter how
>> this ended. Are you sure this (both error paths) shouldn't be handled as
>> a
>> fatal error?
>
> hdmi_hdcp_init() fails (and then we continue without HDCP) if
> qfprom_mmio is NULL..
>
>> 'hdmi' is kzalloc and hence already NULL.
>>
>> [..]
>>
>>> @@ -205,6 +241,13 @@ struct hdmi *hdmi_init(struct drm_device *dev,
>>> struct drm_encoder *encoder)
>>>                 goto fail;
>>>         }
>>>
>>> +       hdmi->hdcp_ctrl = hdmi_hdcp_init(hdmi);
>>> +       if (IS_ERR(hdmi->hdcp_ctrl)) {
>>> +               ret = PTR_ERR(hdmi->hdcp_ctrl);
>>> +               dev_warn(dev->dev, "failed to init hdcp:
>>> %d(disabled)\n", ret);
>>> +               hdmi->hdcp_ctrl = NULL;
>>
>> So either you treat this as an error or you don't.
>>
>> If you're fine continuing execution without hdcp_ctrl then you shouldn't
>> set
>> ret. But in that case it you should probably not print a warning every
>> time you
>> enter hdmi_hdcp_on() and an error on hdmi_hdcp_off().
>
> agreed, I think it would be better for hdcp_on/off() to take struct
> hdmi_hdcp_ctrl as param (and just not be called if hdmi->hdcp_ctrl is
> null)
>
> [snip]
>
>> [..]
>>
>>> +
>>> +struct hdmi_hdcp_reg_data {
>>> +       uint32_t reg_id;
>>
>> You should use u32 instead of uint32_t in the kernel.
>
> tbh, I'd prefer sticking to stdint types..  before stdint was a thing,
> u32 made sense
>
>>> +       uint32_t off;
>>> +       char *name;
>>> +       uint32_t reg_val;
>>> +};
>>> +
>>> +struct hdmi_hdcp_ctrl {
>>> +       struct hdmi *hdmi;
>>> +       uint32_t auth_retries;
>>> +       uint32_t tz_hdcp;
>>
>> Turn this into a bool named something like has_tz_hdcp instead, as
>> that's what
>> it really means.
>>
>>> +       enum hdmi_hdcp_state hdcp_state;
>>> +       struct mutex state_mutex;
>>> +       struct delayed_work hdcp_reauth_work;
>>> +       struct delayed_work hdcp_auth_part1_1_work;
>>> +       struct delayed_work hdcp_auth_part1_2_work;
>>> +       struct work_struct hdcp_auth_part1_3_work;
>>> +       struct delayed_work hdcp_auth_part2_1_work;
>>> +       struct delayed_work hdcp_auth_part2_2_work;
>>> +       struct delayed_work hdcp_auth_part2_3_work;
>>> +       struct delayed_work hdcp_auth_part2_4_work;
>>> +       struct work_struct hdcp_auth_prepare_work;
>>
>> You shouldn't use "work" as a way to express states in your state
>> machine.
>> Better have 1 auth work function that does all these steps, probably
>> having
>> them split in functions just like you do now.
>>
>> That way you can have 1 function running the pass of authentication,
>> starting
>> by checking if you're reauthing or not then processing each step one by
>> one,
>> sleeping inbetween them. You can have the functions return -EAGAIN to
>> indicate
>> that you need to retry the current operation and so on.
>>
>> This would split the state machine from the state executioners and
>> simplify
>> your code.
>
> As a side note (disclaimer, I'm not an hdcp expert), but I wonder if
> eventually some of that should be extracted out into some helpers in
> drm core.  I guess that is something we'll figure out when a 2nd
> driver gains upstream HDCP support.  One big work w/ msleep()'s does
> sound like it would be easier to understand (and therefore easier to
> refactor out into helpers).
>
> [snip]
>

The reason that I break the partI/PartII work into these small works
because I want to avoid to use msleep in work.
Otherwise cancel a HDCP work may cause long delay up to several seconds.

>>> +
>>> +static int hdmi_hdcp_scm_wr(struct hdmi_hdcp_ctrl *hdcp_ctrl, uint32_t
>>> *preg,
>>> +       uint32_t *pdata, uint32_t count)
>>> +{
>>> +       struct hdmi *hdmi = hdcp_ctrl->hdmi;
>>> +       struct scm_hdcp_req scm_buf[SCM_HDCP_MAX_REG];
>>> +       uint32_t resp, phy_addr, idx = 0;
>>> +       int i, ret = 0;
>>> +
>>> +       if (count == 0)
>>> +               return 0;
>>
>> There are no calls to this function where count can be 0, so you can
>> drop this
>> check.
>>
>>> +
>>> +       if (!preg || !pdata) {
>>> +               pr_err("%s: Invalid pointer\n", __func__);
>>> +               return -EINVAL;
>>> +       }
>>
>> There are no calls to this function where either of these are NULL, so
>> you can
>> drop the entire block.
>
> or just WARN_ON() (for both this and the count).. I find that makes
> the constraints more clear for someone coming along later and mucking
> with that code
>
>>
>>> +
>>> +       if (hdcp_ctrl->tz_hdcp) {
>>> +               phy_addr = (uint32_t)hdmi->mmio_phy_addr;
>>> +
>>> +               while (count) {
>>> +                       memset(scm_buf, 0, sizeof(scm_buf));
>>> +                       for (i = 0; i < count && i < SCM_HDCP_MAX_REG;
>>> i++) {
>>> +                               scm_buf[i].addr = phy_addr + preg[idx];
>>> +                               scm_buf[i].val  = pdata[idx];
>>> +                               idx++;
>>> +                       }
>>> +                       ret = scm_call(SCM_SVC_HDCP, SCM_CMD_HDCP,
>>> +                               scm_buf, sizeof(scm_buf), &resp,
>>> sizeof(resp));
>>
>> SCM_SVC_HDCP nor SCM_CMD_HDCP are defined, here. See the comment above
>> related
>> to TZ_HDCP_CMD_ID.
>>
>>> +
>>> +                       if (ret || resp) {
>>> +                               pr_err("%s: error: scm_call ret = %d,
>>> resp = %d\n",
>>> +                                       __func__, ret, resp);
>>> +                               ret = -EINVAL;
>>> +                               break;
>>> +                       }
>>> +
>>> +                       count -= i;
>>> +               }
>>> +       } else {
>>> +               for (i = 0; i < count; i++)
>>> +                       hdmi_write(hdmi, preg[i], pdata[i]);
>>> +       }
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +void hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl)
>>> +{
>>> +       struct hdmi *hdmi;
>>> +       uint32_t regval, hdcp_int_status;
>>> +       unsigned long flags;
>>> +
>>> +       if (!hdcp_ctrl) {
>>> +               DBG("HDCP is disabled");
>>> +               return;
>>> +       }
>>
>> No need to print a debug line here every time.
>>
>> I would have preferred if you made the call from hdmi_irq() conditional
>> instead, then you would need to check here...
>
> me too
>
> [snip]
>
>>> +static void reset_hdcp_ddc_failures(struct hdmi_hdcp_ctrl *hdcp_ctrl)
>>> +{
>>> +       int hdcp_ddc_ctrl1_reg;
>>> +       int hdcp_ddc_status;
>>> +       int failure;
>>> +       int nack0;
>>> +       struct hdmi *hdmi = hdcp_ctrl->hdmi;
>>> +
>>> +       /* Check for any DDC transfer failures */
>>> +       hdcp_ddc_status = hdmi_read(hdmi, REG_HDMI_HDCP_DDC_STATUS);
>>> +       failure = (hdcp_ddc_status >> 16) & 0x1;
>>
>> failure = hdcp_ddc_status & BIT(16);
>>
>>> +       nack0 = (hdcp_ddc_status >> 14) & 0x1;
>>
>> nack0 = hdcp_ddc_status & BIT(14);
>>
>>> +       DBG("On Entry: HDCP_DDC_STATUS=0x%x, FAIL=%d, NACK0=%d",
>>> +               hdcp_ddc_status, failure, nack0);
>>> +
>>> +       if (failure == 0x1) {
>>> +               /*
>>> +                * Indicates that the last HDCP HW DDC transfer failed.
>>> +                * This occurs when a transfer is attempted with HDCP
>>> DDC
>>> +                * disabled (HDCP_DDC_DISABLE=1) or the number of
>>> retries
>>> +                * matches HDCP_DDC_RETRY_CNT.
>>> +                * Failure occurred,  let's clear it.
>>> +                */
>>> +               DBG("DDC failure detected.HDCP_DDC_STATUS=0x%08x",
>>> +                       hdcp_ddc_status);
>>> +
>>> +               /* First, Disable DDC */
>>> +               hdmi_write(hdmi, REG_HDMI_HDCP_DDC_CTRL_0, BIT(0));
>>> +
>>> +               /* ACK the Failure to Clear it */
>>> +               hdcp_ddc_ctrl1_reg = hdmi_read(hdmi,
>>> REG_HDMI_HDCP_DDC_CTRL_1);
>>> +               hdmi_write(hdmi, REG_HDMI_HDCP_DDC_CTRL_1,
>>> +                       hdcp_ddc_ctrl1_reg | BIT(0));
>>> +
>>> +               /* Check if the FAILURE got Cleared */
>>> +               hdcp_ddc_status = hdmi_read(hdmi,
>>> REG_HDMI_HDCP_DDC_STATUS);
>>
>> Replace the following lines with:
>>
>> if (hdcp_ddc_status & BIT(16))
>> pr_info("%s: Unable to clear HDCP DDC Failure\n", __func__);
>>
>> No need to print the debug statement either...
>>
>>> +               hdcp_ddc_status = (hdcp_ddc_status >> 16) & BIT(0);
>>> +               if (hdcp_ddc_status == 0x0)
>>> +                       DBG("HDCP DDC Failure cleared");
>>> +               else
>>> +                       pr_info("%s: Unable to clear HDCP DDC
>>> Failure\n",
>>> +                               __func__);
>>> +
>>> +               /* Re-Enable HDCP DDC */
>>> +               hdmi_write(hdmi, REG_HDMI_HDCP_DDC_CTRL_0, 0);
>>> +       }
>>> +
>>> +       if (nack0 == 0x1) {
>>> +               DBG("Before: HDMI_DDC_SW_STATUS=0x%08x",
>>> +                       hdmi_read(hdmi, REG_HDMI_DDC_SW_STATUS));
>>> +               /* Reset HDMI DDC software status */
>>> +               hdmi_write(hdmi, REG_HDMI_DDC_CTRL,
>>> +                       hdmi_read(hdmi, REG_HDMI_DDC_CTRL) | BIT(3));
>>
>> Split all these in:
>> val = hdmi_read()
>> val |= foo
>> hdmi_write(val);
>>
>> To make this readable.
>>
>>> +               msleep(20);
>>> +               hdmi_write(hdmi, REG_HDMI_DDC_CTRL,
>>> +                       hdmi_read(hdmi, REG_HDMI_DDC_CTRL) &
>>> ~(BIT(3)));
>>> +
>>> +               /* Reset HDMI DDC Controller */
>>> +               hdmi_write(hdmi, REG_HDMI_DDC_CTRL,
>>> +                       hdmi_read(hdmi, REG_HDMI_DDC_CTRL) | BIT(1));
>>> +               msleep(20);
>>> +               hdmi_write(hdmi, REG_HDMI_DDC_CTRL,
>>> +                       hdmi_read(hdmi, REG_HDMI_DDC_CTRL) & ~BIT(1));
>>> +               DBG("After: HDMI_DDC_SW_STATUS=0x%08x",
>>> +                       hdmi_read(hdmi, REG_HDMI_DDC_SW_STATUS));
>>> +       }
>>> +
>>
>> Just end the function here, no need for the extra debug printouts...
>>
>>> +       hdcp_ddc_status = hdmi_read(hdmi, REG_HDMI_HDCP_DDC_STATUS);
>>> +
>>> +       failure = (hdcp_ddc_status >> 16) & BIT(0);
>>> +       nack0 = (hdcp_ddc_status >> 14) & BIT(0);
>>> +       DBG("On Exit: HDCP_DDC_STATUS=0x%x, FAIL=%d, NACK0=%d",
>>> +               hdcp_ddc_status, failure, nack0);
>>> +}
>
> DBG() stuff can always be enabled at runtime.. and when it comes to
> failures seen with certain monitors, it is usually the monitor the
> user has and not the ones the developer has, which have fun bugs ;-)
>
> So in general if it is something useful for debugging a failure, when
> you can just ask the user to 'echo 15 >
> /sys/modules/drm/parameters/debug' then plug in monitor and send
> dmesg, I don't mind keeping it.
>
> BR,
> -R
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/